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 a check to ensure we have a valid PHP version before loading plugin functionality and output an admin notice if needed #210

Merged
merged 6 commits into from
Sep 19, 2023

Conversation

kmgalanakis
Copy link
Contributor

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

  • To see the admin notice and have the plugin functionality not loaded, try to load the plugin on a site with PHP version lower that 7.4.

Changelog Entry

Added - Check for minimum required PHP version before loading the plugin

Credits

Props @kmgalanakis

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@kmgalanakis kmgalanakis requested a review from a team as a code owner August 14, 2023 07:56
@kmgalanakis kmgalanakis requested review from peterwilsoncc and removed request for a team August 14, 2023 07:56
@kmgalanakis kmgalanakis mentioned this pull request Aug 14, 2023
@jeffpaul jeffpaul added this to the 1.1.0 milestone Aug 14, 2023
…in functionality and output an admin notice if needed
@kmgalanakis kmgalanakis force-pushed the feature/add-php-checks branch from eac6ee5 to d96e05c Compare August 18, 2023 05:21
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a 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-

*
* @return string Minimum version required.
*/
function minimum_php_requirement(): string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

*
* @return bool True if meets minimum requirements, false otherwise.
*/
function site_meets_php_requirements(): bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function site_meets_php_requirements(): bool {
function site_meets_php_requirements() {

As above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

);
return;
}

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

<?php
echo wp_kses_post(
sprintf(
/* translators: %s: Minimum required PHP version */
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@kmgalanakis
Copy link
Contributor Author

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-

Done @peterwilsoncc thank you

@kmgalanakis kmgalanakis force-pushed the feature/add-php-checks branch 2 times, most recently from 0081529 to 2e21681 Compare September 15, 2023 15:25
@jeffpaul
Copy link
Member

@peterwilsoncc back to you for review/testing

@jeffpaul jeffpaul removed their request for review September 18, 2023 13:17
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a 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

@peterwilsoncc
Copy link
Contributor

E2E tests are failing on the develop branch for the same versions of WP, I presume due to markup changes.

Merging in.

@peterwilsoncc peterwilsoncc merged commit c3dc156 into develop Sep 19, 2023
@peterwilsoncc peterwilsoncc deleted the feature/add-php-checks branch September 19, 2023 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add PHP checks
3 participants