Block API: Unnecessary JSON decoding in block parser

[dlh]

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.

@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.

@spacedmonkey OK, I removed the docs changes and will submit them separately.

[spacedmonkey]

@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.

[spacedmonkey]

PR: ready at ​https://github.com/WordPress/gutenberg/pull/54424

[dmsnell]

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.

​https://3v4l.org/T4fKB

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 👍

[spacedmonkey]

@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

[dlh]

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.

[dmsnell]

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.

This change was applied to Gutenberg in WordPress/gutenberg#54424.
Follow-up is taking place in WordPress/gutenberg#54496.

Should this be done separately or come through via an update to the Gutenberg packages? cc: @mcsf

[spacedmonkey]

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.

[dmsnell]

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.

[dmsnell]

@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.

[dlh]

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.

[dlh]

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.

Fixed upstream.

[hellofromTonya]

Hello all,

In reviewing, this issue was reported and fixed upstream in Gutenberg. As the changes are within the packages that Gutenberg maintains (and Core consumes), the fix was included in today's package updates [56945] being tracked in #59411.

Closing this ticket as fixed by [56945].