Refactor block binding processing and attribute computation #6059

michalczaplinski · 2024-02-07T13:37:38Z

This PR refactors the processing of block bindings into 2 steps.

Before

process_block_bindings() takes the $block_content as input and:

Gets the value for each "bound" attribute from the respective source.
For each updated attribute calls replace_html() to get the updated $block_content filled in with the values from the updated attributes.

Now

process_block_bindings() doesn't take any input

Gets the value for each "bound" attribute from the respective source.
Returns the updated attributes with values from the sources.
The updated attributes are used in the render() function of the block where replace_html() is called with the updated attributes.

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

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.

src/wp-includes/class-wp-block.php

github-actions · 2024-02-07T13:58:42Z

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.

gziolo · 2024-02-07T14:06:41Z

This is great as it will allow as to use the computed value from the block bindings inside block attributes passed to the inner blocks through the $parent_block variable and as $this->attributes passed to the render_callback. Inside the render_callback we also will have $block_content that will have the bound attributes already replaced.

- wrap the replace_html with `if ( ! empty( $computed_attributes ) )` - move the processing of block bindings to the top of render() - move the logic to update `$this->attributes` inside of `process_block_bindings()`

src/wp-includes/class-wp-block.php

draganescu · 2024-02-08T08:30:01Z

Is there a way to opt out of replace_html and handle the new data coming from the binding in some other special way?

gziolo · 2024-02-08T08:45:11Z

Is there a way to opt out of replace_html and handle the new data coming from the binding in some other special way?

Can you elaborate a bit more on it? I don't think it's part of the changes, but it might be supported regardless with existing filters.

src/wp-includes/class-wp-block.php

gziolo · 2024-02-08T09:51:41Z

src/wp-includes/class-wp-block.php

@@ -411,6 +417,9 @@ public function render( $options = array() ) {
 			)
 		);

+		// Process the block bindings and get attributes updated with the values from the sources.
+		$computed_attributes = $this->process_block_bindings();


I'm comtemplating on my feedback from yesterday and started thinking whether to move back here the changes to update $this->attributes as it now unclear why this call is here.

Yeah, I think that it's a little bit better to keep that type of side-effect
directly in the render() and not hide it in the method, but it's not a strong opinion.

I agree with you. Thank you for trying the alternative approach 🙇

gziolo · 2024-02-08T10:20:59Z

I updated locally unit tests to validate that block attributes get updated correctly:

Index: tests/phpunit/tests/block-bindings/render.php
===================================================================
--- tests/phpunit/tests/block-bindings/render.php	(revision 57564)
+++ tests/phpunit/tests/block-bindings/render.php	(working copy)
@@ -62,6 +62,7 @@
 		$result   = $block->render();
 
 		$this->assertEquals( $expected, $result, 'The block content should be updated with the value returned by the source.' );
+		$this->assertSame( 'test source value', $block->attributes['content'] );
 	}
 
 	/**
@@ -97,5 +98,6 @@
 		$result   = $block->render();
 
 		$this->assertEquals( $expected, $result, 'The block content should be updated with the value returned by the source.' );
+		$this->assertSame( "The attribute name is 'content' and its binding has argument 'key' with value '$value'.", $block->attributes['content'] );
 	}
 }

We could extend the existing tests, but maybe refactor it a bit more. It would also be nice to add a test for the Image block to ensure the url gets correctly replaced when using the saved image placeholder which fixes the issue reported in WordPress/gutenberg#58425:

<!-- wp:image -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image -->

This is the test I drafted that passes:

/**
	 * Tests if the block content is updated with the value returned by the source
	 * for the Image block in the placeholder state.
	 *
	 * @ticket 60282
	 *
	 * @covers ::register_block_bindings_source
	 */
	public function test_update_block_with_value_from_source_imag_placeholder() {
		$get_value_callback = function () {
			return 'https://example.com/image.jpg';
		};

		register_block_bindings_source(
			self::SOURCE_NAME,
			array(
				'label'              => self::SOURCE_LABEL,
				'get_value_callback' => $get_value_callback,
			)
		);

		$block_content = <<<HTML
<!-- wp:image {"metadata":{"bindings":{"url":{"source":"test/source"}}}} -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image -->
HTML;

		$parsed_blocks = parse_blocks( $block_content );
		$block         = new WP_Block( $parsed_blocks[0] );

		$expected = '<figure class="wp-block-image"><img src="https://example.com/image.jpg" alt=""/></figure>';
		$result   = $block->render();

		$this->assertSame( 'https://example.com/image.jpg', $block->attributes['url'] );
		$this->assertEquals( $expected, trim( $result ), 'The block content should be updated with the value returned by the source.' );
	}

github-actions · 2024-02-08T10:34:26Z

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 czapla, gziolo, andraganescu.

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

draganescu · 2024-02-08T10:41:48Z

Can you elaborate a bit more on it? I don't think it's part of the changes, but it might be supported regardless with existing filters.

I am aware I may be asking dumb questions 😁 - so what I meant was what if I have a block and I bind and attribute to some source but I do not want core's processing on the output. Maybe my attribute is a visibility flag and the block has other attributes too bound and I'd short-circuit the binding's replace_html and handle it in my own render callback so avoid useless server work of replacing markup for no reason.

Currently the binding process is completely on, nether $block_content = $this->process_block_bindings( $block_content ); or $modified_block_content = $this->replace_html are filtered. Is it too soon to consider filters? Maybe it is.

Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>

michalczaplinski · 2024-02-08T16:31:52Z

@gziolo I've added the unit test that you drafted, thanks! 🙂

I think that this is looking pretty good so let me know your thoughts about the #6059 (comment) . I think that we can always change it in a follow-up.

gziolo

