#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 | Owned by: | 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.
20 months ago
#1
- Keywords has-patch added
#2
@
20 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:
20 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
@
20 months ago
This changes seems similar to this. Does it relate?
AS @oandregal says, this change needs to be made in gutenberg first.
@flixos90 commented on PR #4380:
20 months ago
#6
@oandregal Makes sense. I just opened https://github.com/WordPress/gutenberg/pull/50266 :)
#7
@
20 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.
@flixos90 commented on PR #4380:
20 months ago
#8
Closing this in favor of https://github.com/WordPress/gutenberg/pull/50266
@isabel_brison commented on PR #4380:
18 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:
18 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
@
18 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.
@flixos90 commented on PR #4380:
18 months ago
#14
Committed in https://core.trac.wordpress.org/changeset/55950.
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 additionalif
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 ofWP_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