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

Add custom PHPCS sniff to warn against usage of empty() #1786

Conversation

ShyamGadde
Copy link
Contributor

Summary

Fixes #1219

Relevant Technical Choices

The dominant-color-images, performance-lab, and webp-uploads plugins have been ignored by the custom PHPCS sniff configuration, similar to the approach used for PHPStan.

@westonruter westonruter added [Type] Enhancement A suggestion for improvement of an existing feature no milestone PRs that do not have a defined milestone for release skip changelog PRs that should not be mentioned in changelogs labels Jan 10, 2025
@ShyamGadde ShyamGadde marked this pull request as ready for review January 10, 2025 08:24
Copy link

github-actions bot commented Jan 10, 2025

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ShyamGadde <[email protected]>
Co-authored-by: swissspidy <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: felixarntz <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@swissspidy
Copy link
Member

Why do we need to write and maintain a custom PHPCS sniff for this when https://github.com/phpstan/phpstan-strict-rules already catches that just fine? 🤔

@westonruter
Copy link
Member

Right, the disallowedEmpty rule from phpstan-strict-rules should do what we need. And this is already being applied, with the exceptions being for the same plugins excluded in this PR.

-
# 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

@ShyamGadde
Copy link
Contributor Author

That’s correct—PHPStan does show the following error:

Construct empty() is not allowed. Use more strict comparison.

Given that phpstan-strict-rules already handles this effectively, this PR doesn’t seem necessary.

Thank you for pointing this out! I’ll go ahead and close it now.

westonruter

This comment was marked as outdated.

@ShyamGadde ShyamGadde closed this Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no milestone PRs that do not have a defined milestone for release skip changelog PRs that should not be mentioned in changelogs [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add checks to disallow use of empty()
3 participants