-
Notifications
You must be signed in to change notification settings - Fork 15
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 a check to ensure we have a valid PHP version before loading plugin functionality and output an admin notice if needed #210
Conversation
…in functionality and output an admin notice if needed
eac6ee5
to
d96e05c
Compare
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.
I've added some notes about keeping the main file compatible with 5.6 while moving the functionality in to another file.
In .github/workflows/php-compatibility.yml
I suggest modifying the compatibility checks to:
- name: Run PHP Compatibility on all files.
run: vendor/bin/phpcs inc --standard=PHPCompatibilityWP --extensions=php --runtime-set testVersion 7.4-
- name: Run PHP Compatibility on main file.
run: vendor/bin/phpcs insert-special-characters.php --standard=PHPCompatibilityWP --extensions=php --runtime-set testVersion 5.6-
insert-special-characters.php
Outdated
* | ||
* @return string Minimum version required. | ||
*/ | ||
function minimum_php_requirement(): string { |
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.
function minimum_php_requirement(): string { | |
function minimum_php_requirement() { |
The PHP version check needs to run in PHP 5.6+ (as we support pre 6.3 versions of WP).
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.
Done
insert-special-characters.php
Outdated
* | ||
* @return bool True if meets minimum requirements, false otherwise. | ||
*/ | ||
function site_meets_php_requirements(): bool { |
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.
function site_meets_php_requirements(): bool { | |
function site_meets_php_requirements() { |
As above.
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.
Done
); | ||
return; | ||
} | ||
|
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.
I suggest moving everything below this point in to /inc/plugin.php
or similar.
This will allow the team to keep this file compatible with older versions of PHP while using newer PHP features in the rest of the plugin. If everything is kept in the one file, we risk syntax errors causing a fatal error and preventing the compatibility check from running.
You can see the approach in the Distributor main plugin file.
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.
Done
insert-special-characters.php
Outdated
<?php | ||
echo wp_kses_post( | ||
sprintf( | ||
/* translators: %s: Minimum required PHP version */ |
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.
Nit: increase indentation of this line.
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.
Done
…nality in a separate file
Done @peterwilsoncc thank you |
0081529
to
2e21681
Compare
…he rest of the plugin
2e21681
to
501dc62
Compare
@peterwilsoncc back to you for review/testing |
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.
LGTM, Thank you!
Testing notes:
- WP 6.2, PHP 5.6: Warning shows
- WP 6.2, PHP 7.4: Warning does not show
E2E tests are failing on the develop branch for the same versions of WP, I presume due to markup changes. Merging in. |
Description of the Change
In this PR, I'm adding a check for the minimum required PHP version before attempting to load the plugin files. If the condition is not met an admin notice is displayed and the plugin files are not loaded.
Closes #205
How to test the Change
Changelog Entry
Credits
Props @kmgalanakis
Checklist: