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.

Sign up for GitHub

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

Jump to bottom

Add checks to disallow use of empty() #1219

Closed
westonruter opened this issue May 16, 2024 · 6 comments · Fixed by #1803
Closed

Add checks to disallow use of empty() #1219

westonruter opened this issue May 16, 2024 · 6 comments · Fixed by #1803
Assignees
@ShyamGadde
Labels
Infrastructure Issues for the overall performance plugin infrastructure [Type] Enhancement A suggestion for improvement of an existing feature

Comments

@westonruter
Copy link
Member

westonruter commented May 16, 2024 •
edited
Loading

Feature Description

The use of empty() and isset( $var ) can mask code problems (e.g. typos). And they rarely need to be used, for example:

cf. comment by @felixarntz in #1091 (comment)

We should consider adding sniffs to warn against their use. (Granted, PHPStan should catch the problematic uses of isset() and empty().)
@westonruter westonruter added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure labels May 16, 2024
@swissspidy
Copy link
Member

https://github.com/phpstan/phpstan-strict-rules Is your friend.

Works like a charm

@westonruter
Copy link
Member Author

But I said we were done with PHPStan PRs 😂

@westonruter westonruter mentioned this issue May 23, 2024
Merged
13 tasks
@westonruter westonruter changed the title Add sniffs to disallow use of isset() and empty() Add checks to disallow use of isset() and empty() May 23, 2024
@westonruter westonruter added this to the performance-lab 3.2.0 milestone May 23, 2024
@westonruter westonruter mentioned this issue Jun 3, 2024
Merged
This was referenced Jul 24, 2024
Merged
Merged
@westonruter westonruter mentioned this issue Jul 30, 2024
Open
@westonruter westonruter removed this from the performance-lab n.e.x.t milestone Aug 13, 2024
@github-project-automation github-project-automation bot moved this to Not Started/Backlog 📆 in WP Performance 2025 Aug 13, 2024
@westonruter westonruter added this to the performance-lab n.e.x.t milestone Aug 13, 2024
@westonruter westonruter mentioned this issue Aug 21, 2024
Merged
@felixarntz
Copy link
Member

Per #1489 (comment), let's change this issue to be only about empty() - or, if we want to include isset(), it should only be prevented on variable checks like isset( $something ), but not on e.g. array key or object property checks like isset( $arr['something'] ).

@westonruter westonruter changed the title Add checks to disallow use of isset() and empty() Add checks to disallow use of isset( $var ) and empty() Aug 21, 2024
@westonruter westonruter removed this from the performance-lab n.e.x.t milestone Sep 20, 2024
@westonruter westonruter changed the title Add checks to disallow use of isset( $var ) and empty() Add checks to disallow use of empty() Nov 13, 2024
@westonruter westonruter moved this from Not Started/Backlog 📆 to To Do 🔧 in WP Performance 2025 Nov 13, 2024
@westonruter westonruter mentioned this issue Nov 13, 2024
Merged
@westonruter westonruter mentioned this issue Nov 20, 2024
Merged
@ShyamGadde ShyamGadde mentioned this issue Jan 9, 2025
Closed
@westonruter westonruter moved this from To Do 🔧 to In Progress 🚧 in WP Performance 2025 Jan 10, 2025
@westonruter
Copy link
Member Author

Since strict rules are now applied with disallowedEmpty with exceptions for three plugins:

performance/phpstan.neon.dist

Lines 68 to 75 in b6e5c5a

-
# TODO: Remove this to fix https://github.com/WordPress/performance/issues/1219
identifier: empty.notAllowed
paths:
- */tests/*
- plugins/dominant-color-images
- plugins/performance-lab
- plugins/webp-uploads

Either we go ahead and close this issue as complete or we eliminate the use of empty() in those plugins.

@ShyamGadde
Copy link
Contributor

If we were to consider eliminating the use of empty(), would you prefer addressing it in a single PR or a separate one for each plugin?

@westonruter
Copy link
Member Author

One PR is fine for me. But if the changes end up being too massive, then separate PRs would make sense too.

@ShyamGadde ShyamGadde mentioned this issue Jan 16, 2025
Merged
@westonruter westonruter moved this from In Progress 🚧 to Code Review 👀 in WP Performance 2025 Jan 28, 2025
@github-project-automation github-project-automation bot moved this from Code Review 👀 to Done 😃 in WP Performance 2025 Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ShyamGadde ShyamGadde

Labels
Infrastructure Issues for the overall performance plugin infrastructure [Type] Enhancement A suggestion for improvement of an existing feature
Projects
WP Performance 2025
Status: Done 😃
Milestone
No milestone
4 participants
@westonruter @swissspidy @felixarntz @ShyamGadde