Make WordPress Core

Opened 5 months ago

Closed 3 weeks ago

Last modified 2 weeks ago

#62046 closed defect (bug) (fixed)

`render_block_context` filter works differently on top-level vs. inner blocks

Reported by: dlh's profile dlh Owned by: joemcgill's profile joemcgill
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

#2 @dlh
5 months ago

  • Keywords has-patch removed

This ticket was mentioned in Slack in #core-editor by dlh. View the logs.


5 months ago

#4 @gziolo
5 months ago

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.

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.

This ticket was mentioned in Slack in #core by obliviousharmony. View the logs.


5 months ago

#6 @obliviousharmony
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 to WP_Block::__construct() so that it can be available to the above filters.
  • Move the $postId and $postType injection from render_block into a create_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?

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 ✅

@gziolo commented on PR #7344:


5 months ago
#8

@mukeshpanchal27, are you talking about the following line when considering changes to the context?

https://github.com/WordPress/wordpress-develop/blob/a78540b0881a195ec8488bb6bdf878114d75f8b8/src/wp-includes/class-wp-block.php#L496-L497

---

What we miss is that the context is set only for the top-level block just before the class gets initialized:

https://github.com/WordPress/wordpress-develop/blob/a78540b0881a195ec8488bb6bdf878114d75f8b8/src/wp-includes/blocks.php#L2110-L2114

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.

@gziolo commented on PR #7344:


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

     
    20872087                $context['postType'] = $post->post_type;
    20882088        }
    20892089
    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 );
    21112091
    2112         $block = new WP_Block( $parsed_block, $context );
    2113 
    21142092        return $block->render();
    21152093}
    21162094
  • src/wp-includes/class-wp-block-list.php

     
    4242        protected $registry;
    4343
    4444        /**
     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        /**
    4554         * Constructor.
    4655         *
    4756         * Populates object properties from the provided block instance argument.
    4857         *
    4958         * @since 5.5.0
     59         * @since 6.7.0 Added the optional `$parent_block` argument.
    5060         *
    5161         * @param array[]|WP_Block[]     $blocks            Array of parsed block data, or block instances.
    5262         * @param array                  $available_context Optional array of ancestry context values.
    5363         * @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.
    5465         */
    55         public function __construct( $blocks, $available_context = array(), $registry = null ) {
     66        public function __construct( $blocks, $available_context = array(), $registry = null, $parent_block = null ) {
    5667                if ( ! $registry instanceof WP_Block_Type_Registry ) {
    5768                        $registry = WP_Block_Type_Registry::get_instance();
    5869                }
     
    6071                $this->blocks            = $blocks;
    6172                $this->available_context = $available_context;
    6273                $this->registry          = $registry;
     74                $this->parent_block      = $parent_block;
    6375        }
    6476
    6577        /**
     
    93105                $block = $this->blocks[ $offset ];
    94106
    95107                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 );
    97109
    98110                        $this->blocks[ $offset ] = $block;
    99111                }
  • src/wp-includes/class-wp-block.php

     
    112112         * its registered type will be assigned to the block's `context` property.
    113113         *
    114114         * @since 5.5.0
     115         * @since 6.7.0 Added the optional `$parent_block` argument.
    115116         *
    116117         * @param array                  $block             {
    117118         *     An associative array of a single parsed block object. See WP_Block_Parser_Block.
     
    125126         * }
    126127         * @param array                  $available_context Optional array of ancestry context values.
    127128         * @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.
    128130         */
    129         public function __construct( $block, $available_context = array(), $registry = null ) {
     131        public function __construct( $block, $available_context = array(), $registry = null, $parent_block = null ) {
    130132                $this->parsed_block = $block;
    131133                $this->name         = $block['blockName'];
    132134
     
    138140
    139141                $this->block_type = $registry->get_registered( $this->name );
    140142
    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 );
    142164
    143165                if ( ! empty( $this->block_type->uses_context ) ) {
    144166                        foreach ( $this->block_type->uses_context as $context_name ) {
     
    159181                                }
    160182                        }
    161183
    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 );
    163185                }
    164186
    165187                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%
        )

@dlh commented on PR #7344:


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 { 
    496496                                                /** This filter is documented in wp-includes/blocks.php */
    497497                                                $inner_block->context = apply_filters( 'render_block_context', $inner_block->context, $inner_block->parsed_block, $parent_block );
    498498
    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();
    500502                                        }
    501503
    502504                                        ++$index;

