WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 9 days ago

#50787 accepted task (blessed)

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

Reported by: SergeyBiryukov Owned by: SergeyBiryukov
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Site Health Keywords: has-patch dev-feedback needs-testing has-unit-tests
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 (2)

50787.diff (28.4 KB) - added by afragen 4 weeks ago.
50787.2.diff (28.7 KB) - added by afragen 2 weeks ago.
rename function and use static variable for performance

Download all attachments as: .zip

Change History (15)

#1 @SergeyBiryukov
3 months ago

  • Milestone changed from Future Release to 5.6

#2 @SergeyBiryukov
3 months ago

  • Description modified (diff)

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


4 weeks ago

#4 @SergeyBiryukov
4 weeks ago

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

#5 @hellofromTonya
4 weeks 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
4 weeks ago

#6 @afragen
4 weeks 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.


2 weeks ago

#8 @hellofromTonya
2 weeks 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 2 weeks ago by hellofromTonya (previous) (diff)

#9 @hellofromTonya
2 weeks 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 2 weeks ago by hellofromTonya (previous) (diff)

@afragen
2 weeks ago

rename function and use static variable for performance

#10 @hellofromTonya
2 weeks ago

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

#11 @afragen
2 weeks 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.


2 weeks ago

  • Keywords has-unit-tests added

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

Props: @afragen

#13 @SergeyBiryukov
9 days 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.

Note: See TracTickets for help on using tickets.