Block API: Block context render_callback not fully backward-compatible as array

[aduth]

Previously: #21467
Related: Automattic/jetpack#15515

As part of the implementation of block context in #21467, the function signature of render_callback was revised in such a way that was intended to be fully backward-compatible. It did so by creating a custom block class which implemented the PHP ArrayAccess interface, where attempting to access array members of the first argument would be proxied automatically to block attribute members, to simulate the same effect of the previous signature wherein an array of block attributes was passed. The intention was to seamlessly allow the argument to be used either as an instance of the block class, or as an array of a block's attributes.

Problem:

While implementing ArrayAccess is expected to satisfy a majority of common usage, it has become obvious that it does not faithfully uphold full backward compatibility. Some of these are rare and extreme edge cases, while others may be more common to anticipate.

Examples:

• In Podcast Player: handle dual-behaviour of render_callback fn Automattic/jetpack#15515, changes were necessary where a block was using extract and json_encode on the argument value. The latter can be accommodated by further implementing the PHP JsonSerializable interface, but the former cannot be supported.
• Array methods would not accept the class directly. A common problem may be a case where a block implementation checks to see if an attribute member is set in the array. empty( $block_attributes['foo'] ) and isset( $block_attributes['foo'] ) would work, but notably array_key_exists( 'foo', $block_attributes ) would not work after the changes in Block API: Add Block Context support #21467.

Possible Solutions:

Some options to consider include:

• Accept these as consequences of the changes in Block API: Add Block Context support #21467, ideally with clear documentation and devnotes including as seamless an upgrade path as possible for these sorts of usages.
  • Optionally, extend additional default interfaces, becoming more like ArrayObject in implementing IteratorAggregate, ArrayAccess, Serializable, JsonSerializable, Countable
    • See: Block API: WP_Block implements JsonSerializable, Serializable, IteratorAggregate, Countable #21798 (example implementation)
• Change the implementation of Block API: Add Block Context support #21467, most likely in a way which requires the developer to opt in to receiving the block instance.
  • Implement a separate render method which accepts the block instance.
    • Example: render_callback_with_block( $block ) (qualified variant of existing name)
    • Example: render( $block ) (new name)
  • Implement a property of settings which acts as a flag to control the behavior
    • Example: 'render_callback_receives_block' => true
  • If possible, reflect on the render_callback callable to consider if it specifies a type hint in its parameter signature.
    • Example: render_callback( WP_Block $block ) vs. render_callback( $block_attributes )
    • Related:
      • https://www.php.net/manual/en/functions.arguments.php#functions.arguments.type-declaration
      • https://www.php.net/manual/en/class.reflectionfunction.php
      • https://www.php.net/manual/en/reflectionfunctionabstract.getparameters.php
• Return to earlier implementations considered
  • Make $block instance available as a global contextualized to the render_callback, but don't otherwise change the function signature.
    • Example: Early iterations of Block API: Add Block Context support #21467
  • Make $block available as a third argument of render_callback
    • Example: https://core.trac.wordpress.org/ticket/48104

Personal preference: I'm most keen to see if there are options around method reflection, in order to avoid the addition of a new settings property while giving the developers the choice to explicitly opt in to the behavior of receiving the block instance. It is not clear to me yet if this is a viable option.

Action Plan:

If a decision amongst the above (or additional) options cannot be reached and implemented before this Friday, April 24, then #21467 should be reverted to allow more time to consider the decision, or at least parts which affect the public API of render_callback.

[epiqueras]

I agree with your personal preference, and we should also implement as many array related interfaces as possible.

ReflectionParameter has a getType method. It looks doable.

[epiqueras]

Actually, if the interfaces were just for background compatibility, we can just drop all of them since now people are explicitly opting in.

[talldan]

I agree with the idea of using an opt-in to WP_Block. Adding all those interfaces to WP_Block seems like it'd make the implementation unnecessarily complicated, and it'd still be hard to guarantee absolute backwards compatibility.

[noisysocks]

Bugger! It's never as easy as it seems, is it! 🙂

I agree with @talldan. I don't think the convenience of re-using render_callback is worth a very complex implementation. I also don't particularly like the idea of using reflection as, to me, it feels "unWordPressy" to rely on reflection. Our PHP codebase is very what you see is what you get, warts and all.

The simplest way to implement an "opt in" is to pick a new name.

Example: render( $block ) (new name)

This is my preference.

[epiqueras]

I do like the idea of dropping _callback. It's a bit redundant. Just render would match up better with React class components too.

[aduth]

