Make WordPress Core

Opened 10 months ago

Closed 10 months ago

Last modified 7 months ago

#58891 closed enhancement (fixed)

Simplify logic in `wp_get_loading_optimization_attributes()`

Reported by: flixos90's profile flixos90 Owned by: flixos90's profile flixos90
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, contains quite complex and hard-to-follow logic. While early returns are usually a good pattern, with the excessive mix of individual if-clauses and early returns, the function is hard to follow and thus complicated to fully understand, which makes it challenging for maintenance.

In order to decentralize the knowledge and understanding from just the few people that were involved in its implementation so far, this ticket aims to rewrite the function using a more coherent approach, which avoids most of the early returns and potentially uses a switch clause for the different context values.

While refactoring on its own is generally discouraged in WordPress core, this refactoring will benefit, simplify, or even enable several other tickets:

It should however not be implemented as part of any of those tickets, since that could easily lead to oversights due to the complex logic. The goal of this ticket should be to refactor the logic in a way that all existing tests pass without any modifications. Coverage is quite extensive on this function (at least that!), so this is a good framework to follow here.

Change History (7)

#1 @flixos90
10 months ago

  • Description modified (diff)

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

10 months ago

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

This PR refactors the wp_get_loading_optimization_attributes() function to make it easier to follow and maintain. It removes all the early returns in favor of a more procedural and sequential single execution paths, with the only early returns remaining being those for which the function should _actually_ return early, as in not return anything / make any modifications to the respective tag. Additionally, several comments were added to help clarify the logic. Furthermore, with the sequential flow the function follows now, we no longer need the closures that were previously used to avoid duplicate code.

As mentioned on the ticket, the goal of this is purely to enhance maintainability of the code without making any functional change. Given the substantial test coverage for this function that already exists in core (either with direct tests for wp_get_loading_optimization_attributes() or indirect tests for wp_filter_content_tags()), all we need to ensure here is that these tests all pass without modification. While there are probably a few areas that we can improve (I noticed a few myself while working on this, and there are already separate tickets for some things), let's avoid any behavioral change in this PR to focus on purely refactoring without breaking anything.

There's a single test change made here, which was for an overly strict test that unnecessarily relied on a specific order for the attributes returned, which does not actually matter.

Trac ticket:

@flixos90 commented on PR #4912:

10 months ago

@joemcgill @kt-12 Would love to get your review on this.

#4 @flixos90
10 months ago

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

In 56347:

Media: Simplify logic in wp_get_loading_optimization_attributes().

While the wp_get_loading_optimization_attributes() function was only recently introduced in 6.3, its code was mostly ported over from the now deprecated wp_get_loading_attr_default() function introduced in 5.5.

That function started out in a simple way, but over time was expanded with more and more conditionals on when to avoid lazy-loading, which ended up making the logic extremely complex and hard to follow.

This changeset refactors the logic to simplify it, in a way that allows to follow it more sequentially, and without making any functional changes, ensuring that the extensive existing unit test coverage still passes. This will facilitate future enhancements to the function to be less error-prone and make it more accessible to new contributors.

Props flixos90, joemcgill.
Fixes #58891.

#6 @flixos90
8 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.

#7 @flixos90
7 months ago

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