Podcast Player: handle dual-behaviour of render_callback fn

[retrofox]

This PR handles a bug that happens in 7.9.1 version of Gutenberg. These changes were introduced in this PR.

In terms of the functionality and how it affects the Podcast Player, what the PR changed is the value behavior of the first argument the callback render function. Now it supports a dual-behavior which could be a WP_Block object instance, or simply the $attributes array.

It means that you can pick up the block name through of the object property, for instance:

function render_block( $block ) {
  echo $block->name;
};

Or use this param as an array (block attributes):

function render_block( $attributes ) {
  echo $attributes['showEpisodeDescription'];
};

It works as expected as long as the argument is treated explicitly as an array or as an object. The issue happens when we use this value expecting it acts as the attributes array (the current, and unique, the behavior of it so far) but it acts as a WP_Block object instance.

It happens in parts of our code. When it encodes the data to be consumed bt the client, and when it extracts the array items to be used in the server-side templates.

To deal with this, what I suggest is checking if the $attributes is in fact an array, and also checks if the attributes property is defined. Take a look at the code to get the idea:

Changes proposed in this Pull Request:

• Deal with dual-behavior of render_callback function introduced in Block API: Add Block Context support WordPress/gutenberg#21467

Testing instructions:

1. activate Gutenberg 7.9.1 or later
2. Confirm that it doesn't generate a PHP notice when tries to pick up some block attributes. Without these changes, it should trigger something like the following:

[21-Apr-2020 16:28:07 UTC] PHP Notice:  Undefined index: showCoverArt in /var/www/html/wp-content/plugins/jetpack/extensions/blocks/podcast-player/templates/podcast-header.php on line 25

Also, you could print the $attributes on the server side for different versions of Gutenberg. It should shown something like the following:

Array
(
    [url] => https://distributed.blog/category/podcast/feed/
    [primaryColor] => accent
    [hexPrimaryColor] => #cd2653
    [customSecondaryColor] => #1b092e
    [hexSecondaryColor] => #1b092e
    [backgroundColor] => subtle-background
    [hexBackgroundColor] => #dcd7ca
    [itemsToShow] => 5
    [showCoverArt] => 1
    [showEpisodeDescription] => 1
)

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello retrofox! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D42246-code before merging this PR. Thank you!
This revision will be updated with each commit to this PR

[jetpackbot]

	Warnings
⚠️	"Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against debd495

[talldan]

The goal is for WP_Block class to be backwards compatible with the $attributes array, so ideally this code wouldn't have to change.

It happens in parts of our code. When it encodes the data to be consumed bt the client

I did a quick bit of research and an option might be for WP_Block to implement the JsonSerializable interface in the same way that it implements ArrayAccess:
https://www.php.net/manual/en/class.jsonserializable.php

That would allow this part of the block's code to continue working.

it extracts the array items to be used in the server-side templates.

I'm not completely familiar with how this works, but it seems like an unsafe practice to pass all the attributes into extract given the warnings in the docs (https://www.php.net/manual/en/function.extract.php):

Warning Do not use extract() on untrusted data, like user input

While it might be fine with the current set of attributes, it doesn't seem clear to anyone extending this block in the future that adding additional attributes could have consequences.

That said, it's interesting that the intended backwards compatibility for WP_Block doesn't seem to work here (cc @noisysocks, @aduth).

[retrofox]

Thanks, @talldan for your feedback.

That said, it's interesting that the intended backwards compatibility for WP_Block doesn't seem to work here

It won't work in all cases where the callback argument is called implicitly, without trying to access to either a property (defines object behavior) or tries to access an item (array behavior). The default behavior will be as a class instance.

I did a quick bit of research and an option might be for WP_Block to implement the JsonSerializable interface in the same way that it implements ArrayAccess:
https://www.php.net/manual/en/class.jsonserializable.php

That's smart. Something to integrate in core, right?

I'm not completely familiar with how this works, but it seems like an unsafe practice to pass all the attributes into extract given the warnings in the docs

Yes, right. it's something that we would consider to change?

While it might be fine with the current set of attributes, it doesn't seem clear to anyone extending this block in the future that adding additional attributes could have consequences.

