WordPress.org

Make WordPress Core

Opened 9 months ago

Last modified 7 weeks ago

#50787 accepted task (blessed)

Consolidate the logic for displaying WP & PHP compatibility messages for themes

Reported by: SergeyBiryukov Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Site Health Keywords: needs-testing needs-patch
Focuses: administration Cc:

Description (last modified by SergeyBiryukov)

Background: #48491

As part of the changes on [48636-48638,48640], these messages are duplicated in at least seven different places:

  • "This theme doesn’t work with your versions of WordPress and PHP. Please update WordPress, and then learn more about updating PHP."
  • "This theme doesn’t work with your version of WordPress. Please update WordPress."
  • "This theme doesn’t work with your version of PHP. Learn more about updating PHP."

See comment:36:ticket:48491 for the list of relevant areas.

It would be nice to consolidate the logic in one place.

Attachments (6)

50787.diff (28.4 KB) - added by afragen 6 months ago.
50787.2.diff (28.7 KB) - added by afragen 6 months ago.
rename function and use static variable for performance
50787-with-php56-before.png (141.1 KB) - added by hellofromTonya 3 months ago.
Before: On "Add Plugins" UI with PHP 5.6 and with WooCommerce (which requires PHP 7+)
50787-with-php56-after.png (136.5 KB) - added by hellofromTonya 3 months ago.
After: On "Add Plugins" UI with PHP 5.6 and with WooCommerce (which requires PHP 7+)
50787-theme-php-incompatible-before.png (657.0 KB) - added by hellofromTonya 3 months ago.
Before: Theme PHP incompatibility when on PHP 5.6 and theme requires at least PHP 7
50787-theme-php-incompatible-after.png (655.1 KB) - added by hellofromTonya 3 months ago.
After: Theme PHP incompatibility when on PHP 5.6 and theme requires at least PHP 7

Download all attachments as: .zip

Change History (30)

#1 @SergeyBiryukov
9 months ago

  • Milestone changed from Future Release to 5.6

#2 @SergeyBiryukov
9 months ago

  • Description modified (diff)

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


6 months ago

#4 @SergeyBiryukov
6 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#5 @hellofromTonya
6 months ago

  • Keywords needs-patch added

Per Sergey during today's scrub:

Needs a patch, I'll try to get to it before beta, or punt otherwise.

@afragen
6 months ago

#6 @afragen
6 months ago

  • Keywords has-patch dev-feedback needs-testing added; needs-patch removed

50787.diff is an initial pass at this ticket.

This ticket was mentioned in Slack in #core by afragen. View the logs.


6 months ago

#8 @hellofromTonya
6 months ago

A couple of code review notes:

Function naming

For the new function name, it's not clear to me what the function will do when invoked. I can infer. But I'm wondering if it would be more clear if action-based such as

wp_get_compatibility_string

to tell us that it "gets" and returns a single string. What do you think @afragen ?

Performance

Each time the function is called, the $strings array rebuilds. This means each __() has to run again, ie there are 12 of them. Is this necessary? Do we need to retranslate and rebuild the array each time the function is invoked?

We can use memoization for a tiny peformance boost.

Last edited 6 months ago by hellofromTonya (previous) (diff)

#9 @hellofromTonya
6 months ago

Revising my performance note above. See slack conversation.

The last 3 string elements are dynamic, i.e. build from the injected $name. if no name is passed to the function, then we can skip building those last 3 elements.

For the non-dynamic strings, we can leverage memoization to generate the $strings array once and then reuse the same array each time the function is invoked.

It's a tiny tiny performance boost.

Last edited 6 months ago by hellofromTonya (previous) (diff)

@afragen
6 months ago

rename function and use static variable for performance

#10 @hellofromTonya
6 months ago

@SergeyBiryukov Can you please do a final review on the latest patch Andy and I did? What do you think?

#11 @afragen
6 months ago

@SergeyBiryukov I think we're ready for a final review.

Thanks @hellofromTonya 🤗

This ticket was mentioned in PR #607 on WordPress/wordpress-develop by hellofromtonya.


6 months ago

  • Keywords has-unit-tests added

Trac ticket: https://core.trac.wordpress.org/ticket/50787

Props: @afragen