I opened #21833 as an exploration to see how function reflection can solve this problem. It's quite straight-forward. In fact, the pull request removes more code than it adds.

@noisysocks As to your concern about using reflection: I understand it. It does seem more "magical" than might be typical in WordPress code. I think I've generally been of the mind to optimize toward simple and obvious from the perspective of those integrating (the public API), even if the internal implementation needs to be more complex to support it. In this case, the public API difference is function( WP_Block $block ) {} vs. function( $block_attributes ) {}. It feels pretty nice and intuitive to me from that angle as an explicit opt in to this behavior. Furthermore, if you consider this as a deprecation, an argument can be made that this is very much a "WordPress way", in that any other project might be inclined to just break backward-compatibility. While we've had to go out of our way to do so, the proposed implementation is the way it is largely for the sake of preserving this compatibility. If we'd be comfortable to make a breaking change, reflection would not be necessary here.

To the thought about changing the name, a few extra considerations:

• Over time, I've started to see this less about being a deprecation in that: A majority of blocks may not need anything more than just the block attributes, so making those available conveniently should be an optimization to consider. The current render_callback excels at this because it's giving the array directly. If we pass the $block instance, it's less direct ($block->attributes['foo']). And if we give people the choice, where the choice is between render_callback and render, the difference in the names themselves leaves a lot of room for ambiguity (why is one receiving $block and the other $block_attributes?). I'd really only be most comfortable if we buy into this as being a deprecation, and accept that attributes are less convenient to access than they were before.
• The _callback convention has some prior art, which I believe is why it was used in the first place. Notably WP_REST_Controller (callback, sanitize_callback, validate_callback, permission_callback`).

[noisysocks]

@noisysocks As to your concern about using reflection: I understand it. It does seem more "magical" than might be typical in WordPress code. I think I've generally been of the mind to optimize toward simple and obvious from the perspective of those integrating (the public API), even if the internal implementation needs to be more complex to support it. In this case, the public API difference is function( WP_Block $block ) {} vs. function( $block_attributes ) {}. It feels pretty nice and intuitive to me from that angle as an explicit opt in to this behavior.

I'm still worried 😀

My big concern is that, in all the PHP literature (ex, ex), type hinting is advertised as an optional feature that allows the programmer to enforce some level of type safety.

I don't think that a typical PHP programmer would expect that turning a function( T $x ) {} into a function( $x ) {} would change the behaviour of their program.

Furthermore, if you consider this as a deprecation, an argument can be made that this is very much a "WordPress way", in that any other project might be inclined to just break backward-compatibility. While we've had to go out of our way to do so, the proposed implementation is the way it is largely for the sake of preserving this compatibility. If we'd be comfortable to make a breaking change, reflection would not be necessary here.

This is a really good point but there are other non-reflection ways to ensure backwards compatibility: a third parameter to render_callback, a global $block variable, deprecating render_callback in lieu of render.

I agree that we must not ship anything that breaks backwards compatibility.

Over time, I've started to see this less about being a deprecation in that: A majority of blocks may not need anything more than just the block attributes, so making those available conveniently should be an optimization to consider.

This is also a really good point but, again, there are alternative approaches that don't involve deprecating render_callback or using reflection: a third parameter to render_callback, a global $blockvariable.

I'd really only be most comfortable if we buy into this as being a deprecation, and accept that attributes are less convenient to access than they were before.

It's certainly not more convenient to write $block->attributes instead of $attributes but I'd argue it's more explicit and future-proof.

If it were 2018 and we knew what we know now I think we would probably choose render_callback( $block ). (I'd also stock up on toilet paper 🙂)

---

My preference is:

• If we're viewing this as a deprecation: Add render( $block ), mark render_callback as private, eventually add _doing_it_wrong() to render_callback.
• If we're viewing this as an opt-in feature that not all block developers will want: Add global $block. Globals scare me but at least PHP makes it pretty explicit.

[aduth]

@noisysocks These are all great arguments. Reflecting on this some more, I think the way I've tried using reflection here is as a sort of function overloading in the traditional sense, which does not exist in PHP (or at least doesn't mean the same thing). I do feel more now that it's probably inappropriate to try to force this, especially when type declaration already has a purpose, which is definitely not one intended to be used in this way of changing the interpreted behavior of the function.

We still need some solution by today. Right now I'm leaning toward the $block global, or to simply do a partial revert of #21467 to give more time to think about the problem. I think a partial revert could be limited to keeping all of the changes, and simply change this line to pass $this->attributes in place of $this.

[aduth]

• Over time, I've started to see this less about being a deprecation in that: A majority of blocks may not need anything more than just the block attributes, so making those available conveniently should be an optimization to consider.

There's an interesting argument to be made here toward passing $block as an additional third argument of render_callback, which is: Yes, there's redundancy in approaching it this way, but if you acknowledge the redundancy as both optional and in pursuit of optimizing to the majority use-case, it might not be so bad if the argument is made available as an opt-in to these more advanced, rarer use-cases.

• Majority: "I just need block attributes"
  • function( $block_attributes ) {}
• Optional: "I've got advanced needs, and opt-in via the additional $block argument"
  • function( $block_attributes, $block_content, $block ) {}

[ZebulanStanphill]

Personally, I would prefer render( $block ) over a global or third argument.

(Notably, using just the attributes would be easier in the future once WordPress raises its minimum PHP version to one that supports destructuring associative arrays.)

[aduth]

I opened #21868 to introduce the global. I subsequently renamed it as $_experimental_block (see #21868 (comment)). There's too much difference of opinion here currently, and this needs a fix today. With it being prefixed as experimental and left undocumented, can move choose to move away from the global in the future.

[epiqueras]

I think that given all the concerns raised here. The third parameter to render_callback makes the most sense.

As @aduth pointed out:

There's an interesting argument to be made here toward passing $block as an additional third argument of render_callback, which is: Yes, there's redundancy in approaching it this way, but if you acknowledge the redundancy as both optional and in pursuit of optimizing to the majority use-case, it might not be so bad if the argument is made available as an opt-in to these more advanced, rarer use-cases.

• Majority: "I just need block attributes"
  • function( $block_attributes ) {}
• Optional: "I've got advanced needs, and opt-in via the additional $block argument"
  • function( $block_attributes, $block_content, $block ) {}

[aduth]

I merged #21868, which should give a temporary fix for the issues here by restoring render_callback to always passing a real associative array as the first argument. I'll keep this issue open since it will still need a decision on a final implementation. I'm hopeful it can be reached before the release following this week's.

[noisysocks]

• Majority: "I just need block attributes"
  • function( $block_attributes ) {}
• Optional: "I've got advanced needs, and opt-in via the additional $block argument"
  • function( $block_attributes, $block_content, $block ) {}

This is a nice way of framing it 🙂

[aduth]

An additional question which arises from this is whether we should keep the WP_Block class at all. It largely served the purpose of this ArrayAccess implementation, and in fact did not exist in earlier iterations of #21467. I do think it comes with quite a few benefits (#21467 (comment)) and brings a nice consistency with the client-side (#21467 (comment)). On the other hand, I do worry that it adds a decent amount of overhead which didn't exist previously: Notably in the constructor in how it eagerly assigns default attributes and maps inner blocks with context. I don't think it's significant work involved here, but (a) it may not scale very well with very deep hierarchies as the project moves toward full-site editing and (b) it should be contrasted with the current render_block implementation, which does almost nothing on a block until the point it determines it to be dynamic [1] [2].

I like a lot of what the class brings, and I especially like using it for a public API offering of the block object, notably with the consistency it brings through its property names. But these concerns do trouble me.

A few thoughts:

• In the very short-term (maybe even for this week's release), it may be worth considering to revert more of the server-side filtering implementation, if there is a legitimate worry here and possibility of removing the class altogether.
• Separately, we can consider one or a combination of:
  • Return to something more like earlier iterations of Block API: Add Block Context support #21467, removing the class and implementing the bulk of the logic for block context in render_block.
  • Or, try to incorporate more lazy evaluation, either by waiting for render-time to set up parts of the block relevant for the render, or perhaps even by __get magic method to wait to the time if and when a property (attributes, inner_blocks) is called to prepare the value.
    • Note: One of the purported benefits of preparing a block in the constructor was to make the instance properties more predictably available even outside render logic.
  • Or, keep things more or less as they are with the class, under the assumption that the additional overhead is insignificant or at least outweighed by the benefits.

[aduth]

• In the very short-term (maybe even for this week's release), it may be worth considering to revert more of the server-side filtering implementation, if there is a legitimate worry here and possibility of removing the class altogether.

There was another issue raised in Slack regarding the implementation of #21467. I proposed more a more thorough revert of the PHP implementation at #21921.

[aduth]

Separately, I opened #21925 as a temporary pull request to experiment with a few of the ideas and issues discussed: Around possible resolutions for the render_block filter bug, passing $block as the third argument of render_callback, and deferring some of the constructor behavior.

[epiqueras]

We should move forward with #21925, but leverage getters to avoid looping over inner blocks twice, and we should also make sure the filters run on inner blocks when rendering.