render_block_context another approach that solve the context issue

[mukeshpanchal27]

Trac ticket: https://core.trac.wordpress.org/ticket/62046

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[github-actions]

Test using WordPress Playground

The 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

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[dlh01]

@mukeshpanchal27 Heads up that the tests in #7344 have been updated to reflect the last few comments there.

[gziolo]

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?

[felixarntz]

@mukeshpanchal27 This looks very promising. Mostly questions from my end, to make sure I understand this correctly.

[dlh01]

Thanks for the ping! I should be able to take a look at this Wednesday or Thursday.

[mukeshpanchal27]

The PR is ready for review. Address the feedbacks.

[github-actions]

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props mukesh27, flixos90, gziolo, dlh, joemcgill, santosguillamot.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[felixarntz]

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.

[felixarntz]

@mukeshpanchal27 Holding off of my previous approval based on the valid points raised in #7522 (comment) and related comments.

For instance:

• We don't need to pass the values for the properties to the WP_Block class methods, as the filters update the values for these properties in place.
• We should separate the updates into two methods: One that updates data based on the parsed_block and another that updates data based on the context. This way we can also be more efficient about the check for whether the value changed: We only need to make updates to the two groups of dependent properties if that one property (parsed_block or context) changed.

[mukeshpanchal27]

@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

[felixarntz]

@mukeshpanchal27 Technically this almost looks good, just one problem to address (and then the failing tests).

I think we need to revise the method naming though, and their documentation as well, as it is now no longer correct.

[mukeshpanchal27]

@felixarntz @joemcgill @dlh01 @gziolo The PR is ready for review.

[mukeshpanchal27]

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?

[mukeshpanchal27]

@felixarntz @joemcgill @dlh01 @gziolo

In c83ca82, 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.

[felixarntz]

@mukeshpanchal27 This looks almost there for me, just one note.

[felixarntz]

This line doesn't look right to me. If we want to update $available_context based on $context, this should happen within the refresh_context_dependents() because then $available_context is a dependent of $context.