Opened 14 months ago
Closed 12 months ago
#59168 closed defect (bug) (fixed)
Block API: Unnecessary JSON decoding in block parser
Reported by: | dlh | Owned by: | dmsnell |
---|---|---|---|
Milestone: | 6.4 | Priority: | normal |
Severity: | normal | Version: | 5.0 |
Component: | Editor | Keywords: | has-patch gutenberg-merge |
Focuses: | Cc: |
Description
The WP_Block_Parser::$empty_attrs
property is initialized with:
$this->empty_attrs = json_decode( '{}', true );
which produces an empty array. json_decode()
isn't needed to create this array; a plain array()
is equivalent. See https://3v4l.org/M1Qie.
Some background that I can find: This logic predates the original Gutenberg merge in WordPress 5.0. It was introduced in GB PR 10107. In the original implementation, the JSON was decoded into an object, not an array. It was updated to its current form in GB PR 11434.
In the linked PR, I've removed the function call from the property initialization. I've also made a couple fixes to the documentation. First, I removed the inline comment about creating "an empty associative array," which I think was targeted at the change in GB PR 10107 and made less relevant by the update in PR 11434. Second, to update some @since
tags to WordPress core versions, not Gutenberg.
Change History (18)
This ticket was mentioned in PR #5049 on WordPress/wordpress-develop by dlh01.
14 months ago
#1
- Keywords has-patch added
@spacedmonkey commented on PR #5049:
14 months ago
#2
#4
@
14 months ago
@dlh It seems this code comes from the gutenberg repo here - https://github.com/WordPress/gutenberg/blob/e2237b8ffd98c3211af6bbff3b642fb9682e29c8/packages/block-serialization-default-parser/class-wp-block-parser.php#L76.
If you make a PR against gutenberg, I will approve and merge.
#6
@
13 months ago
- Milestone changed from Future Release to 6.4
PR: ready at https://github.com/WordPress/gutenberg/pull/54424
#7
@
13 months ago
Interesting update @dlh.
By the way @spacedmonkey I was in the process of reviewing this when it was merged, so please in the future if you are willing, give me some notice to review changes where I'm listed as the code owner, or ask if I plan on reviewing them. This one took some extra consideration to verify.
The heart of the issue is on json_encode()
and not on json_decode()
, but since false
for $associative
is what makes the difference, I'm wondering if historically it was a mistake here to use true
, thus giving the impression that this was superfluous.
That is, json_decode( '{}', false )
is not equivalent to array()
. That was true at the time GB#11434 merged and it's true today. I think that the augmentation in the unit tests is covering up the change, but since everything didn't crash, it's probably safe as is.
That being said, while it seems safe to apply this change now, it's incomplete and it would be great if you would be willing to finish by updating the spec parser and refactoring the block parser to avoid using $empty_attrs
entirely. Now that there's no more trace of the distinction in the code, it's almost silly to continue using this quirk.
There's certainly no measurable impact on removing the json_decode()
.
So I'd be interested in seeing a follow-up that removes the use of $empty_attrs
and associated documentation, and instead creates in-place array()
. If we choose to stick with $empty_attrs
we still need to update the associated documentation.
This is a fine patch, but it needed a few more steps before merging. Let's finish the job 👍
#8
@
13 months ago
@dmsnell I will wait next time. Avoid using json_decode
fixes the performance problem for me.
I put together a little pr to remove this attribute - https://github.com/WordPress/gutenberg/pull/54496
#9
@
13 months ago
Thanks for the review, @dmsnell!
I wondered the same thing about the use of true
as I went through the old PRs. Given that the current code has been in place since the merge into core, I figured the ship had sailed.
I'm happy to try to update the spec parser, but I'm not familiar with it. Can you provide a little more guidance about what needs to be updated?
Removing the use of $empty_attrs
seems a little more questionable to me. It's a public property on a non-final class, and the block_parser_class
filter makes it possible to use an extended version of that class that depends on the property. Arguably, both removing the use of the property within the class and removing the property itself are backwards-incompatible changes. If nothing else, the property still provides the code a bit of a semantic boost.
#10
@
13 months ago
Thanks @spacedmonkey - by the way, if I'm unresponsive on an issue I totally understand moving forward, so thanks for helping out on this.
the performance problem
What performance problem are we talking about?
I figured the ship had sailed.
@dlh same thought. I think there's a chance this only ever exposed itself in the tests themselves, as everything else was happy with the loose types.
I'm happy to try to update the spec parser, but I'm not familiar with it. Can you provide a little more guidance about what needs to be updated?
The spec parser (in the block-serialization-spec-parser/grammar.pegjs
file) leans on peg_empty_attrs()
to provide this same value. We can probably just remove that function and replace all calls to it with array()
.
Removing the use of $empty_attrs seems a little more questionable to me. It's a public property on a non-final class, and the block_parser_class filter makes it possible to use an extended version of that class that depends on the property.
It's probably fine. I doubt anything is relying on it: no public plugins are plugin search for "empty_attrs"
Arguably, both removing the use of the property within the class and removing the property itself are backwards-incompatible changes.
This is true, and again, probably fine. We can deprecate it if we really want to. That would involve changing the internal code while leaving the attribute in place, adding a deprecation notice in the docblock comment.
the property still provides the code a bit of a semantic boost.
🤷♂️ this seems arguable to me at best. it was added originally to address a quirk, and array()
is common enough that it would be surprising if it confused people that it's supposed to represent anything other than an empty array.
#12
@
13 months ago
- Keywords gutenberg-merge added
- Owner set to dmsnell
- Status changed from new to assigned
This change needs to be backported from gutenberg, so marked as "gutenberg-merge". Assigned to @dmsnell to own, as he is the code owner in gutenberg.
#13
@
13 months ago
Assigned to @dmsnell to own, as he is the code owner in gutenberg.
Ha! Didn't stop you before 😄
Anyway, so I have reference, what performance issue did this resolve? I'm still struggling to imagine how it could have any measurable impact on performance or be meaningful in any way. Would love to get the answer to this question.
Also please note that I can't commit any backport because I don't have commit access, so we'll still need other people involved. @spacedmonkey are you asking me to recreate the patch on the WordPress/wordpress-develop side? it's unclear to me what you are expecting by assigning me, so I would be grateful if you could clarify that.
#14
@
13 months ago
@spacedmonkey I apologize for my poor comment above. it wasn't meant to be rude, just a small tease (and not about this ticket at all), but not helpful. Thanks for calling me out on it.
The other stuff I'm still confused about, so please let me know what you expect from me on this.
#15
@
13 months ago
- Focuses performance removed
I think it's safe to say that this change doesn't resolve any performance issues, and it's not meaningful in any way. I've submitted a PR that updates the spec parser here: https://github.com/WordPress/gutenberg/pull/54770.
#16
@
13 months ago
My turn to apologize for the nastiness in my comment. As the person who added the performance
tag to this ticket, I decided to remove it because I wanted to shift this discussion away from a debate about whether it qualifies as a performance improvement and toward whatever steps remain toward closing the ticket. I'm sorry to both of you for not expressing that constructively.
@spacedmonkey commented on PR #5049:
13 months ago
#17
Fixed upstream.
@dlh01 I like this change. But I would remove the changes to docs. We can do this elsewhere and it does not need to block this change.