Add a check to ensure we have a valid PHP version before loading plugin functionality and output an admin notice f needed #135
Conversation
kmgalanakis
requested review from
peterwilsoncc
and removed request for
a team
kmgalanakis
force-pushed
the
feature/add-php-checks
branch
3 times, most recently
from
bb38784
to
5ea47da
Compare
…in functionality and output an admin notice if needed
kmgalanakis
force-pushed
the
feature/add-php-checks
branch
from
5ea47da
to
6975365
Compare
There was a problem hiding this comment.
A couple of notes about keeping the main file compatible with PHP 5.6 as the plugin supports WP 5.7 and later.
In the linting workflow it would be good to add some compatibility checks:
- for the entire plugin, check that it's compatible with PHP 7.4 and later
- for the main file only, check that it's compatible with PHP 5.6 and later
insecure-content-warning.php
Outdated
| <?php | ||
| echo wp_kses_post( | ||
| sprintf( | ||
| /* translators: %s: Minimum required PHP version */ |
There was a problem hiding this comment.
Alignment nit: one extra tab to align with line below.
insecure-content-warning.php
Outdated
| * | ||
| * @return string Minimum version required. | ||
| */ | ||
| function minimum_php_requirement(): string { |
There was a problem hiding this comment.
| function minimum_php_requirement(): string { | |
| function minimum_php_requirement() { |
This is to keep the file compatible with PHP 5.6 (as it supports versions of WP prior to 6.3)
There was a problem hiding this comment.
(sorry, wrong profile)
insecure-content-warning.php
Outdated
| * | ||
| * @return bool True if meets minimum requirements, false otherwise. | ||
| */ | ||
| function site_meets_php_requirements(): bool { |
There was a problem hiding this comment.
| function site_meets_php_requirements(): bool { | |
| function site_meets_php_requirements() { |
There was a problem hiding this comment.
(sorry, wrong profile)
@peterwilsoncc Curious on your reasoning for this? I get that we support WordPress 5.7+ which itself supports PHP 5.6+ but we're also saying we only support PHP 7.4+. While I can see this may be looked at as a conflict (how can we truly claim to support WP 5.7 if we don't support PHP 5.6) I'm personally fine with us pushing users to new PHP versions even if they're running older WP versions. Is there worry that someone may be running PHP 5.6 and also be running this plugin, even though the plugin only supports PHP 7.4+? I guess to me feels like extra work to try and ensure the main plugin file always stays PHP 5.6 compatible for what is hopefully an edge case (someone running PHP 5.6 that happens to be able to bypass WordPress' update/install requirements and ends up with this plugin installed) |
|
@dkotter There are two main reasons:
I agree it's an edge case but as it's relatively trivial to protect against I think it's a good idea to do so. |
|
Good point @dkotter, thank you for your feedback |
kmgalanakis
force-pushed
the
feature/add-php-checks
branch
from
f377108
to
7b9c550
Compare
kmgalanakis
force-pushed
the
feature/add-php-checks
branch
2 times, most recently
from
12648bb
to
ba41914
Compare
…he rest of the plugin
kmgalanakis
force-pushed
the
feature/add-php-checks
branch
from
ba41914
to
256eda3
Compare
|
@peterwilsoncc back to you for review/testing |
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 #129
How to test the Change
Changelog Entry
Credits
Props @kmgalanakis
Checklist: