Make WordPress Core

Opened 15 months ago

Closed 13 months ago

Last modified 12 months ago

#58193 closed enhancement (fixed)

Introduce `WP_Theme_JSON::prepend_to_selector()` in favor of parameter to conditionally modify `append_to_selector()` behavior

Reported by: flixos90's profile flixos90 Owned by: flixos90's profile flixos90
Milestone: 6.3 Priority: normal
Severity: normal Version: 5.8
Component: Editor Keywords: has-patch gutenberg-merge commit
Focuses: performance Cc:

Description

Originally, the WP_Theme_JSON::append_to_selector() method was introduced solely to append something to a selector, which was perfectly fine, but in 6.1 a new parameter to also allow prepending was added.

This is most notably a code smell, using a parameter to modify what a function or method does is a discouraged pattern. Technically speaking, this results in lots of additional if checks on every call to the method, which could easily be avoided, since at the time of calling the method the code knows already whether to append or prepend something. Note that while this is generally a very cheap method, it is typically called over 1,000 times in a single page load when using a block theme.

Change History (16)

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


15 months ago
#1

  • Keywords has-patch added

This PR contains a minor performance improvement related to the WP_Theme_JSON::append_to_selector() method. Originally, the method was solely used to append something to a selector, which was perfectly fine, but in 6.1 a new parameter to also allow prepending was added. This is most notably a code smell, using a parameter to modify what a function or method does is a discouraged pattern. Technically speaking, this results in lots of additional if checks on every call to the method, which could easily be avoided, since at the time of calling the method the code knows already whether to append or prepend something. Note that while this is generally a very cheap method, it is typically called over 1,000 times in a single page load when using a block theme.

For that reason, this PR introduces a corresponding WP_Theme_JSON::prepend_to_selector() method in favor of the 3rd parameter of WP_Theme_JSON::append_to_selector(), which gets silently deprecated. Since this is a protected method of a relatively new class that is not intended to be extended, we can probably save ourselves the conditional _deprecated_argument() call (which may even have a tiny negative performance impact since this method is called so many times in a single page load).

In terms of performance, this is a very marginal enhancement that is likely so small that the benchmark-server-timing command cannot identify a clear win, due to the typical variance between results, which is higher than the benefits this change would bring. Yet, from a code perspective it's a cleaner pattern and it certainly does not regress performance, so I am opening this PR for consideration.

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

#2 @flixos90
15 months ago

cc @oandregal @spacedmonkey for consideration

This is honestly extremely minor when it comes to performance, but I recently ran into this code smell and thought we may want to adjust it.

@oandregal commented on PR #4380:


15 months ago
#3

Hi Felix, would you be able to send this PR to Gutenberg? That's where the source of truth for this code lives.

This makes sense from a "code wise" perspective to me. It'd be good to engage the original authors/reviewers for help as well, so the pattern is more widely shared.

#4 @spacedmonkey
15 months ago

This changes seems similar to this. Does it relate?

AS @oandregal says, this change needs to be made in gutenberg first.

#5 @spacedmonkey
15 months ago

@flixos90 Do you have profile or benchmark data?

@flixos90 commented on PR #4380:


15 months ago
#6

@oandregal Makes sense. I just opened https://github.com/WordPress/gutenberg/pull/50266 :)

#7 @flixos90
15 months ago

@spacedmonkey This doesn't result in a visible performance improvement, it's more about the code pattern used.

Note that I opened this as a Gutenberg enhancement now in https://github.com/WordPress/gutenberg/pull/50266, we should treat that as source of truth.

@isabel_brison commented on PR #4380:


13 months ago
#9

Hi! I see the respective change in Gutenberg has been merged, but unless I'm mistaken this PR will still be needed to make the update in core? Shall we reopen it?

@flixos90 commented on PR #4380:


13 months ago
#10

Thanks @tellthemachines, good point. I thought this was gonna be backported together with #4412, but I've reopened this PR and updated it against trunk as well as the minor changes in order to match the Gutenberg implementation that was merged.

#11 @flixos90
13 months ago

  • Keywords 2nd-opinion removed
  • Milestone changed from Awaiting Review to 6.3
  • Owner set to flixos90
  • Status changed from new to assigned

Milestoning for 6.3 as a backport of the merged Gutenberg change https://github.com/WordPress/gutenberg/pull/50266.

#12 @spacedmonkey
13 months ago

  • Keywords gutenberg-merge commit added

#13 @flixos90
13 months ago

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

In 55950:

Editor: Introduce WP_Theme_JSON::prepend_to_selector() to improve code quality and performance.

The WP_Theme_JSON::append_to_selector() method was previously used for both appending and prepending which violated the single responsibility principle. It resulted in additional conditionals which also came at a performance cost, particularly because the method is called over 1,000 times during a regular WordPress request.

With the new WP_Theme_JSON::prepend_to_selector() method, there are now two distinct methods for the two distinct purposes. The now useless third parameter on WP_Theme_JSON::append_to_selector() has been removed (rather than deprecated), which is acceptable given that it is a protected method on a class that is not intended for extensions.

Props bor0, costdev, flixos90, isabel_brison, oandregal, spacedmonkey.
Fixes #58193.
See #58457.

#15 @milana_cap
12 months ago

  • Keywords needs-dev-note added

#16 @milana_cap
12 months ago

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