@gziolo commented on PR #7344:


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:

https://github.com/WordPress/wordpress-develop/blob/a78540b0881a195ec8488bb6bdf878114d75f8b8/src/wp-includes/blocks.php#L2110-L2114

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.

@gziolo commented on PR #7344:


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 like update_available_context ? A rough sketch of the idea:

How it help to solve the issue?

@gziolo commented on PR #7344:


5 months ago
#17

Could it be mitigated by introducing a new method on WP_Block like update_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 { 
    496496                                                /** This filter is documented in wp-includes/blocks.php */
    497497                                                $inner_block->context = apply_filters( 'render_block_context', $inner_block->context, $inner_block->parsed_block, $parent_block );
    498498
    499                                                 $block_content .= $inner_block->render();
     499                                                $inner_block->update_available_context( $inner_block->context );
     500                                                $block_content .= $inner_block->render();
    500501                                        }
    501502
    502503                                        ++$index;

@gziolo commented on PR #7344:


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

     
    138138
    139139                $this->block_type = $registry->get_registered( $this->name );
    140140
     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 ) {
    141153                $this->available_context = $available_context;
    142154
    143155                if ( ! empty( $this->block_type->uses_context ) ) {
     
    158170                                        }
    159171                                }
    160172                        }
    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 );
    163174                }
    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                 }
    172175        }
    173176
    174177        /**
     
    505508                                                /** This filter is documented in wp-includes/blocks.php */
    506509                                                $inner_block->context = apply_filters( 'render_block_context', $inner_block->context, $inner_block->parsed_block, $parent_block );
    507510
     511                                                $inner_block->update_available_context( $inner_block->parsed_block, $inner_block->context );
    508512                                                $block_content .= $inner_block->render();
    509513                                        }

@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 { 
    138138
    139139                $this->block_type = $registry->get_registered( $this->name );
    140140
     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
    141155                $this->available_context = $available_context;
    142156
    143157                if ( ! empty( $this->block_type->uses_context ) ) {
    class WP_Block { 
    159173                                }
    160174                        }
    161175
    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 );
    171177                }
    172178        }
    173179
    class WP_Block { 
    505511                                                /** This filter is documented in wp-includes/blocks.php */
    506512                                                $inner_block->context = apply_filters( 'render_block_context', $inner_block->context, $inner_block->parsed_block, $parent_block );
    507513
     514                                                $inner_block->update_available_context( $inner_block->parsed_block, $inner_block->context );
     515
    508516                                                $block_content .= $inner_block->render();
    509517                                        }

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 { 
    138138
    139139                $this->block_type = $registry->get_registered( $this->name );
    140140
    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                }
    142160
    143161                if ( ! empty( $this->block_type->uses_context ) ) {
    144162                        foreach ( $this->block_type->uses_context as $context_name ) {
    class WP_Block { 
    159177                                }
    160178                        }
    161179
    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 );
    171181                }
    172182        }
    173183
    class WP_Block { 
    505515                                                /** This filter is documented in wp-includes/blocks.php */
    506516                                                $inner_block->context = apply_filters( 'render_block_context', $inner_block->context, $inner_block->parsed_block, $parent_block );
    507517
     518                                                $inner_block->update_available_context( $inner_block->parsed_block, $inner_block->context );
     519
    508520                                                $block_content .= $inner_block->render();
    509521                                        }

@dlh commented on PR #7344:


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?

@gziolo commented on PR #7344:


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.

@gziolo commented on PR #7344:


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:

https://github.com/WordPress/wordpress-develop/blob/328284c0865be63b906042be0585a022c4f2f5e1/src/wp-includes/blocks/post-template.php#L111-L124

This block doesn't define providesContext:

https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/blocks/post-template/block.json

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 @gziolo
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 @joemcgill
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?

@dlh commented on PR #7344:


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

#31 @mukesh27
4 months ago

The PR #7522 solved the unit tests.

@dlh commented on PR #7344:


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!

@dlh commented on PR #7522:


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.

@gziolo commented on PR #7522:


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

@dlh commented on PR #7522:


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.

@gziolo commented on PR #7522:


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

@dlh commented on PR #7522:


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 the inner_html and inner_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.

@dlh commented on PR #7522:


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 @flixos90
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 @joemcgill
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 @dlh
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.

Last edited 3 months ago by dlh (previous) (diff)

