Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the @ silencing warnings from PL #709

Closed
mukeshpanchal27 opened this issue Apr 10, 2023 · 3 comments · Fixed by #1231
Closed

Remove the @ silencing warnings from PL #709

mukeshpanchal27 opened this issue Apr 10, 2023 · 3 comments · Fixed by #1231
Labels
Infrastructure Issues for the overall performance plugin infrastructure [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only [Type] Enhancement A suggestion for improvement of an existing feature

Comments

@mukeshpanchal27
Copy link
Member

Bug Description

Followup for #695

As @joemcgill suggested in other conversation we have to remove @ silencing warnings from all files.

cc. @felixarntz

@mukeshpanchal27 mukeshpanchal27 added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure labels Apr 10, 2023
@felixarntz
Copy link
Member

@mukeshpanchal27 @joemcgill One thing I'd like to highlight here for context is that all of the @ usages in Performance Lab are there because closely related code in WordPress core uses them. So we need to review these usages individually and decide whether or not they are worth fixing:

  • All @ usages in perflab_get_modules() are there because the function was inspired by WordPress core's get_plugins(). I am unsure why the @ usage is there in WordPress core, but we should probably figure that out before we simply remove our own.
  • Note that there is a // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged comment in the Dominant_Color_Image_Editor_Imagick::get_dominant_color() method, however that seems misplaced there. Maybe something old that was forgotten to be removed?

@joemcgill
Copy link
Member

Thanks for the context, @felixarntz. I know that WP Core has gone through a lengthy process of identifying and removing error silencing from a bunch of places in the past few years, so it would be interesting to know if these were not updated in get_plugins() intentionally, or if they just haven't gotten around to reviewing these yet.

It looks like there are currently [14 instances of opendir() across 9 files in core](https://github.com/search?q=repo%3AWordPress%2Fwordpress-develop%20opendir(&type=code) and they inconsistently use error silencing, so it's probably fine for us to remove them given that we're always checking for the existence of the module file in order to load it, so a warning is probably appropriate in that case.

Regardless, I don't see this as I high priority issue for us to address. More of a nice maintenance cleanup.

@felixarntz felixarntz added the [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only label Jul 19, 2023
@mukeshpanchal27
Copy link
Member Author

  • All @ usages in perflab_get_modules() are there because the function was inspired by WordPress core's get_plugins(). I am unsure why the @ usage is there in WordPress core, but we should probably figure that out before we simply remove our own.

As we removed the module infrastucture we no longer needs to check for PL plugin codebase as we remove those code now.

  • Note that there is a // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged comment in the Dominant_Color_Image_Editor_Imagick::get_dominant_color() method, however that seems misplaced there. Maybe something old that was forgotten to be removed?

@pbearne Could you tell us which error you want to by pass here? As we are set min 7.2 now can we remove that PHPCS comment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Issues for the overall performance plugin infrastructure [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants