-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
render_block_context
test for #62046
#7344
base: trunk
Are you sure you want to change the base?
render_block_context
test for #62046
#7344
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
53e5121
to
d558031
Compare
<!-- /wp:group --> | ||
HTML | ||
); | ||
$this->assertTrue( isset( $provided_context['example'] ), 'Test block is inner block: Block context should include "example"' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Test inner block context when the provider block is a top-level block. | ||
do_blocks( | ||
<<<HTML | ||
<!-- wp:tests/context-provider --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the top level, it runs the filter inside render_block
:
wordpress-develop/src/wp-includes/blocks.php
Line 2110 in a78540b
$context = apply_filters( 'render_block_context', $context, $parsed_block, $parent_block ); |
do_blocks( | ||
<<<HTML | ||
<!-- wp:group {"layout":{"type":"constrained"}} --> | ||
<!-- wp:tests/context-provider --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, it runs the same filter in a different place:
$inner_block->context = apply_filters( 'render_block_context', $inner_block->context, $inner_block->parsed_block, $parent_block ); |
What if we merge the parent context in inner blocks context in // 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 ✅ |
@mukeshpanchal27, are you talking about the following line when considering changes to the context? wordpress-develop/src/wp-includes/class-wp-block.php Lines 496 to 497 in a78540b
What we miss is that the context is set only for the top-level block just before the class gets initialized: wordpress-develop/src/wp-includes/blocks.php Lines 2110 to 2114 in a78540b
So the best way to address it would be to, move that to |
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: Index: src/wp-includes/blocks.php
===================================================================
--- src/wp-includes/blocks.php (revision 59073)
+++ src/wp-includes/blocks.php (working copy)
@@ -2087,30 +2087,8 @@
$context['postType'] = $post->post_type;
}
- /**
- * Filters the default context provided to a rendered block.
- *
- * @since 5.5.0
- * @since 5.9.0 The `$parent_block` parameter was added.
- *
- * @param array $context Default context.
- * @param array $parsed_block {
- * An associative array of the block being rendered. See WP_Block_Parser_Block.
- *
- * @type string $blockName Name of block.
- * @type array $attrs Attributes from block comment delimiters.
- * @type array[] $innerBlocks List of inner blocks. An array of arrays that
- * have the same structure as this one.
- * @type string $innerHTML HTML from inside block comment delimiters.
- * @type array $innerContent List of string fragments and null markers where
- * inner blocks were found.
- * }
- * @param WP_Block|null $parent_block If this is a nested block, a reference to the parent block.
- */
- $context = apply_filters( 'render_block_context', $context, $parsed_block, $parent_block );
+ $block = new WP_Block( $parsed_block, $context, null, $parent_block );
- $block = new WP_Block( $parsed_block, $context );
-
return $block->render();
}
Index: src/wp-includes/class-wp-block-list.php
===================================================================
--- src/wp-includes/class-wp-block-list.php (revision 59073)
+++ src/wp-includes/class-wp-block-list.php (working copy)
@@ -42,17 +42,28 @@
protected $registry;
/**
+ * Reference to the parent block.
+ *
+ * @since 6.7.0
+ * @var WP_Block|null
+ * @access protected
+ */
+ protected $parent_block;
+
+ /**
* Constructor.
*
* Populates object properties from the provided block instance argument.
*
* @since 5.5.0
+ * @since 6.7.0 Added the optional `$parent_block` argument.
*
* @param array[]|WP_Block[] $blocks Array of parsed block data, or block instances.
* @param array $available_context Optional array of ancestry context values.
* @param WP_Block_Type_Registry $registry Optional block type registry.
+ * @param WP_Block|null $parent_block Optional. Optional. If this is a nested block, a reference to the parent block.
*/
- public function __construct( $blocks, $available_context = array(), $registry = null ) {
+ public function __construct( $blocks, $available_context = array(), $registry = null, $parent_block = null ) {
if ( ! $registry instanceof WP_Block_Type_Registry ) {
$registry = WP_Block_Type_Registry::get_instance();
}
@@ -60,6 +71,7 @@
$this->blocks = $blocks;
$this->available_context = $available_context;
$this->registry = $registry;
+ $this->parent_block = $parent_block;
}
/**
@@ -93,7 +105,7 @@
$block = $this->blocks[ $offset ];
if ( isset( $block ) && is_array( $block ) ) {
- $block = new WP_Block( $block, $this->available_context, $this->registry );
+ $block = new WP_Block( $block, $this->available_context, $this->registry, $this->parent_block );
$this->blocks[ $offset ] = $block;
}
Index: src/wp-includes/class-wp-block.php
===================================================================
--- src/wp-includes/class-wp-block.php (revision 59073)
+++ src/wp-includes/class-wp-block.php (working copy)
@@ -112,6 +112,7 @@
* its registered type will be assigned to the block's `context` property.
*
* @since 5.5.0
+ * @since 6.7.0 Added the optional `$parent_block` argument.
*
* @param array $block {
* An associative array of a single parsed block object. See WP_Block_Parser_Block.
@@ -125,8 +126,9 @@
* }
* @param array $available_context Optional array of ancestry context values.
* @param WP_Block_Type_Registry $registry Optional block type registry.
+ * @param WP_Block|null $parent_block Optional. If this is a nested block, a reference to the parent block.
*/
- public function __construct( $block, $available_context = array(), $registry = null ) {
+ public function __construct( $block, $available_context = array(), $registry = null, $parent_block = null ) {
$this->parsed_block = $block;
$this->name = $block['blockName'];
@@ -138,7 +140,27 @@
$this->block_type = $registry->get_registered( $this->name );
- $this->available_context = $available_context;
+ /**
+ * Filters the default context provided to a rendered block.
+ *
+ * @since 5.5.0
+ * @since 5.9.0 The `$parent_block` parameter was added.
+ *
+ * @param array $context Default context.
+ * @param array $parsed_block {
+ * An associative array of the block being rendered. See WP_Block_Parser_Block.
+ *
+ * @type string $blockName Name of block.
+ * @type array $attrs Attributes from block comment delimiters.
+ * @type array[] $innerBlocks List of inner blocks. An array of arrays that
+ * have the same structure as this one.
+ * @type string $innerHTML HTML from inside block comment delimiters.
+ * @type array $innerContent List of string fragments and null markers where
+ * inner blocks were found.
+ * }
+ * @param WP_Block|null $parent_block If this is a nested block, a reference to the parent block.
+ */
+ $this->available_context = apply_filters( 'render_block_context', $available_context, $block, $parent_block );
if ( ! empty( $this->block_type->uses_context ) ) {
foreach ( $this->block_type->uses_context as $context_name ) {
@@ -159,7 +181,7 @@
}
}
- $this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $registry );
+ $this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $registry, $parent_block );
}
if ( ! empty( $block['innerHTML'] ) ) {
|
@gziolo The POC is in right direction. The Failed unit tests if we apply the POC.
The second test needs to check why the filter is called an extra time, the first one needs to fix in Unit test IMO. |
As was mentioned in a comment in the Trac ticket, moving the 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 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 diff --git a/src/wp-includes/class-wp-block.php b/src/wp-includes/class-wp-block.php
index f7fd53dfc9..43d729998b 100644
--- a/src/wp-includes/class-wp-block.php
+++ b/src/wp-includes/class-wp-block.php
@@ -496,7 +496,9 @@ class WP_Block {
/** This filter is documented in wp-includes/blocks.php */
$inner_block->context = apply_filters( 'render_block_context', $inner_block->context, $inner_block->parsed_block, $parent_block );
- $block_content .= $inner_block->render();
+ $this->inner_blocks[ $index ] = new WP_Block( $inner_block->parsed_block, $inner_block->context, $this->registry );
+
+ $block_content .= $this->inner_blocks[ $index ]->render();
}
++$index; |
Intriguing alternative. It replicates how the wordpress-develop/src/wp-includes/blocks.php Lines 2110 to 2114 in a78540b
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 |
Could it be mitigated by introducing a new method on 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 |
How it help to solve the issue? |
See the comment from @dlh01 #7344 (comment). It's the same idea with a twist: diff --git a/src/wp-includes/class-wp-block.php b/src/wp-includes/class-wp-block.php
index f7fd53dfc9..43d729998b 100644
--- a/src/wp-includes/class-wp-block.php
+++ b/src/wp-includes/class-wp-block.php
@@ -496,7 +496,9 @@ class WP_Block {
/** This filter is documented in wp-includes/blocks.php */
$inner_block->context = apply_filters( 'render_block_context', $inner_block->context, $inner_block->parsed_block, $parent_block );
- $block_content .= $inner_block->render();
+ $inner_block->update_available_context( $inner_block->context );
+ $block_content .= $inner_block->render();
}
++$index; It probably doesn't even need a public method, as we could hide it behind the magic |
Here is the diff with the very quick draft that passes existing and the new tests added in this PR: Index: src/wp-includes/class-wp-block.php
===================================================================
--- src/wp-includes/class-wp-block.php (revision 59092)
+++ src/wp-includes/class-wp-block.php (working copy)
@@ -138,6 +138,19 @@
$this->block_type = $registry->get_registered( $this->name );
+ $this->update_available_context( $block, $available_context );
+
+ if ( ! empty( $block['innerHTML'] ) ) {
+ $this->inner_html = $block['innerHTML'];
+ }
+
+ if ( ! empty( $block['innerContent'] ) ) {
+ $this->inner_content = $block['innerContent'];
+ }
+ }
+
+ public function update_available_context( $block, $available_context ) {
+ $this->context = array();
$this->available_context = $available_context;
if ( ! empty( $this->block_type->uses_context ) ) {
@@ -158,17 +171,8 @@
}
}
}
-
- $this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $registry );
+ $this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $this->registry );
}
-
- if ( ! empty( $block['innerHTML'] ) ) {
- $this->inner_html = $block['innerHTML'];
- }
-
- if ( ! empty( $block['innerContent'] ) ) {
- $this->inner_content = $block['innerContent'];
- }
}
/**
@@ -505,6 +509,7 @@
/** This filter is documented in wp-includes/blocks.php */
$inner_block->context = apply_filters( 'render_block_context', $inner_block->context, $inner_block->parsed_block, $parent_block );
+ $inner_block->update_available_context( $inner_block->parsed_block, $inner_block->context );
$block_content .= $inner_block->render();
} |
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). |
@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 diff --git a/src/wp-includes/class-wp-block.php b/src/wp-includes/class-wp-block.php
index d4481a68a7..8c2928d2a6 100644
--- a/src/wp-includes/class-wp-block.php
+++ b/src/wp-includes/class-wp-block.php
@@ -138,6 +138,20 @@ class WP_Block {
$this->block_type = $registry->get_registered( $this->name );
+ $this->update_available_context( $block, $available_context );
+
+ if ( ! empty( $block['innerHTML'] ) ) {
+ $this->inner_html = $block['innerHTML'];
+ }
+
+ if ( ! empty( $block['innerContent'] ) ) {
+ $this->inner_content = $block['innerContent'];
+ }
+ }
+
+ public function update_available_context( $block, $available_context ) {
+ $this->context = array();
+
$this->available_context = $available_context;
if ( ! empty( $this->block_type->uses_context ) ) {
@@ -159,15 +173,7 @@ class WP_Block {
}
}
- $this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $registry );
- }
-
- if ( ! empty( $block['innerHTML'] ) ) {
- $this->inner_html = $block['innerHTML'];
- }
-
- if ( ! empty( $block['innerContent'] ) ) {
- $this->inner_content = $block['innerContent'];
+ $this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $this->registry );
}
}
@@ -505,6 +511,8 @@ class WP_Block {
/** This filter is documented in wp-includes/blocks.php */
$inner_block->context = apply_filters( 'render_block_context', $inner_block->context, $inner_block->parsed_block, $parent_block );
+ $inner_block->update_available_context( $inner_block->parsed_block, $inner_block->context );
+
$block_content .= $inner_block->render();
}
Unfortunately, this looks like it is causing page level context to no get passed to innerBlocks, so blocks like the |
I think I may have figured it out. When calling (tip, if you have trouble applying this, I noticed that my editor was removing the final empty line when copying this into a file) diff --git a/src/wp-includes/class-wp-block.php b/src/wp-includes/class-wp-block.php
index d4481a68a7..11ef862d71 100644
--- a/src/wp-includes/class-wp-block.php
+++ b/src/wp-includes/class-wp-block.php
@@ -138,7 +138,25 @@ class WP_Block {
$this->block_type = $registry->get_registered( $this->name );
- $this->available_context = $available_context;
+ $this->update_available_context( $block, $available_context );
+
+ if ( ! empty( $block['innerHTML'] ) ) {
+ $this->inner_html = $block['innerHTML'];
+ }
+
+ if ( ! empty( $block['innerContent'] ) ) {
+ $this->inner_content = $block['innerContent'];
+ }
+ }
+
+ public function update_available_context( $block, $available_context ) {
+ $this->context = array();
+
+ if ( null === $this->available_context ) {
+ $this->available_context = $available_context;
+ } else {
+ $this->available_context = array_merge( $this->available_context, $available_context );
+ }
if ( ! empty( $this->block_type->uses_context ) ) {
foreach ( $this->block_type->uses_context as $context_name ) {
@@ -159,15 +177,7 @@ class WP_Block {
}
}
- $this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $registry );
- }
-
- if ( ! empty( $block['innerHTML'] ) ) {
- $this->inner_html = $block['innerHTML'];
- }
-
- if ( ! empty( $block['innerContent'] ) ) {
- $this->inner_content = $block['innerContent'];
+ $this->inner_blocks = new WP_Block_List( $block['innerBlocks'], $child_context, $this->registry );
}
}
@@ -505,6 +515,8 @@ class WP_Block {
/** This filter is documented in wp-includes/blocks.php */
$inner_block->context = apply_filters( 'render_block_context', $inner_block->context, $inner_block->parsed_block, $parent_block );
+ $inner_block->update_available_context( $inner_block->parsed_block, $inner_block->context );
+
$block_content .= $inner_block->render();
}
|
As I was thinking about the When
Done and pushed to this branch. One observation regarding the |
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. |
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: wordpress-develop/src/wp-includes/blocks/post-template.php Lines 111 to 124 in 328284c
This block doesn't define 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. |
This reverts commit 6a37f03.
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 (Side note: The example from the Post Template block is extra weird because the It seems that we're dealing with a couple of inconsistencies now: Context supplied with 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. |
I just noticed that I forgot to push this update. It's there now. Sorry! |
Trac ticket: https://core.trac.wordpress.org/ticket/62046