#13 @SergeyBiryukov
6 months ago

  • Type changed from enhancement to task (blessed)

Thanks for the patch!

Didn't get a chance to review this in time for 5.6 Beta 1, but I think it can still go in before RC, since it's more of a cleanup than new functionality. Converting to a task for now.

This ticket was mentioned in Slack in #core-site-health by clorith. View the logs.


5 months ago

#15 @SergeyBiryukov
5 months ago

  • Milestone changed from 5.6 to 5.7

Didn't get a chance to review this in time for 5.6 RC, moving to the next release for now.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 months ago

#17 @hellofromTonya
3 months ago

  • Keywords dev-feedback removed

This ticket is ready for code review.

#18 @hellofromTonya
3 months ago

For manually testing this patch:

  • Get a plugin or theme that is not compatible with the WP version running in your local env
  • Check that incompatible message appears

For example, plugin incompatible message:

This plugin doesn't work with your versions of WordPress and PHP.

This ticket was mentioned in Slack in #core by francina. View the logs.


3 months ago

@hellofromTonya
3 months ago

Before: On "Add Plugins" UI with PHP 5.6 and with WooCommerce (which requires PHP 7+)

@hellofromTonya
3 months ago

After: On "Add Plugins" UI with PHP 5.6 and with WooCommerce (which requires PHP 7+)

#20 follow-up: @SergeyBiryukov
3 months ago

Thanks for the patch!

Just noting that 50787.2.diff is a good start, as it touches all the relevant areas, but is not quite what I envisioned for this ticket.

I would like to consolidate not just the strings (that doesn't seem to reduce much code), but also the repeated logic for these messages, i.e. various conditionals for adjusting them based on user capabilities and what actually needs updating. Sorry if that was not clear from the description.

Last edited 3 months ago by SergeyBiryukov (previous) (diff)

@hellofromTonya
3 months ago

Before: Theme PHP incompatibility when on PHP 5.6 and theme requires at least PHP 7

@hellofromTonya
3 months ago

After: Theme PHP incompatibility when on PHP 5.6 and theme requires at least PHP 7

#21 @hellofromTonya
3 months ago

PHP Incompatibility Testing Strategy

How to test the PR

Pull the PR down to your local machine

git fetch origin pull/607/head:pr607
git checkout pr607

Step 2: Change the LOCAL_PHP=5.6-fpm in .env.

Step 3: Reprovision

Step 4: Log into the admin area as an admin

Testing the plugin's PHP incompatibility message:

We'll attempt to add a plugin that requires PHP 7+ while your local dev environment is running PHP 5.6.

Step 5: Go to the "Plugins" and then "Add Plugins" screen

Step 6: Type "WooCommerce" in the search plugins field. Press enter/return.

The incompatibility notice should appear across the top of the plugin:

This plugin doesn’t work with your version of PHP. Learn more about updating PHP.

Testing the theme's PHP incompatibility message:

Let's repeat this test with a theme that requires at least PHP 7.

Step 7: Go to the "Themes" and then "Add Theme" screen

Step 8: Type "Charitas Lite" in the search themes field. Press enter/return.

The incompatibility notice should appear across the top of the plugin:

This theme doesn’t work with your version of PHP. Learn more about updating PHP.

#22 in reply to: ↑ 20 @hellofromTonya
3 months ago

  • Keywords needs-patch added; has-patch has-unit-tests removed

Removing has-patch and has-unit-tests and adding needs-patch to continue iterating on the solution per Sergey's review.

Replying to SergeyBiryukov:

Thanks for the patch!

Just noting that 50787.2.diff is a good start, as it touches all the relevant areas, but is not quite what I envisioned for this ticket.

I would like to consolidate not just the strings (that doesn't seem to reduce much code), but also the repeated logic for these messages, i.e. various conditionals for adjusting them based on user capabilities and what actually needs updating. Sorry if that was not clear from the description.

This ticket was mentioned in Slack in #core by paaljoachim. View the logs.


3 months ago

#24 @hellofromTonya
7 weeks ago

  • Milestone changed from 5.7 to Future Release

With 5.7 RC1 landing tomorrow and no activity on this ticket for a new patch, punting this ticket to Future Release.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

Note: See TracTickets for help on using tickets.