#62046 closed defect (bug) (fixed)
`render_block_context` filter works differently on top-level vs. inner blocks
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.8 | Priority: | normal |
Severity: | normal | Version: | 5.9 |
Component: | Editor | Keywords: | has-unit-tests has-patch commit needs-dev-note |
Focuses: | Cc: |
Description
The render_block_context
filter has a different effect depending on whether the block being rendered is a top-level block or an inner block. When the block is a top-level block, then context provided with render_block_context
is made available as context to its inner blocks. When the block is an inner block, the context provided with the filter isn't made available to its inner blocks.
By "top-level block," I mean a single block that's rendered with render_block()
, such as within do_blocks()
. By "inner block," I mean an inner block rendered within WP_Block::render()
.
I think this difference in behavior can be traced back to when the filter is applied.
The logic to make context available to inner blocks occurs within the WP_Block
constructor. In render_block()
, the value returned by the render_block_context
filter is passed to the new WP_Block
object, so the filtered value is the one made available to inner blocks.
Within WP_Block::render()
, the WP_Block
object for the inner block has already been instantiated (in WP_Block_List
). The value returned by render_block_context
is assigned directly to the inner block's $context
property, so the logic in the constructor to provide that context to deeper inner blocks isn't invoked.
The linked PR contains a unit test demonstrating this behavior. The test is derived from the real situation in which we observed it: A custom block capable of passing query
context to an inner core/post-template
block via render_block_context
. The post template block received the expected context when the custom block was a top-level block but not when it was an inner block, such as inside a group.
render_block_context
was applied to inner blocks in #51612 along with related filters, and there's quite a bit of discussion there about when exactly to apply them, so the logic in place now certainly has reasons behind it.
Still, I would expect the filter to behave the same way with respect to inner blocks regardless of where in the tree the block being filtered is. Between the two options, I would expect the filter to behave like it does in render_block()
: Context supplied with a filter is made available to inner blocks.
Change History (76)
This ticket was mentioned in PR #7344 on WordPress/wordpress-develop by @dlh.
5 months ago
#1
- Keywords has-patch has-unit-tests added
This ticket was mentioned in Slack in #core-editor by dlh. View the logs.
5 months ago
This ticket was mentioned in Slack in #core by obliviousharmony. View the logs.
5 months ago
#6
@
5 months ago
How well timed, I just debugged this same problem lol. I'm working on a PR refactoring how we're passing dynamic context in some of our blocks and ran into it. I've spent the better part of my weekend digging through all of the rendering logic for blocks and I've got a possible idea.
I'm sure there are reasons related to how inner blocks initially developed, however, It's a little weird that we make a distinction between a top-level block and an inner-block with respect to rendering. I think we should use render_block()
for both inner and top-level blocks so that we can keep all of the hooks together. As for the problem itself, the caveat here is that the available context is only populated on __construct
and there's no filtering before that point.
My first thought is to move render_block_data
and render_block_context
into the constructor but I think this might be a breaking change since it moves these filters to a different stage of the block rendering pipeline. Anyone relying on this being called between pre_render_block
and render_block
will be in for a shock. In my case, for example, I'm using pre_render_block
to set the global post and then a low-priority render_block
callback to reset it. While this change doesn't impact me, I'm very close conceptually to changing something that is then expected to be use as context.
My second thought is that, technically, none of these hooks require the block instance to exist. They operate on the $parsed_block
array that is then passed to the WP_Block
constructor. The caveat here is that WP_Block_List
handles our instantiation and it does so using offsetGet
. Anyone relying on this behavior before WP_Block::render()
instantiates it will still need that functional block instance.
My third thought is that, since WP_Block_List
holds onto the available context before the blocks are instantiated, we would need some way to filter it there to alter the context being given to the block.
The way that we're instantiating the block essentially forces either a breaking change or new filters. As much as I'd love to move these filters into the constructor, I think they have surrounded the block's render()
behavior for too long to be safe to touch. I propose this:
- New Filter:
create_block_available_context
: This filter goes in__construct()
and allows developers to filter the context available to the block. - New Filter:
create_block_data
: This filter also goes in__construct()
and filters the$parsed_block
it is given. - Add
$parent_block
as an optional parameter toWP_Block::__construct()
so that it can be available to the above filters. - Move the
$postId
and$postType
injection fromrender_block
into acreate_block_available_context
hook and inject it if it's not already set.- This should be safe because
render_block_context
would still receive these without any knowledge of where they came from. Even if someone was filtering to change them, since it's set on construction (and they can't currently alter that behavior), their code would continue to work. - There's a small breaking change here since right now you can filer this to remove those from the block hierarchy. I don't know why you would do that though and it would probably break everything?
- This should be safe because
Technically we documented render_block_context
as filtering the default context. There's a meaningful distinction between filtering the rendered context and the context provided to a block hierarchically. These changes should allow is to provide dynamic context options while still being able to override the context at render time.
@mukesh27 commented on PR #7344:
5 months ago
#7
What if we merge the parent context in inner blocks context in WP_Block::render()
method before the render_block_context
filter? If we did that the parent context is available in inner blocks context.
// Merge parent context.
if ( $parent_block->context ) {
$inner_block->context = array_merge( $parent_block->context, $inner_block->context );
}
I did quick test in local and it solve the issue and unit test ✅
5 months ago
#8
@mukeshpanchal27, are you talking about the following line when considering changes to the context?
---
What we miss is that the context is set only for the top-level block just before the class gets initialized:
So the best way to address it would be to, move that to WP_Block::construct
, but I have no idea what side-effects that would have.
5 months ago
#9
I did a quick proof of concept, and it passed the newly added test, but there are some changes in two other tests that I need to investigate. The diff:
-
src/wp-includes/blocks.php
2087 2087 $context['postType'] = $post->post_type; 2088 2088 } 2089 2089 2090 /** 2091 * Filters the default context provided to a rendered block. 2092 * 2093 * @since 5.5.0 2094 * @since 5.9.0 The `$parent_block` parameter was added. 2095 * 2096 * @param array $context Default context. 2097 * @param array $parsed_block { 2098 * An associative array of the block being rendered. See WP_Block_Parser_Block. 2099 * 2100 * @type string $blockName Name of block. 2101 * @type array $attrs Attributes from block comment delimiters. 2102 * @type array[] $innerBlocks List of inner blocks. An array of arrays that 2103 * have the same structure as this one. 2104 * @type string $innerHTML HTML from inside block comment delimiters. 2105 * @type array $innerContent List of string fragments and null markers where 2106 * inner blocks were found. 2107 * } 2108 * @param WP_Block|null $parent_block If this is a nested block, a reference to the parent block. 2109 */ 2110 $context = apply_filters( 'render_block_context', $context, $parsed_block, $parent_block ); 2090 $block = new WP_Block( $parsed_block, $context, null, $parent_block ); 2111 2091 2112 $block = new WP_Block( $parsed_block, $context );2113 2114 2092 return $block->render(); 2115 2093 } 2116 2094 -
src/wp-includes/class-wp-block-list.php
42 42 protected $registry; 43 43 44 44 /** 45 * Reference to the parent block. 46 * 47 * @since 6.7.0 48 * @var WP_Block|null 49 * @access protected 50 */ 51 protected $parent_block; 52 53 /** 45 54 * Constructor. 46 55 * 47 56 * Populates object properties from the provided block instance argument. 48 57 * 49 58 * @since 5.5.0 59 * @since 6.7.0 Added the optional `$parent_block` argument. 50 60 * 51 61 * @param array[]|WP_Block[] $blocks Array of parsed block data, or block instances. 52 62 * @param array $available_context Optional array of ancestry context values. 53 63 * @param WP_Block_Type_Registry $registry Optional block type registry. 64 * @param WP_Block|null $parent_block Optional. Optional. If this is a nested block, a reference to the parent block. 54 65 */ 55 public function __construct( $blocks, $available_context = array(), $registry = null ) {66 public function __construct( $blocks, $available_context = array(), $registry = null, $parent_block = null ) { 56 67 if ( ! $registry instanceof WP_Block_Type_Registry ) { 57 68 $registry = WP_Block_Type_Registry::get_instance(); 58 69 } … … 60 71 $this->blocks = $blocks; 61 72 $this->available_context = $available_context; 62 73 $this->registry = $registry; 74 $this->parent_block = $parent_block; 63 75 } 64 76 65 77 /** … … 93 105 $block = $this->blocks[ $offset ]; 94 106 95 107 if ( isset( $block ) && is_array( $block ) ) { 96 $block = new WP_Block( $block, $this->available_context, $this->registry );108 $block = new WP_Block( $block, $this->available_context, $this->registry, $this->parent_block ); 97 109 98 110 $this->blocks[ $offset ] = $block; 99 111 } -
src/wp-includes/class-wp-block.php
112 112 * its registered type will be assigned to the block's `context` property. 113 113 * 114 114 * @since 5.5.0 115 * @since 6.7.0 Added the optional `$parent_block` argument. 115 116 * 116 117 * @param array $block { 117 118 * An associative array of a single parsed block object. See WP_Block_Parser_Block. … … 125 126 * } 126 127 * @param array $available_context Optional array of ancestry context values. 127 128 * @param WP_Block_Type_Registry $registry Optional block type registry. 129 * @param WP_Block|null $parent_block Optional. If this is a nested block, a reference to the parent block. 128 130 */ 129 public function __construct( $block, $available_context = array(), $registry = null ) {131 public function __construct( $block, $available_context = array(), $registry = null, $parent_block = null ) { 130 132 $this->parsed_block = $block; 131 133 $this->name = $block['blockName']; 132 134 … … 138 140 139 141 $this->block_type = $registry->get_registered( $this->name ); 140 142 141 $this->available_context = $available_context; 143 /** 144 * Filters the default context provided to a rendered block. 145 * 146 * @since 5.5.0 147 * @since 5.9.0 The `$parent_block` parameter was added. 148 * 149 * @param array $context Default context. 150 * @param array $parsed_block { 151 * An associative array of the block being rendered. See WP_Block_Parser_Block. 152 * 153 * @type string $blockName Name of block. 154 * @type array $attrs Attributes from block comment delimiters. 155 * @type array[] $innerBlocks List of inner blocks. An array of arrays that 156 * have the same structure as this one. 157 * @type string $innerHTML HTML from inside block comment delimiters. 158 * @type array $innerContent List of string fragments and null markers where 159 * inner blocks were found. 160 * } 161 * @param WP_Block|null $parent_block If this is a nested block, a reference to the parent block. 162 */ 163 $this->available_context = apply_filters( 'render_block_context', $available_context, $block, $parent_block ); 142 164 143 165 if ( ! empty( $this->block_type->uses_context ) ) { 144 166 foreach ( $this->block_type->uses_context as $context_name ) { … … 159 181 } 160 182 } 161 183 162 $this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $registry );184 $this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $registry, $parent_block ); 163 185 } 164 186 165 187 if ( ! empty( $block['innerHTML'] ) ) {
@mukesh27 commented on PR #7344:
5 months ago
#10
@gziolo The POC is in right direction.
The Failed unit tests if we apply the POC.
1) Tests_Blocks_RenderReusableCommentTemplate::test_rendering_comment_template_sets_comment_id_context commentId block context wasn't set correctly. Failed asserting that 45 is identical to '45'. /var/www/tests/phpunit/tests/blocks/renderCommentTemplate.php:575 /var/www/vendor/bin/phpunit:122 2) Tests_Blocks_wpBlock::test_block_filters_for_inner_blocks Failed asserting that 3 is identical to 2. /var/www/tests/phpunit/tests/blocks/wpBlock.php:799 /var/www/vendor/bin/phpunit:122
The second test needs to check why the filter is called an extra time, the first one needs to fix in Unit test IMO.
This ticket was mentioned in PR #7420 on WordPress/wordpress-develop by @mukesh27.
5 months ago
#11
- Keywords has-patch added
### Testing PR. DO NOT REVIEW.
@mukesh27 commented on PR #7420:
5 months ago
#12
The POC code used from https://github.com/WordPress/wordpress-develop/pull/7344#issuecomment-2363536005
If we refactor core code per POC the ancestors context will be available in available_context
protected key.
For Column block i add two context. 1) how much column wpp-columns
2) column width wpp-col-width
[available_context:protected] => Array
(
[postId] => 6
[postType] => page
[wpp-columns] => 1
[wpp-col-width] => 50%
)
{{{
If we use `get_block_type_uses_context` filter then we get those context in `$block->context` public array key.
}}}php
/*
* Inspiration taken from current block binding implementation.
* https://github.com/WordPress/wordpress-develop/blob/6.6/src/wp-includes/class-wp-block-bindings-registry.php#L193-L204
*/
add_filter(
'get_block_type_uses_context',
function ( $uses_context, $block_type ) {
if ( 'core/image' === $block_type->name ) {
// Use array_values to reset the array keys.
return array_values( array_unique( array_merge( $uses_context, array( 'wpp-columns','wpp-col-width' ) ) ) );
}
return $uses_context;
},
10,
2
);
}}}
{{{php
[context] => Array
(
[postId] => 6
[postType] => page
[wpp-columns] => 1
[wpp-col-width] => 50%
)
5 months ago
#13
As was mentioned in a comment in the Trac ticket, moving the render_block_context
filter into the constructor would probably be considered backwards-incompatible because it changes the order in which the various block filters run. Additionally, it seems less desirable to me because it reinforces the pattern of having logic in the class constructor, a pattern that's partly responsible for the problem to begin with. Those are counterarguments to the approach, even though it might end up being the best approach anyway.
I wanted to offer another idea, similar to one mentioned in that same Trac comment, which is to use the filtered data to make a new WP_Block
instance with all the expected context, then render that. See the diff below.
In terms of backwards compatibility, there are still risks, but I think there might be fewer. As far as I can tell, the only developer assumption that would be violated is that WP_Block_List
will always return the exact same WP_Block
instance for a given index. Someone could be relying on that assumption, but perhaps that's less likely than a developer relying on filters running in a consistent order.
-
src/wp-includes/class-wp-block.php
diff --git a/src/wp-includes/class-wp-block.php b/src/wp-includes/class-wp-block.php index f7fd53dfc9..43d729998b 100644
a b class WP_Block { 496 496 /** This filter is documented in wp-includes/blocks.php */ 497 497 $inner_block->context = apply_filters( 'render_block_context', $inner_block->context, $inner_block->parsed_block, $parent_block ); 498 498 499 $block_content .= $inner_block->render(); 499 $this->inner_blocks[ $index ] = new WP_Block( $inner_block->parsed_block, $inner_block->context, $this->registry ); 500 501 $block_content .= $this->inner_blocks[ $index ]->render(); 500 502 } 501 503 502 504 ++$index;
5 months ago
#14
I wanted to offer another idea, similar to one mentioned in that same Trac comment, which is to use the filtered data to make a new WP_Block instance with all the expected context, then render that. See the diff below.
Intriguing alternative. It replicates how the render_block
works today, precisely:
Can you apply that change to this branch to ensure it passes all the tests? We can discuss further whether there are some opportunities to avoid recreating the instance of WP_Block
when it isn't necessary for some block types.
5 months ago
#15
In terms of backwards compatibility, there are still risks, but I think there might be fewer. As far as I can tell, the only developer assumption that would be violated is that WP_Block_List will always return the exact same WP_Block instance for a given index. Someone could be relying on that assumption, but perhaps that's less likely than a developer relying on filters running in a consistent order.
Could it be mitigated by introducing a new method on WP_Block
like update_available_context
? A rough sketch of the idea:
function update_available_context( $context ) {
$this->context = array();
$this->available_context = $context;
if ( ! empty( $this->block_type->uses_context ) ) {
foreach ( $this->block_type->uses_context as $context_name ) {
if ( array_key_exists( $context_name, $this->available_context ) ) {
$this->context[ $context_name ] = $this->available_context[ $context_name ];
}
}
}
if ( ! empty( $this->parsed_block['innerBlocks'] ) ) {
$child_context = $this->available_context;
if ( ! empty( $this->block_type->provides_context ) ) {
foreach ( $this->block_type->provides_context as $context_name => $attribute_name ) {
if ( array_key_exists( $attribute_name, $this->attributes ) ) {
$child_context[ $context_name ] = $this->attributes[ $attribute_name ];
}
}
}
$this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $this->registry );
}
}
I might be missing some nuances, but it shouldn't differ from the original idea while retaining the existing WP_Block
instance. One important consideration is that rendering happens bottom-up, while all objects get initialized in the top-down fashion, so that needs to be further investigated.
@mukesh27 commented on PR #7344:
5 months ago
#16
Could it be mitigated by introducing a new method on
WP_Block
likeupdate_available_context
? A rough sketch of the idea:
How it help to solve the issue?
5 months ago
#17
Could it be mitigated by introducing a new method on
WP_Block
likeupdate_available_context
? A rough sketch of the idea:
How it help to solve the issue?
See the comment from @dlh01 https://github.com/WordPress/wordpress-develop/pull/7344#issuecomment-2375599875. It's the same idea with a twist:
-
src/wp-includes/class-wp-block.php
diff --git a/src/wp-includes/class-wp-block.php b/src/wp-includes/class-wp-block.php index f7fd53dfc9..43d729998b 100644
a b class WP_Block { 496 496 /** This filter is documented in wp-includes/blocks.php */ 497 497 $inner_block->context = apply_filters( 'render_block_context', $inner_block->context, $inner_block->parsed_block, $parent_block ); 498 498 499 $block_content .= $inner_block->render(); 499 $inner_block->update_available_context( $inner_block->context ); 500 $block_content .= $inner_block->render(); 500 501 } 501 502 502 503 ++$index;
5 months ago
#18
Here is the diff with the very quick draft that passes existing and the new tests added in this PR:
-
src/wp-includes/class-wp-block.php
138 138 139 139 $this->block_type = $registry->get_registered( $this->name ); 140 140 141 $this->update_available_context( $block, $available_context ); 142 143 if ( ! empty( $block['innerHTML'] ) ) { 144 $this->inner_html = $block['innerHTML']; 145 } 146 147 if ( ! empty( $block['innerContent'] ) ) { 148 $this->inner_content = $block['innerContent']; 149 } 150 } 151 152 public function update_available_context( $block, $available_context ) { 141 153 $this->available_context = $available_context; 142 154 143 155 if ( ! empty( $this->block_type->uses_context ) ) { … … 158 170 } 159 171 } 160 172 } 161 162 $this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $registry ); 173 $this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $this->registry ); 163 174 } 164 165 if ( ! empty( $block['innerHTML'] ) ) {166 $this->inner_html = $block['innerHTML'];167 }168 169 if ( ! empty( $block['innerContent'] ) ) {170 $this->inner_content = $block['innerContent'];171 }172 175 } 173 176 174 177 /** … … 505 508 /** This filter is documented in wp-includes/blocks.php */ 506 509 $inner_block->context = apply_filters( 'render_block_context', $inner_block->context, $inner_block->parsed_block, $parent_block ); 507 510 511 $inner_block->update_available_context( $inner_block->parsed_block, $inner_block->context ); 508 512 $block_content .= $inner_block->render(); 509 513 }
@joemcgill commented on PR #7344:
5 months ago
#19
@gziolo
Here is the diff with the very quick draft that passes existing and the new tests added in this PR
I like this approach at first glance. I'll plan on giving it a test but am unable to apply that diff (probably something I'm doing wrong).
@joemcgill commented on PR #7344:
5 months ago
#20
@gziolo I was able to convert the diff above into an diff that I could apply to my git checkout by copying to a file and running git apply ~/path/to/file.diff
(pasting here for others and to double check that I didn't miss anything:
-
src/wp-includes/class-wp-block.php
diff --git a/src/wp-includes/class-wp-block.php b/src/wp-includes/class-wp-block.php index d4481a68a7..8c2928d2a6 100644
a b class WP_Block { 138 138 139 139 $this->block_type = $registry->get_registered( $this->name ); 140 140 141 $this->update_available_context( $block, $available_context ); 142 143 if ( ! empty( $block['innerHTML'] ) ) { 144 $this->inner_html = $block['innerHTML']; 145 } 146 147 if ( ! empty( $block['innerContent'] ) ) { 148 $this->inner_content = $block['innerContent']; 149 } 150 } 151 152 public function update_available_context( $block, $available_context ) { 153 $this->context = array(); 154 141 155 $this->available_context = $available_context; 142 156 143 157 if ( ! empty( $this->block_type->uses_context ) ) { … … class WP_Block { 159 173 } 160 174 } 161 175 162 $this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $registry ); 163 } 164 165 if ( ! empty( $block['innerHTML'] ) ) { 166 $this->inner_html = $block['innerHTML']; 167 } 168 169 if ( ! empty( $block['innerContent'] ) ) { 170 $this->inner_content = $block['innerContent']; 176 $this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $this->registry ); 171 177 } 172 178 } 173 179 … … class WP_Block { 505 511 /** This filter is documented in wp-includes/blocks.php */ 506 512 $inner_block->context = apply_filters( 'render_block_context', $inner_block->context, $inner_block->parsed_block, $parent_block ); 507 513 514 $inner_block->update_available_context( $inner_block->parsed_block, $inner_block->context ); 515 508 516 $block_content .= $inner_block->render(); 509 517 }
Unfortunately, this looks like it is causing page level context to no get passed to innerBlocks, so blocks like the core/post-title
block are not rendering correctly with this diff applied. I'm trying to debug it now.
@joemcgill commented on PR #7344:
5 months ago
#21
I think I may have figured it out. When calling update_avialable_context()
on a block that already has available context, if the new context being passed is empty, the old available context was being overwritten. Instead it should be merged. Here's a new diff:
-
src/wp-includes/class-wp-block.php
diff --git a/src/wp-includes/class-wp-block.php b/src/wp-includes/class-wp-block.php index d4481a68a7..11ef862d71 100644
a b class WP_Block { 138 138 139 139 $this->block_type = $registry->get_registered( $this->name ); 140 140 141 $this->available_context = $available_context; 141 $this->update_available_context( $block, $available_context ); 142 143 if ( ! empty( $block['innerHTML'] ) ) { 144 $this->inner_html = $block['innerHTML']; 145 } 146 147 if ( ! empty( $block['innerContent'] ) ) { 148 $this->inner_content = $block['innerContent']; 149 } 150 } 151 152 public function update_available_context( $block, $available_context ) { 153 $this->context = array(); 154 155 if ( null === $this->available_context ) { 156 $this->available_context = $available_context; 157 } else { 158 $this->available_context = array_merge( $this->available_context, $available_context ); 159 } 142 160 143 161 if ( ! empty( $this->block_type->uses_context ) ) { 144 162 foreach ( $this->block_type->uses_context as $context_name ) { … … class WP_Block { 159 177 } 160 178 } 161 179 162 $this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $registry ); 163 } 164 165 if ( ! empty( $block['innerHTML'] ) ) { 166 $this->inner_html = $block['innerHTML']; 167 } 168 169 if ( ! empty( $block['innerContent'] ) ) { 170 $this->inner_content = $block['innerContent']; 180 $this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $this->registry ); 171 181 } 172 182 } 173 183 … … class WP_Block { 505 515 /** This filter is documented in wp-includes/blocks.php */ 506 516 $inner_block->context = apply_filters( 'render_block_context', $inner_block->context, $inner_block->parsed_block, $parent_block ); 507 517 518 $inner_block->update_available_context( $inner_block->parsed_block, $inner_block->context ); 519 508 520 $block_content .= $inner_block->render(); 509 521 }
5 months ago
#22
As I was thinking about the update_available_context()
method, it occurred to me that there's a second difference between the two invocations of the render_block_context
filter. @ObliviousHarmony's comment in Trac hinted at it, and perhaps it's on your mind as well.
When render_block_context
runs for top-level blocks in render_block()
, the filtered value becomes the $available_context
parameter and is reduced down to the context that the block type uses. When render_block_context
runs for inner blocks in WP_Block::render()
, the result is applied directly to the $context
property, meaning that inner blocks can be given context properties outside of what the block type supports. I've added a test to this branch to demonstrate this difference. I'm not sure, but the difference seems similarly unintentional.
Can you apply that change to this branch to ensure it passes all the tests? We can discuss further whether there are some opportunities to avoid recreating the instance of WP_Block when it isn't necessary for some block types.
Done and pushed to this branch.
One observation regarding the update_available_context()
method as drafted here: If I'm reading correctly, it seems to me that the risk of violating developer assumptions is the same as it would be if a new WP_Block
was instantiated as happens in 6a37f03b2b. The update_available_context()
method would update the $inner_blocks
property with a new WP_Block_List
instance, and won't that will return new instances of WP_Block
anyway?
4 months ago
#23
The update_available_context() method would update the $inner_blocks property with a new WP_Block_List instance, and won't that return new instances of WP_Block anyway?
Yes, that's a good observation. If there are inner blocks, then all of them would have a new instance recreated. So, it is not so much of a win to call the method on the existing instance. Overall, it's a very complex arrangement with all these parsed block.
4 months ago
#24
When render_block_context runs for top-level blocks in render_block(), the filtered value becomes the $available_context parameter and is reduced down to the context that the block type uses. When render_block_context runs for inner blocks in WP_Block::render(), the result is applied directly to the $context property, meaning that inner blocks can be given context properties outside of what the block type supports. I've added a test to this branch to demonstrate this difference. I'm not sure, but the difference seems similarly unintentional.
Overall, my understanding was that the context injected could be outside of what the block supports. Example from the Query block, in particular Post Template block:
This block doesn't define providesContext
:
So, this is very concerning if it works differently depending on where the filters get called from. It needs to be further investigated. I hope we can sort out all these differences somehow.
#25
@
4 months ago
- Milestone changed from Awaiting Review to 6.8
We missed the date to land it in WordPress 6.7. I will explicitly set 6.8 milestone to ensure we sort it out in the next weeks. I think we are getting close to finding an acceptable solution in the PR from @dlh: https://github.com/WordPress/wordpress-develop/pull/7344. More testing is needed to confirm that it won't produce unintended side effects.
This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.
4 months ago
#27
@
4 months ago
We missed the date to land it in WordPress 6.7.
@gziolo since this is a bug, I think we could likely still land a fix for this during beta, which would be ideal given that there will likely be a large time gap before 6.8 is released (likely end of Q1, 2025 based on past trends).
Could we possibly update the PR currently in progress to include one of the approaches we've been iterating on so that it's easier to test?
4 months ago
#28
Overall, my understanding was that the context injected could be outside of what the block supports. Example from the Query block, in particular Post Template block:
OK, fair enough. In that case, I think the inconsistency is just the other way around: When the block is a top-level block, it's not possible to provide context outside of what the block supports using render_block_context
because the filtered value becomes the $available_context
and is reduced down to the context that the block type uses. I've updated test_render_block_context_allowed_context()
in this PR to demonstrate this behavior.
(Side note: The example from the Post Template block is extra weird because the blockName
is set to core/null
just above the highlighted lines. Even if the Post Template block defined providesContext
, I don't think it would be referenced because the instantiated WP_Block
wouldn't retrieve the Post Template block type.)
It seems that we're dealing with a couple of inconsistencies now: Context supplied with render_block_context
isn't always made available to a block's inner blocks, nor is it always applied to the block itself.
Accordingly, I've reverted 6a37f03 from this PR (for now) because it addressed only the first of these. I thought that having an incomplete solution in place might get in the way of stepping back to reconsider the entire situation. Both new tests are failing again, as expected.
#29
@
4 months ago
It sounds reasonable to land a bug fix in WordPress 6.7 during the beta cycle if that would delay other efforts. I mentioned that on WordPress Slack, just wanted to quickly repeat that the biggest challenge is to agree on the best way forward to unify handling for this filter.
This ticket was mentioned in PR #7522 on WordPress/wordpress-develop by @mukesh27.
4 months ago
#30
Testing PR. DO NOT REVIEW.
4 months ago
#32
I've updated
test_render_block_context_allowed_context()
in this PR to demonstrate this behavior.
I just noticed that I forgot to push this update. It's there now. Sorry!
4 months ago
#33
@mukeshpanchal27 Heads up that the tests in https://github.com/WordPress/wordpress-develop/pull/7344 have been updated to reflect the last few comments there.
4 months ago
#34
Nice one @mukeshpanchal27. @dlh01 and @joemcgill, it looks like all the unit tests we could think of pass in this branch. How do you feel about the proposed solution? Is it good enough to move forward as part of the WP 6.7 release?
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
4 months ago
4 months ago
#36
Thanks for the ping! I should be able to take a look at this Wednesday or Thursday.
@mukesh27 commented on PR #7522:
4 months ago
#37
The PR is ready for review. Address the feedbacks.
4 months ago
#38
I don't have more code comments for now. As mentioned earlier, at this point, we need to confirm that no accidental regressions have been introduced, as this API is used with different assumptions to overcome the preexisting shortcomings.
@mukesh27 commented on PR #7420:
4 months ago
#39
Closing in favour of #7522
@mukesh27 commented on PR #7522:
4 months ago
#40
Outstanding comments like https://github.com/WordPress/wordpress-develop/pull/7522/files#r1793229135 would be good to address, but fairly minor.
4 months ago
#41
Joe has accurately summarized my hesitancy about the change to the test as it relates to #7344. Here are a few other thoughts after reading the PR.
First: Both times that update_available_context()
is called, the passed parameters are already present on the block instance. For example:
$inner_block->update_available_context( $inner_block->parsed_block, $inner_block->context );
This seems like a bit of a code smell. Why would it be necessary to offer the parameters if the block already has the data?
Second: This is something that was on my mind in #7344, and even now I can't quite put my finger on it, but I keep thinking that there's an inconsistency of some kind between the use of a new public method that's designed to update a block's state and the call to new WP_Block_List()
within that method, since new WP_Block_List()
will wipe out the previous WP_Block
instances that were being used as inner blocks.
If update_available_context()
was called on a child block before being called on a parent block, the updated context in the child block would be lost because the inner blocks on the parent would be replaced after new WP_Block_List()
in the parent. I'm purely speculating here, but it seems like behavior that unexpectedly might get in the way of future features, or at least surprise a plugin developer who attempts to make use of the new public API. There's an appearance of stability (arguably) that isn't quite there.
Third: I'm not sure that I understand why update_available_context()
would also update the inner_html
and inner_content
class properties. Neither of these properties relates to block context, as far as I can tell. Also, doesn't it make the block state inconsistent to update just those properties with the passed block? For example, the attributes
class property, accessed just a few lines earlier, would still reference the parsed_block
property, which isn't changed by update_available_context()
. Different properties in the same WP_Block
would end up referencing different parsed block arrays.
@flixos90 commented on PR #7522:
4 months ago
#42
@dlh01
This seems like a bit of a code smell. Why would it be necessary to offer the parameters if the block already has the data?
This is necessary because these two properties are filtered outside of the instance. Since they are modified outside the block instance, it is crucial to pass the modified values back to the instance. Otherwise its state is incorrect, and that partly led to the bug this PR is aiming to address.
Third: I'm not sure that I understand why
update_available_context()
would also update theinner_html
andinner_content
class properties. Neither of these properties relates to block context, as far as I can tell.
This relates to my above answer. Both inner_html
and inner_content
depend on parsed_block
, which is being filtered outside the instance. So if parsed_block
changes, those changes need to be reflected in inner_html
and inner_content
.
Maybe there are better solutions than filtering these properties outside of the class instance, it could be worth exploring that. But in its current state, this approach is broken because it leads to outdated property values (in case the parsed block or block context are filtered), and that's fixed by this PR.
4 months ago
#43
This is necessary because these two properties are filtered outside of the instance. Since they are modified outside the block instance, it is crucial to pass the modified values back to the instance. Otherwise its state is incorrect, and that partly led to the bug this PR is aiming to address.
I'm sorry, but I'm not following you. I get that the properties are being filtered outside of the instance, but the filter is being applied directly to the properties. Wouldn't it be possible for update_available_context()
to accept no parameters and refer to the properties themselves?
This relates to my above answer. Both inner_html and inner_content depend on parsed_block, which is being filtered outside the instance. So if parsed_block changes, those changes need to be reflected in inner_html and inner_content. Maybe another method name would help. Or we could try having two separate methods, one to update data based on a modified parsed_block, another to update data based on a modified context.
That's a great point, and I hadn't thought about it. Still, it seems like a separate bug than the one raised in the ticket, since it has to do with a different filter (I assume you're referring to render_block_data
). So, yes, a separate method or method name makes sense to me.
@flixos90 commented on PR #7522:
4 months ago
#44
This is necessary because these two properties are filtered outside of the instance. Since they are modified outside the block instance, it is crucial to pass the modified values back to the instance. Otherwise its state is incorrect, and that partly led to the bug this PR is aiming to address.
I'm sorry, but I'm not following you. I get that the properties are being filtered outside of the instance, but the filter is being applied directly to the properties. Wouldn't it be possible for
update_available_context()
to accept no parameters and refer to the properties themselves?
Ah I see what you're saying, yes. Probably the method should be parameterless and simply update the other properties based on the current values of the updated properties.
@mukesh27 commented on PR #7522:
4 months ago
#45
@joemcgill @felixarntz @dlh01
I've spent some time addressing the feedback, and I would appreciate your input before I proceed with fixing the unit tests.
- Removed the parameter from the
update_block_context
function. - Introduced a new function
update_parsed_block_content
as per this PR review.
Some of the existing unit tests are failing. Below are the details:
There were 5 failures: 1) WP_Block_Bindings_Render::test_blocks_can_just_access_the_specific_uses_context The 'content' should be updated with the value of the second source context value. Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'Value: source two context value' +'Value: source one context value' /var/www/tests/phpunit/tests/block-bindings/render.php:213 phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97 /var/www/vendor/bin/phpunit:118 2) Tests_Blocks_RenderBlock::test_provides_block_context Failed asserting that two arrays are identical. --- Expected +++ Actual @@ @@ Array &0 ( + 'postId' => 70 + 'postType' => 'post' + 'tests/contextWithAssigned' => 10 'tests/contextWithDefault' => 0 - 'tests/contextWithAssigned' => 10 ) /var/www/tests/phpunit/tests/blocks/renderBlock.php:114 phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97 /var/www/vendor/bin/phpunit:118 3) Tests_Blocks_RenderBlock::test_default_context_is_filterable Failed asserting that two arrays are identical. --- Expected +++ Actual @@ @@ Array &0 ( + 'postId' => 72 + 'postType' => 'post' 'example' => 'ok' ) /var/www/tests/phpunit/tests/blocks/renderBlock.php:193 phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97 /var/www/vendor/bin/phpunit:118 4) Tests_Blocks_wpBlock::test_constructor_assigns_context_from_block_type Failed asserting that two arrays are identical. --- Expected +++ Actual @@ @@ Array &0 ( 'requested' => 'included' + 'unrequested' => 'not included' ) /var/www/tests/phpunit/tests/blocks/wpBlock.php:173 phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97 /var/www/vendor/bin/phpunit:118 5) Tests_Blocks_wpBlock::test_constructor_prepares_context_for_inner_blocks Failed asserting that actual size 1 matches expected size 0. /var/www/tests/phpunit/tests/blocks/wpBlock.php:221 phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97 /var/www/vendor/bin/phpunit:118
This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.
4 months ago
@mukesh27 commented on PR #7522:
4 months ago
#47
@felixarntz @joemcgill @dlh01 @gziolo The PR is ready for review.
@mukesh27 commented on PR #7522:
4 months ago
#48
I take another pass and try to find the the solution to not merge the $this->available_context
to $this->context
but somehow it break the old or new unit tests. Could anyone take a look when they you have moment?
@mukesh27 commented on PR #7522:
3 months ago
#49
@felixarntz @joemcgill @dlh01 @gziolo
In https://github.com/WordPress/wordpress-develop/pull/7522/commits/c83ca8268e3b9db13ad3e17cf0e2a478529dbdc6, I removed the context merging from $this->available_context
to $this->context
and reverted the unit test changes.
Currently, there’s only one uncovered case: when a block needs context for itself, not for nested blocks. The test_render_block_context_allowed_context()
test partially covers this—the arbitrary
context isn't available for a single block but works correctly for nested blocks.
In the single block case, the arbitrary
context is in available_context
but not in context
. To address this, we could either merge contexts or apply the render_block_context
filter to context
. However, the latter would impact existing unit tests.
Would it be acceptable to remove the failing unit test and merge this PR, as it resolves the ancestor block context issue? We can then open a follow-up discussion to address the remaining part.
#50
@
3 months ago
I reviewed the PR and today's implementation further and I think identified the remaining key problem, which stems from the inconsistent place the filters are applied (before block instantiation vs after block instantiation).
TL;DR: The main problem we still need to address is that render_block_context
effectively filters a block's $context
for inner blocks, but a block's $available_context
for top-level blocks.
See https://github.com/WordPress/wordpress-develop/pull/7522#pullrequestreview-2427816516.
It would be great to get your perspectives on this @dlh @gziolo @joemcgill. Which of the two ways is in your opinion the way it should work? Filter the actual $context
(bypassing the uses_context
restriction of the block) or filter the $available_context
? Obviously the filter name makes me think it's the first, but let's ignore naming here for a moment to think about this from a behavior perspective.
#51
@
3 months ago
The way that I would expect the context API to work is that any time information is added to a block's $context
prop (either via a block's provides_context
property or by being added via a filter), that information is added to the $available_context
that is available to that block and any of its ancestors, regardless of whether a block is top level or not.
A block's $context
value (regardless of whether it's top level or not) will include the subset of $available_context
that the block itself supports (e.g., uses_context
) and any additional context that has been dynamically added via a filter of that block's context.
In addition, any context that is added to a block via a filter should be included in that block's $available_context
which gets passed to any of that block's ancestors.
This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.
3 months ago
#53
@
3 months ago
Of all the times that I've wanted to use the render_block_context
filter, I've approached it as if it was filtering the available context to be passed to inner blocks, not filtering the actual context of a block. I can't recall a time when I've meant to filter the actual context. So, regardless of what ends up happening with this particular filter, from a practical perspective, a consistent filter for the available context is the functionality that would be useful to me to have.
3 months ago
#54
Basically, it's not clear what
render_block_context
is exactly supposed to filter:
- Is it supposed to filter the block's
$context
, explicitly allowing to ignore whatever the block'suses_context
property says? That's what it's doing for inner blocks today.
- If so, then discard my previous statement, and we need to fix this for how it works on the top-level block so that your test
test_render_block_context_allowed_context()
passes. For example, set the$context
property on the block instance directly to the filtered value.
- Is it actually supposed to filter the
$available_context
? On the top-level block, that's what it's doing today.
- If so, we need to fix the
refresh_context_dependents()
method so that it sanitizes$context
to still only contain what the block'suses_context
allows for.$available_context
should still include anything including arbitrary values.
@felixarntz I wanted to make sure you saw this older conversation in the other PR about this topic, where @gziolo mentioned that at least some of the time, the filter is meant to override uses_context
: https://github.com/WordPress/wordpress-develop/pull/7344#issuecomment-2382991949. That conversation is what led to the arbitrary
test that I switched to in https://github.com/WordPress/wordpress-develop/pull/7344/commits/90c63a2684dc28efa341d7e8133dcefef3709916.
@flixos90 commented on PR #7522:
3 months ago
#55
@dlh01 Thank you for providing this context.
In that case, I would argue we should adjust how this is handled in render_block()
, so that the filter allows overriding uses_context
as well, for consistency. For example, in addition to passing the provided data into the constructor ($available_context
), we should right after that set the $context
property to it on the instantiated block.
This way, it should behave consistently going forward, with the filter allowing to override $uses_context
everywhere it's applied.
#56
@
3 months ago
I need to catch up with the discussion as I wasn't able to follow up recently. I will start by exploring how this filter is consumed inside core blocks before jumping into the conversation on GitHub, where the PR lives.
3 months ago
#57
Existing usage of render_block_context
in WordPress core:
---
My understanding of how it should work is as follows:
- Every block can define
providesContext
with the list of attributes that are going to be exposed as block context. render_block_context
allows overriding the values set withprovidesContext
or inject new properties when necessary.uses_context
needs to be always explicitly used to read the provided context no matter how it was set. If you inspect.
In practical terms, it means that available_context
is properly documented:
It's the property that gets impacted by the render_block_context
filter. It also means that the block's context is derived from the available context:
It gets filtered by scanning uses_context
, which can be provided in the block definition, or through Block Bindings registration. Regardless, whenever $this->available_context
changes, then $this->context
should get updated, too.
3 months ago
#58
In that case, I would argue we should adjust how this is handled in render_block(), so that the filter allows overriding uses_context as well, for consistency. For example, in addition to passing the provided data into the constructor ($available_context), we should right after that set the $context property to it on the instantiated block.
In https://github.com/WordPress/wordpress-develop/pull/7522#issuecomment-2484973281 I shared the way I was always thinking about block context and related filters. Now, let's go with the most realistic approach which is tied to the current implementation. In practice, the contract established with uses_context
is violated by the code that allows extending the specific context providef for the individual block. I tend to agree that we should replicate that when using the render_block
utility that as of today changes the available context, but doesn't force new properties to be exposed to block rendering engine unless they are defined with uses_context
. I think we need to fix it in the first place for consistency. In effect, we probably in both places need to adjust both available_context
and context
so they both always stay in sync, plus the context
for individual block contains properties allows through uses_context
and these exposed by the filter.
3 months ago
#59
For the sake of the experiment, I played a bit with changes that try to aggressively run the same hook to update both the available context and the context:
-
src/wp-includes/blocks.php
2163 2163 $context['postType'] = $post->post_type; 2164 2164 } 2165 2165 2166 $block = new WP_Block( $parsed_block, $context ); 2167 2166 2168 /** 2167 2169 * Filters the default context provided to a rendered block. 2168 2170 * … … 2183 2185 * } 2184 2186 * @param WP_Block|null $parent_block If this is a nested block, a reference to the parent block. 2185 2187 */ 2186 $ context = apply_filters( 'render_block_context', $context, $parsed_block, $parent_block );2188 $block->context = apply_filters( 'render_block_context', $block->context, $block->parsed_block, $parent_block ); 2187 2189 2188 $block = new WP_Block( $parsed_block, $context );2189 2190 2190 return $block->render(); 2191 2191 } 2192 2192 -
src/wp-includes/class-wp-block.php
138 138 139 139 $this->block_type = $registry->get_registered( $this->name ); 140 140 141 $this->available_context = $available_context; 141 /** This filter is documented in wp-includes/blocks.php */ 142 $this->available_context = apply_filters( 'render_block_context', $available_context, $this->parsed_block, null ); 142 143 143 144 if ( ! empty( $this->block_type->uses_context ) ) { 144 145 foreach ( $this->block_type->uses_context as $context_name ) {
I passed null
as the parent block in the constructor of WP Block, but it probably should be handled better.
3 months ago
#60
For the sake of the experiment, I played a bit with changes that try to aggressively run the same hook to update both the available context and the context:
-
src/wp-includes/blocks.php
2163 2163 $context['postType'] = $post->post_type; 2164 2164 } 2165 2165 2166 $block = new WP_Block( $parsed_block, $context ); 2167 2166 2168 /** 2167 2169 * Filters the default context provided to a rendered block. 2168 2170 * … … 2183 2185 * } 2184 2186 * @param WP_Block|null $parent_block If this is a nested block, a reference to the parent block. 2185 2187 */ 2186 $ context = apply_filters( 'render_block_context', $context, $parsed_block, $parent_block );2188 $block->context = apply_filters( 'render_block_context', $block->context, $block->parsed_block, $parent_block ); 2187 2189 2188 $block = new WP_Block( $parsed_block, $context );2189 2190 2190 return $block->render(); 2191 2191 } 2192 2192 -
src/wp-includes/class-wp-block.php
138 138 139 139 $this->block_type = $registry->get_registered( $this->name ); 140 140 141 $this->available_context = $available_context; 141 /** This filter is documented in wp-includes/blocks.php */ 142 $this->available_context = apply_filters( 'render_block_context', $available_context, $this->parsed_block, null ); 142 143 143 144 if ( ! empty( $this->block_type->uses_context ) ) { 144 145 foreach ( $this->block_type->uses_context as $context_name ) {
I passed null
as the parent block in the constructor of WP Block, but it probably should be handled better.
@flixos90 commented on PR #7522:
3 months ago
#61
@gziolo I think I understand most of your exploration, but not your conclusion of how it _should_ behave.
Sharing my understanding and asking for clarification:
- How I understand your comment https://github.com/WordPress/wordpress-develop/pull/7522#issuecomment-2484973281 is that
render_block_context
should filter the block's$available_context
. This means it indirectly also filters$context
, but only for the values in the filtered$available_context
that are allowlisted in$uses_context
. - Based on this, what
render_block
does is correct, and whatWP_Block::render()
does for its child blocks is incorrect (because it filters the$context
itself directly, allowing to ignore$uses_context
). So far, so good. - But then I don't understand what you're concluding in https://github.com/WordPress/wordpress-develop/pull/7522#issuecomment-2485483432 and why. That seems to contradict what you said before. Can you clarify why the behavior should be different from what you outlined before as the ideal behavior?
- That question also applies to https://github.com/WordPress/wordpress-develop/pull/7522#issuecomment-2485869808, though here I would point out specifically that I don't think it's a good idea to use the same filter to filter
$context
in one place and$available_context
in another place. That's what Core is already doing today, and it causes the problems. We need to make it behave consistently one way or another.
3 months ago
#62
I think I understand most of your exploration, but not your conclusion of how it should behave.
I'm not surprised as it's a very complex problem to tackle as we have an important backward compatibility problem to solve. I offered my preferred solution in https://github.com/WordPress/wordpress-develop/pull/7522#issuecomment-2484973281 (that might require some deprecation, phase out for current behavior, detailed dev note), and an alternative (https://github.com/WordPress/wordpress-develop/pull/7522#issuecomment-2485483432) that should work in most cases (the dev note might be sufficient). We should pick one.
So far, what would make more sense to me based on your first comment is that WP_Block::render() is adjusted to filter the child block's $available_context, and based on that the child block's $context should be updated from it, but only for those keys included in $uses_context.
Again, I'm open to the idea of applying the changes you proposed. I only wanted to emphasize again it's hard to predict what that would cause as a side effect. In some cases, inner block during rendering might depend on the presence of the context that isn't explicitly exposed through uses_context
. Overall, I don't like this behavior introduced in the inner block rendering as this is something that doesn't translate well to the editor, where React offers a much more predictable interface for how you pair the provider and consumer when working with the context.
Anyone from @WordPress/gutenberg-core willing to chime in and share their opinions on this topic that will help to make the final decision?
@flixos90 commented on PR #7522:
3 months ago
#63
@gziolo Regarding backward compatibility, I assume you're referring to plugins that may potentially access a block's $context
and expect to have one of the filtered pieces of data in there even if it is not in the block's $uses_context
, based on the current behavior?
We could potentially still fix the behavior to get to the preferred state, but have a deprecation strategy to not break such behavior right away. Something like:
- We change the inner block behavior to first apply the filtered value to
$available_context
. Then the$context
will be "re-generated" based on the modified$available_context
and the unchanged$uses_context
. That's the ideal and eventual state. - We then also set the
$context
value directly to the filtered value, to (for now) maintain that behavior. But before doing that, we check whether there is a diff between the "correct"$context
and the overriding$context
and store that. We can use that to trigger a deprecation warning if any code accesses one of those context values (because eventually those shouldn't be present anymore). - To catch the problematic accesses, we could implement a simple class, e.g.
WP_Block_Context
which implementsArrayAccess
and use that instead of an array for the$context
parameter. By using a class, we are able to know which keys are accessed and can trigger the deprecation warning if it's one of the keys that should ideally not be allowed anymore.
WDYT?
3 months ago
#64
Yes, that’s exactly what I was thinking about in terms of backward compatibility. It seems like a viable path forward. We can explore specific details of the depreciation strategy separately later. I was thinking also about making the context a virtual property and handle everything through magic methods. Whatever works best here 😀
@flixos90 commented on PR #7522:
3 months ago
#65
I was thinking also about making the context a virtual property and handle everything through magic methods.
That was my first thought too, but I think that wouldn't work because magic methods would only be called for the entire $context
property, so we wouldn't know which keys someone is accessing on it. That's how I got to the ArrayAccess
class idea.
3 months ago
#66
I think got it know, we would pass WP_Block_Context
instance to the filter when processing the inner block. This way we would be able to easily derive the context update, based on detected changes in the class. Interesting idea 👍🏻
@flixos90 commented on PR #7522:
3 months ago
#67
I think got it know, we would pass
WP_Block_Context
instance to the filter when processing the inner block. This way we would be able to easily derive the context update, based on detected changes in the class. Interesting idea 👍🏻
That's actually not entirely what I thought, but that's even better than what I thought 😆 So let's try that.
@joemcgill commented on PR #7522:
2 months ago
#68
@gziolo and @felixarntz, I've been spending time today catching up on this conversation and wanted to summarize what I understand to be the current thinking. Based on what @gziolo said is his preferred solution, the render_block_context
filter should be filtering the available_context
that a block can access, based on it's own uses_context
properties and NOT be filtering the actual context
property of a block before it is rendered.
Conceptually, this makes sense to me, and I think could be achieved by filtering the $child_context
passed to WP_Block_List
here.
In practice, however, the render_block_context
filter seems to be used to affect the context
property during rendering even in Core's own application of the filter in the comment template block and the post template block, so I do think planning a deprecation path as has been discussed would be important and would also likely require additional changes to the places where context is being dynamically filtered in certain blocks.
I've put together the following visual to help illustrate the concept of block context as it currently exists:
Restating what is in the illustration:
- Initially, the available context gets filtered before the root block is initialized (ref)
- The root block adds additional context to the available context prior to instantiating innerBlocks (ref)
- innerBlock context gets filtered during rendering, but available context is not affected by this filter (ref)
If applying the filter to available_context
is the goal, I think we should start with fixing that bug, as is the expected behavior preferred by @dlh01 in the original ticket description:
Between the two options, I would expect the filter to behave like it does in render_block(): Context supplied with a filter is made available to inner blocks.
Whether or not we then change the current ability for a block's context
property to be modified to include values that a block itself does not have listed in its 'uses_context' property can be a separate decision.
@flixos90 commented on PR #7522:
2 months ago
#69
Thanks for the helpful summary and visualization @joemcgill.
It makes sense for now to focus on only addressing the bug that $available_context
of an inner block is currently not affected and thus not correctly passed to its child blocks.
As far as I can tell, to fix that this PR is _almost_ there. It would be great to get your code review as well based on your understanding.
This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.
2 months ago
2 months ago
#71
@joemcgill, thank you for your analysis to this complex problem. I agree with the proposed path forward, where the focus moves on solving one problem at a time. In effect, we seem to have a path forward to land this PR after addressing the feedback continued by @felixarntz. I'm happy to have another look from that angle next week.
@mukesh27 commented on PR #7522:
5 weeks ago
#72
@felixarntz I have addressed the recent feedback and PR is ready for review.
I'm happy to have another look from that angle next week.
@gziolo Happy to get your feedback if you have moments. Thanks.
@mukesh27 commented on PR #7522:
4 weeks ago
#73
Thanks, @felixarntz, for the feedback! The unit tests have passed now. @joemcgill @felixarntz, this is ready for re-review and commit as the initial part of the solution.
Great findings. I was always unsure whether some of these filters should get applied in the constructor before
WP_Block_List
is instatiated. I'm wondering whether we would need to apply the filter there, too.By the way, I added some debug information on GitHub to illustrate better which path is responsible for calling filters depending on the scenario.