By the way, I can apply the necessary change discussed before committing tomorrow.

Great work with all refactoring this week 👏

gziolo · 2024-02-09T10:39:16Z

Committed with https://core.trac.wordpress.org/changeset/57574.

mukeshpanchal27 · 2024-02-09T10:44:57Z

src/wp-includes/class-wp-block.php

+		// If there are computed attributes, update the attributes with the
+		// computed ones.
+		if ( ! empty( $computed_attributes ) ) {
+			// Merge the computed attributes with the original attributes


Missing end period.

@gziolo ☝️

I noticed that myself, too. 🙈 I'm working on another commit and I plan to include it. Thank you as always for your attention to detail and checking new commits 🙇🏻

Done in acad00e.

gziolo · 2024-02-09T10:46:27Z

Currently the binding process is completely on, nether $block_content = $this->process_block_bindings( $block_content ); or $modified_block_content = $this->replace_html are filtered. Is it too soon to consider filters? Maybe it is.

@draganescu, we could always discuss introducing new settings for the block binding source:

filter_callback to post-process the raw value returned by the source, example: date formatting.
should_replace_callback to run before $this->replace_html() that would let folks decide whether to replace the value for an attribute in HTML.

We should be iterating on the public API as we learn about the most common needs reported by developers.

Refactor block binding processing and attribute computation

ef71ff9

gziolo requested a review from draganescu February 7, 2024 13:46

gziolo reviewed Feb 7, 2024

View reviewed changes

src/wp-includes/class-wp-block.php Outdated Show resolved Hide resolved

gziolo reviewed Feb 7, 2024

View reviewed changes

src/wp-includes/class-wp-block.php Outdated Show resolved Hide resolved

gziolo reviewed Feb 7, 2024

View reviewed changes

src/wp-includes/class-wp-block.php Outdated Show resolved Hide resolved

This was referenced Feb 7, 2024

Pattern Overrides: Image block doesn't render on frontend when it's saved as a placeholder in a pattern WordPress/gutenberg#58425

Closed

Connecting block attributes and custom fields & Block Bindings API for WP 6.5 WordPress/gutenberg#53300

Closed

michalczaplinski added 2 commits February 7, 2024 18:03

Refactor block attribute processing:

313af35

- wrap the replace_html with `if ( ! empty( $computed_attributes ) )` - move the processing of block bindings to the top of render() - move the logic to update `$this->attributes` inside of `process_block_bindings()`

Update the docstring for process_block_bindings()

849c2c8

gziolo reviewed Feb 7, 2024

View reviewed changes

src/wp-includes/class-wp-block.php Outdated Show resolved Hide resolved

gziolo reviewed Feb 8, 2024

View reviewed changes

src/wp-includes/class-wp-block.php Outdated Show resolved Hide resolved

gziolo reviewed Feb 8, 2024

View reviewed changes

michalczaplinski marked this pull request as ready for review February 8, 2024 10:34

michalczaplinski and others added 5 commits February 8, 2024 15:33

only apply changes if the block's content is not empty.

b931198

Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>

Update docstring wording

646a569

Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>

Merge branch 'trunk' into refactor/block-bindings-processing

3810ebe

Add block attribute assertions in render tests

d8e32d6

Add a new unit test for rendering the Image block

0eda9e8

gziolo approved these changes Feb 8, 2024

View reviewed changes

gziolo closed this Feb 9, 2024

mukeshpanchal27 reviewed Feb 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor block binding processing and attribute computation #6059

Refactor block binding processing and attribute computation #6059

michalczaplinski commented Feb 7, 2024 •

edited by gziolo

Loading

github-actions bot commented Feb 7, 2024

gziolo commented Feb 7, 2024

draganescu commented Feb 8, 2024

gziolo commented Feb 8, 2024

gziolo Feb 8, 2024

michalczaplinski Feb 8, 2024

gziolo Feb 8, 2024

gziolo commented Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

draganescu commented Feb 8, 2024

michalczaplinski commented Feb 8, 2024

gziolo left a comment •

edited

Loading

gziolo commented Feb 9, 2024

mukeshpanchal27 Feb 9, 2024

mukeshpanchal27 Feb 9, 2024

gziolo Feb 9, 2024

gziolo Feb 9, 2024

gziolo commented Feb 9, 2024

Refactor block binding processing and attribute computation #6059

Refactor block binding processing and attribute computation #6059

Conversation

michalczaplinski commented Feb 7, 2024 • edited by gziolo Loading

Before

Now

github-actions bot commented Feb 7, 2024

Test using WordPress Playground

Some things to be aware of

gziolo commented Feb 7, 2024

draganescu commented Feb 8, 2024

gziolo commented Feb 8, 2024

gziolo Feb 8, 2024

Choose a reason for hiding this comment

michalczaplinski Feb 8, 2024

Choose a reason for hiding this comment

gziolo Feb 8, 2024

Choose a reason for hiding this comment

gziolo commented Feb 8, 2024 • edited Loading

github-actions bot commented Feb 8, 2024 • edited Loading

draganescu commented Feb 8, 2024

michalczaplinski commented Feb 8, 2024

gziolo left a comment • edited Loading

Choose a reason for hiding this comment

gziolo commented Feb 9, 2024

mukeshpanchal27 Feb 9, 2024

Choose a reason for hiding this comment

mukeshpanchal27 Feb 9, 2024

Choose a reason for hiding this comment

gziolo Feb 9, 2024

Choose a reason for hiding this comment

gziolo Feb 9, 2024

Choose a reason for hiding this comment

gziolo commented Feb 9, 2024

michalczaplinski commented Feb 7, 2024 •

edited by gziolo

Loading

gziolo commented Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

gziolo left a comment •

edited

Loading