Performance improvement: Reduce use of _wp_array_get
#5244
Conversation
aristath
force-pushed
the
try/_wp_array_get
branch
from
f6efc5b
to
8584c59
Compare
|
@aristath thanks for the PR. Could you please share the performance number before after the changes? |
Trunk: After this PR: So on my test, we're saving 2582 call to the |
|
The coding standards disallow using the null coalescing operator right now, see https://core.trac.wordpress.org/ticket/58874 |
The coding standards do no such thing: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/ The coding standards have no opinion on whether you use it or not. It's a matter of the minimum PHP version. As for that Trac ticket: that's about whether to do a bulk update of the Core code base for it (and possibly enforce its use), which is a different matter. |
|
Thank you @swissspidy 👍 Do you think it would be better if I change these to be |
|
My bad, just glanced over it on mobile. Thanks for correcting me! |
|
As this feels like a code qualiaty thing, I am going to ping @SergeyBiryukov |
aristath
force-pushed
the
try/_wp_array_get
branch
from
8584c59
to
9ed797f
Compare
|
Rebased the PR, resolving conflicts 👍 |
| $background_image_source = _wp_array_get( $block_attributes, array( 'style', 'background', 'backgroundImage', 'source' ), null ); | ||
| $background_image_url = _wp_array_get( $block_attributes, array( 'style', 'background', 'backgroundImage', 'url' ), null ); | ||
| $background_size = _wp_array_get( $block_attributes, array( 'style', 'background', 'backgroundSize' ), 'cover' ); | ||
| $background_image_source = $block_attributes['style']['background']['backgroundImage']['source'] ?? null; |
There was a problem hiding this comment.
There is not yet consensus to use ?? in Core. See https://core.trac.wordpress.org/ticket/58874. IMHO I think this should not yet to be introduced until its Trac ticket has consensus to move forward. Then the work here can be updated as part of that ticket.
There was a problem hiding this comment.
Thank you for the feedback @hellofromtonya!
That ticket is about converting all existing isset( $var ) ? $var : 'default' checks in Core to use ?? where appropriate. It has nothing to do with this PR - which removes unnecessary calls to _wp_array_get. It should not be a part of that ticket... the fact that both PRs use ?? is conincidental. We could convert this PR to use isset() checks and it would be just as valuable and valid as it is now.
The fact that we don't yet use ?? anywhere in Core does not mean that it's not allowed, or that it should not be used. When WordPress had to support PHP 5.6 we could not use it because we had to be compatible with that PHP version. However, "could not use" is not the same as "WordPress does not allow".
The ??operator is a part of the PHP language - just like == or &&. The only difference is the supported PHP versions, something which is no longer blocking us.
|
@hellofromtonya if the use of Would that help us move this ticket forward? We can convert |
|
Thank you all for the feedback! I pushed a commit, changing the syntax so it doesn't use |
|
Thanks for the PR! Merged in r56709. |
This is a backport of WordPress/gutenberg#51116 from Gutenberg
Trac ticket: https://core.trac.wordpress.org/ticket/59405
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.