Make WordPress Core

Opened 16 months ago

Closed 14 months ago

Last modified 13 months ago

#58894 closed enhancement (fixed)

Enhance `wp_get_loading_optimization_attributes()` to support arbitrary context values

Reported by: flixos90's profile flixos90 Owned by: mukesh27's profile mukesh27
Milestone: 6.4 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch, has-unit-tests, has-dev-note
Focuses: performance Cc:

Description (last modified by flixos90)

The wp_get_loading_optimization_attributes() function, which was introduced in 6.3 (see #58235) but inherited most of its logic from the now deprecated wp_get_loading_attr_default() function introduced in 5.5, relies on a $context parameter based on which it may alter its behavior and the attributes returned. At the moment, it only supports context values used within WordPress core.

While this made sense for a first implementation, it is technically not required and could easily be expanded in several areas to support custom context values. Having to use WordPress core contexts is limiting today when relying on the function in plugins or themes, as at the moment any custom context used would result in incorrect handling of the attributes. This leads to the awkward situation where we currently have to recommend always using one of the core context values, even when the function is actually used in a different context (see e.g. the dev note post which recommends using the wp_get_attachment_image context even though that function is never called in the example). An actual example for the problem is this theme pull request which, despite looking correct, would lead to incorrect behavior as of today.

We should abstract certain clauses in the function away from specific contexts so that the function can also be correctly used with custom contexts.

This ticket's implementation should be considered blocked by #58891 as making the changes before that would be unnecessarily complicated and error-prone.

Change History (18)

#1 @flixos90
16 months ago

  • Description modified (diff)

#2 @flixos90
15 months ago

With [56347] committed, this effort is now unblocked. Please refer to the latest codebase for any changes to wp_get_loading_optimization_attributes().

#3 @oglekler
15 months ago

  • Keywords needs-patch added

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


14 months ago

This ticket was mentioned in Slack in #core-performance by pereirinha. View the logs.


14 months ago

#6 @mukesh27
14 months ago

@flixos90, could you please provide more details about the problem you were trying to address with this ticket? I'm having difficulty fully understanding it. Thanks!

#7 @flixos90
14 months ago

@mukesh27 Currently, the logic in wp_get_loading_optimization_attributes() is heavily tied to specific context values. For example, if you want to display a custom image in the header to be enforced without loading="lazy", you would need to pass the get_header_image_tag context, even if the image is not in fact the header image. Or, if I display an image within the main query loop, it only works correctly if I use one of the predefined contexts like the_post_thumbnail, even if I'm not displaying a featured image. So the limitation of specific contexts is confusing and inflexible, and mostly, unnecessary.

So technically speaking, this ticket is fundamentally about replacing the two big switch clauses with more generally applicable logic that is mostly decoupled from specific contexts.

More specifically, here is what I would propose:

  • Replace the switch clause for the early return if doing_filter( 'the_content' ) with a more general clause: It should return early if the current context is anything other than 'the_content' while doing_filter( 'the_content' ).
  • Replace the bigger switch clause with a set of if elseif clauses that ignores context, except for the "header enforced contexts":
    • Define a list with "header enforced contexts", which by default should only have 'template_part_' . WP_TEMPLATE_PART_AREA_HEADER and 'get_header_image_tag'. If the current context is one of them, run the logic that is currently within that 1st case group. That could be the first if condition.
    • Then use the exact logic from the 2nd case group (i.e. if ( ! is_admin() && in_the_loop() && is_main_query() ) { ...) as elseif.
    • Then use the exact logic from the 3rd case group (i.e. if ( $wp_query->before_loop ...) as elseif.

That way, all the logic, except for the "header enforced contexts" one becomes context unaware, allowing more flexibility for plugins.

Something that needs to be ensured while making these changes is that all existing tests must still pass without modifications.

Last edited 14 months ago by flixos90 (previous) (diff)

#8 @mukesh27
14 months ago

  • Owner set to mukesh27
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


14 months ago

This ticket was mentioned in PR #5197 on WordPress/wordpress-develop by @mukesh27.


14 months ago
#10

  • Keywords has-patch added; needs-patch removed

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

I have marked this pull request as a draft due to some test failures that occurred after implementing the changes.

It's crucial to ensure that all the existing tests continue to pass without any modifications while making these changes. As mentioned by @felixarntz earlier here, I am unable to update any unit tests, but the failures seem to be related to certain context values that need adjustment.

Here are lists of tests that going fails.

  1. test_wp_get_loading_optimization_attributes
  2. test_content_rendering_with_shortcodes
  3. test_content_rendering_with_shortcodes_nested

@mukesh27 commented on PR #5197:


14 months ago
#11

cc. @felixarntz @joemcgill

@mukesh27 commented on PR #5197:


14 months ago
#12

Thank you, @felixarntz, for your valuable review feedback. I have taken the necessary steps to address the comments, and the PR is now prepared for further review. cc. @joemcgill.

#13 @mukesh27
14 months ago

  • Keywords has-unit-tests added

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


14 months ago

#15 @flixos90
14 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 56612:

Media: Enhance wp_get_loading_optimization_attributes() to support arbitrary context values.

The wp_get_loading_optimization_attributes() function, which was introduced in 6.3, based on the now deprecated wp_get_loading_attr_default() function introduced in 5.5, relies on a $context parameter based on which it may alter its behavior and the attributes returned. So far, it has only supported context values used within WordPress core.

This changeset decouples the behaviors of the function from specific contexts, allowing for more flexibility. Theme and plugin developers will be able to rely on their own context values when rendering images in non-standard ways, rather than being forced to use a core context, to get the loading optimization benefits the function provides.

As part of this change, a wp_loading_optimization_force_header_contexts filter is introduced, which allows filtering the map of context values and whether they should be considered header contexts, i.e. i.e. any image having one of these contexts will be assumed to appear above the fold.

Props mukesh27, costdev, flixos90.
Fixes #58894.

#17 @flixos90
14 months ago

  • Keywords needs-dev-note added

The 4 tickets related to wp_get_loading_optimization_attributes() should receive a single dev note post explaining those changes.

#18 @flixos90
13 months ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.