We can improve the documentation in order to let folks be aware of this.

[aduth]

I did a quick bit of research and an option might be for WP_Block to implement the JsonSerializable interface in the same way that it implements ArrayAccess:
https://www.php.net/manual/en/class.jsonserializable.php

It's an interesting idea. I guess it speaks to a naivety in thinking that the value was only being used in very predictable array access patterns. It would be interesting to see how far down this road we'd have to go to preserve expected behaviors, both in terms of coverage possible and the remaining risk.

For example, another one could be Iterator, in case someone would be doing a foreach over the $block_attributes value. 🤷

[retrofox]

For example, another one could be Iterator, in case someone would be doing a foreach over the $block_attributes value. 🤷

That sounds like something that would happen more often than the cases that appeared on this issue.

[aduth]

For example, another one could be Iterator, in case someone would be doing a foreach over the $block_attributes value. 🤷

That sounds like something that would happen more often than the cases that appeared on this issue.

I'll probably plan to make an issue or pull request in Gutenberg. I was experimenting with this some more today. There's quite a few other interfaces to consider as well (Serializable, Countable). It can get quite close to feeling exactly like an array, but notably array methods still don't work. One I could anticipate being quite common and very problematic is a pattern like array_key_exists( 'some_attribute_name', $block_attributes );. I don't know that we can implement this to retain backward-compatibility.

[retrofox]

array_key_exists( 'some_attribute_name', $block_attributes );. I don't know that we can implement this to retain backward-compatibility.

🤚 Pretty sure Navigation block checks the attribute key using array_key_exists

[retrofox]

array_key_exists( 'some_attribute_name', $block_attributes );. I don't know that we can implement this to retain backward-compatibility.

🤚 Pretty sure Navigation block checks the attribute key using array_key_exists

confirmed :-/

[jeherve]

This is triggering quite a few notices for me now:

Undefined index: showEpisodeDescription
 Type: PHP Notice Line: 25
 File: wp-content/plugins/jetpack-dev/extensions/blocks/podcast-player/templates/podcast-header.php

Undefined index: showCoverArt
 Type: PHP Notice Line: 24
 File: wp-content/plugins/jetpack-dev/extensions/blocks/podcast-player/templates/podcast-header.php

Undefined variable: block_attributes
 Type: PHP Notice Line: 94
 File: wp-content/plugins/jetpack-dev/extensions/blocks/podcast-player/podcast-player.php

[obenland]

cf9902c fixed the block for me.

[aduth]

I created an issue at WordPress/gutenberg#21797 to track the problem and potential solutions. I also created a pull request at WordPress/gutenberg#21798 which explores potential further enhancements to this approach to backward-compatibility. I am hopeful to find some time tomorrow to be able to dedicate to this.

It may be worth stating: Because of the incompatibilities observed here, I don't have high confidence that the changes implemented in WordPress/gutenberg#21467 (at least in how they impact render_callback) will ship as-is for next week's Gutenberg 8.0 release. Mentioning in case that has an impact on whether you'd want to merge these changes.

[aduth]

This PR handles a bug that happens in 7.9.1 version of Gutenberg. These changes were introduced in this PR.

To clarify, the behavior implemented in WordPress/gutenberg#21467 is not part of Gutenberg 7.9.1, and not (yet) part of any public stable release of the plugin. It is only present in the master development branch of the plugin.

[retrofox]

To clarify, the behavior implemented in WordPress/gutenberg#21467 is not part of Gutenberg 7.9.1, and not (yet) part of any public stable release of the plugin. It is only present in the master development branch of the plugin.

Thanks for the updates 👍

[aduth]

WordPress/gutenberg#21868 was merged, which should restore the original behavior of the first argument being passed as a true associative array. In other words, you shouldn't need the changes here anymore.

[retrofox]

WordPress/gutenberg#21868 was merged, which should restore the original behavior of the first argument being passed as a true associative array. In other words, you shouldn't need the changes here anymore.

I was trying to reproduce the problem and I couldn't. Now I understand why. Thanks, Andrew!

[retrofox]

Closing because of this.