-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Chore Update phpcs config to test php 5.6 compatibility instead of 5.2 #16674
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this should only be done by also bumping the minimum required PHP and WordPress versions in the readme. The former is currently missing, the latter is set to WordPress 5.1.0.
Otherwise there will be PHP 5.6 code changes that will crash people's WordPress 5.1 sites.
Hi @swissspidy, than you for the review. The readme file is part of the SVN repository, but not part of this repository on Github. I guess the minimum required PHP version and the minimum WordPress version should be updated during the next plugin release? |
Yep :) I just re-opened #6004 to consider adding the readme to this repo. There's really no reason why it can't be part of it. Otherwise it's easy to forget these kind of things. I also left a comment on #16356 explaining why requiring a higher PHP version for the closures there doesn't make sense. Overall, however, I think it makes sense to require WordPress 5.2 (and thus PHP 5.6). |
Problem referred was meanwhile solved
Hi @swissspidy, the minimum WordPress version is now 5.2 and I opened another PR #18828 to move readme and changelog files to the repository. |
@jorgefilipecosta This line was already changed in #15809. So I think we can close this PR? |
Yes @Soean you are right. Thank you for bringing this 👍 |
Description
This changes phpcs to verify compatibility against 5.6 instead of PHP 5.2.
It was extracted from #16356 and it is required to merge that PR.