Add an admin notice about the upcoming change in PHP requirements #31557

Konamiman · 2022-01-04T16:33:38Z

All Submissions:

Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

The minimum required PHP version will be 7.2 as of WooCommerce 6.5. This pull request adds a dismissable admin notice visible when PHP 7.0 or 7.1 is used:

The notice is created (if needed) when the theme is switched or a new version of WooCommerce is installed.

TODO: Add the url of the announcement post in the link included in the notice.

How to test the changes in this Pull Request:

Run the following in command line to simulate a new WooCommerce version installed, this will also attempt to clear some user meta that doesn't exist yet:

wp eval 'WC_Admin_Notices::reset_admin_notices(); delete_user_meta(get_current_user_id(), "dismissed_php72_required_in_woo_65_notice");' --user=<your WP user name or id>

Switch to PHP 7.2 and load any admin page, e.g. orders. You shouldn't see any notice.
Switch to PHP 7.1, repeat step 1, and load the admin page again. You should see the notice.
Switch to PHP 7.2, reload the admin page (DON'T repeat step 1 this time), and you should see that the notice has disappeared.
Switch to PHP 7.1, repeat step 1, reload the admin page, and click the "Dismiss" button. The notice shouldn't appear anymore unless you repeat step 1.

1+2 simulates a user that was already on PHP 7.2+ at the time of upgrading to the WooCommerce release that includes this pull request, nothing will happen for these users; 3+4 simulates a user that upgrades his PHP without dismissing the notice; and 3+5 simulates a user that dismisses the notice without (or before) upgrading his PHP.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully run tests with your changes locally?

Changelog entry

Add - Admin notice warning about the upcoming minimum PHP version bump

FOR PR REVIEWER ONLY:

I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

The minimum required PHP version will be 7.2 as of WooCommerce 6.5. This adds a dismissable admin notice to PHP 7.0 and 7.1 users.

ObliviousHarmony · 2022-01-04T19:13:13Z

@Konamiman it looks like this functionality already exists. Could we instead look at updating this notice and reset it for those who have already dismissed it, rather than create a new notice?

beaulebens · 2022-01-06T00:50:41Z

Can we tweak the wording of this to make it explicit to the merchant/user that this change will impact them (vs it just being some informational thing to ignore)?

Maybe change the "An upgrade to at least PHP 7.4 is recommended" part to something like "Your server is currently running an older version of PHP, so this change will impact your store. Upgrading to at least PHP 7.4 is recommended".

Konamiman · 2022-01-07T08:59:13Z

@ObliviousHarmony I think these serve different purposes. The intent of the original notice was actually to inform about the recommended PHP version for WooCommerce, and that's why it was implemented long before we decided about the bump; but indeed the wording seems to suggest a hard requirement.

Given that we don't do a PHP version requirement bump often, I think that a "disposable" notice with whatever wording and links we need in each case is the way to go. I think that what we can do too (after Woo 6.5) is to make the original notice more clear and use it to recommend an upgrade to 7.4.

- Update the link to the announcement post - Extend the notice text with "Your server is currently running an older version of PHP, so this change will impact your store".

Konamiman · 2022-01-07T09:29:00Z

@beaulebens Text updated with your suggestion, see the updated screenshot.

peterfabian · 2022-01-11T09:47:16Z

Related: #23975

peterfabian

Thanks, works fine, I just have one note regarding security of unrelated code.

peterfabian · 2022-01-11T09:34:51Z

plugins/woocommerce/includes/admin/class-wc-admin-notices.php

 	 * @deprecated 4.6.0
 	 */
 	public static function install_notice() {
+		// phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped


The third parameter is the replacement. I think

we shouldn't ignore the escaping if we're passing a translated string to it, but

we shouldn't actually pass anything as that's not a replacement function, no?

Indeed, this looks like an attempt to convey some extra information in a way that isn't really supported by the function (but no real harm done since this parameter is only used for printing error messages).

plugins/woocommerce/includes/admin/class-wc-admin-notices.php

ObliviousHarmony

@Konamiman To be fair, given how strong the wording was, as well as the name of the notice (wp_php_min_requirements_notice), I do think that the original intention is not only lost but probably irrelevant. Personally, I'd still prefer we adjust the wording of the notice and reset the dismissal in a migration, but there's probably nothing wrong with a duplicate notice. I do have one performance concern with this implementation, so I've left a comment.

plugins/woocommerce/includes/admin/class-wc-admin-notices.php

peterfabian

Looks good, works fine as far as I can see. There are some question marks around which actions to use to make things efficient, but this will go away in 3 versions, so let's take the safer route.

github-actions · 2022-01-13T09:14:27Z

Hi @peterfabian, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

Add the status: needs changelog label
Add the status: needs testing instructions label

Add an admin notice about the upcoming change in PHP requirements

860a8b9

The minimum required PHP version will be 7.2 as of WooCommerce 6.5. This adds a dismissable admin notice to PHP 7.0 and 7.1 users.

Konamiman self-assigned this Jan 4, 2022

Update class-wc-admin-notices.php

3ce4a7c

jonathansadowski marked this pull request as ready for review January 4, 2022 19:04

Changes in the notice for the PHP 7.2 bump:

d3036be

- Update the link to the announcement post - Extend the notice text with "Your server is currently running an older version of PHP, so this change will impact your store".

Konamiman requested review from a team and peterfabian and removed request for a team January 10, 2022 10:13

peterfabian reviewed Jan 11, 2022

View reviewed changes

ObliviousHarmony reviewed Jan 11, 2022

View reviewed changes

plugins/woocommerce/includes/admin/class-wc-admin-notices.php Show resolved Hide resolved

Konamiman added 2 commits January 13, 2022 08:57

Tryy to remove th PHP 7.2 bump notice only if it actually exists.

9821d5d

Fix a "__" that should be "esc_html__" (triggering deprecation notice)

0853bf4

Konamiman force-pushed the add-php-72-version-bump-notice branch from 80bc066 to 0853bf4 Compare January 13, 2022 08:57

peterfabian approved these changes Jan 13, 2022

View reviewed changes

peterfabian merged commit 28f6760 into trunk Jan 13, 2022

peterfabian deleted the add-php-72-version-bump-notice branch January 13, 2022 09:14

github-actions bot added this to the 6.2.0 milestone Jan 13, 2022

Konamiman mentioned this pull request Jan 27, 2022

Fix: double "PHP version upgrade required" notice #31744

Merged

7 tasks

Konamiman mentioned this pull request Mar 28, 2022

Remove the admin notice about the upcoming PHP 7.2 requirement #32376

Merged

7 tasks

Konamiman mentioned this pull request Jan 16, 2023

Add an admin notice about the upcoming change in PHP requirements (PHP 7.3) #36444

Merged

8 tasks

Add an admin notice about the upcoming change in PHP requirements #31557

Add an admin notice about the upcoming change in PHP requirements #31557

Uh oh!

Conversation

Konamiman commented Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

Changelog entry

FOR PR REVIEWER ONLY:

Uh oh!

ObliviousHarmony commented Jan 4, 2022

Uh oh!

beaulebens commented Jan 6, 2022

Uh oh!

Konamiman commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Konamiman commented Jan 7, 2022

Uh oh!

peterfabian commented Jan 11, 2022

Uh oh!

peterfabian left a comment

Choose a reason for hiding this comment

Uh oh!

peterfabian Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

Konamiman Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ObliviousHarmony left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peterfabian left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Konamiman commented Jan 4, 2022 •

edited

Loading

Konamiman commented Jan 7, 2022 •

edited

Loading