@dlh commented on PR #7522:


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's uses_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's uses_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 @gziolo
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.

@gziolo commented on PR #7522:


3 months ago
#57

Existing usage of render_block_context in WordPress core:

https://github.com/WordPress/wordpress-develop/blob/dec5c906459c5cdebe7630fb6a61bcbf3e4344b9/src/wp-includes/block-template.php#L311-L324

https://github.com/WordPress/wordpress-develop/blob/dec5c906459c5cdebe7630fb6a61bcbf3e4344b9/src/wp-includes/blocks/block.php#L76-L90

https://github.com/WordPress/wordpress-develop/blob/dec5c906459c5cdebe7630fb6a61bcbf3e4344b9/src/wp-includes/blocks/comment-template.php#L29-L50

https://github.com/WordPress/wordpress-develop/blob/dec5c906459c5cdebe7630fb6a61bcbf3e4344b9/src/wp-includes/blocks/post-template.php#L101-L123

---

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 with providesContext 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:

https://github.com/WordPress/wordpress-develop/blob/dec5c906459c5cdebe7630fb6a61bcbf3e4344b9/src/wp-includes/class-wp-block.php#L52-L59

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:

https://github.com/WordPress/wordpress-develop/blob/dec5c906459c5cdebe7630fb6a61bcbf3e4344b9/src/wp-includes/class-wp-block.php#L44C2-L50C28

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.

@gziolo commented on PR #7522:


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.

@gziolo commented on PR #7522:


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

     
    21632163                $context['postType'] = $post->post_type;
    21642164        }
    21652165
     2166        $block = new WP_Block( $parsed_block, $context );
     2167
    21662168        /**
    21672169         * Filters the default context provided to a rendered block.
    21682170         *
     
    21832185         * }
    21842186         * @param WP_Block|null $parent_block If this is a nested block, a reference to the parent block.
    21852187         */
    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 );
    21872189
    2188         $block = new WP_Block( $parsed_block, $context );
    2189 
    21902190        return $block->render();
    21912191}
    21922192
  • src/wp-includes/class-wp-block.php

     
    138138
    139139                $this->block_type = $registry->get_registered( $this->name );
    140140
    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 );
    142143
    143144                if ( ! empty( $this->block_type->uses_context ) ) {
    144145                        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.

@gziolo commented on PR #7522:


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

     
    21632163                $context['postType'] = $post->post_type;
    21642164        }
    21652165
     2166        $block = new WP_Block( $parsed_block, $context );
     2167
    21662168        /**
    21672169         * Filters the default context provided to a rendered block.
    21682170         *
     
    21832185         * }
    21842186         * @param WP_Block|null $parent_block If this is a nested block, a reference to the parent block.
    21852187         */
    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 );
    21872189
    2188         $block = new WP_Block( $parsed_block, $context );
    2189 
    21902190        return $block->render();
    21912191}
    21922192
  • src/wp-includes/class-wp-block.php

     
    138138
    139139                $this->block_type = $registry->get_registered( $this->name );
    140140
    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 );
    142143
    143144                if ( ! empty( $this->block_type->uses_context ) ) {
    144145                        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 what WP_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.

@gziolo commented on PR #7522:


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 implements ArrayAccess 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?

@gziolo commented on PR #7522:


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.

@gziolo commented on PR #7522:


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:

https://github.com/user-attachments/assets/ada626e5-431e-4cdd-99fb-7a99321589f7

Restating what is in the illustration:

  1. Initially, the available context gets filtered before the root block is initialized (ref)
  2. The root block adds additional context to the available context prior to instantiating innerBlocks (ref)
  3. 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

@gziolo commented on PR #7522:


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.

#74 @joemcgill
3 weeks ago

  • Keywords commit added
  • Owner set to joemcgill
  • Status changed from new to assigned

#75 @joemcgill
3 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 59662:

Editor: Improve consistency of render_block_context filter.

This ensures that when block context is filtered via render_block_context, the filtered value is provided as available context to inner blocks.

For backwards compatibility reasons, filtered context is added to inner block context regardless of whether that block has declared support via the uses_context property.

Props mukesh27, flixos90, gziolo, dlh, joemcgill, santosguillamot.
Fixes #62046.

#76 @audrasjb
2 weeks ago

  • Keywords needs-dev-note added

Marking as needs-dev-note.

Note: See TracTickets for help on using tickets.