Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block bindings: Ensure the block receives the fully expanded __default bindings when rendering #7394

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Sep 18, 2024

Trac ticket: https://core.trac.wordpress.org/ticket/62069
Gutenberg issue: WordPress/gutenberg#64688

What

Fixes an issue with the image block when using pattern overrides caused by a bug in the block binding logic.

Why does the bug happen?

The pattern overrides feature is built upon block bindings, and uses a special __default binding that means all block attributes that support binding are bound:

bindings: {
    __default: 'core/pattern-overrides'
}

During processing of the bindings, this single __default binding is replaced with the individual binding attributes - e.g.:

bindings: {
    src: 'core/pattern-overrides',
    id: 'core/pattern-overrides',
    caption: 'core/pattern-overrides',
}

In the gutenberg plugin this updated bindings metadata is assigned back to the block before rendering so that the block can reason about which individual attributes are bound:
https://github.com/WordPress/gutenberg/blob/98b8d415830fa9ebf7b4b0a2b95d65b9fd1e813a/lib/compat/wordpress-6.6/blocks.php#L40

This allows blocks like the image block to check whether an individual attribute, like id, has a binding:
https://github.com/WordPress/gutenberg/blob/98b8d415830fa9ebf7b4b0a2b95d65b9fd1e813a/packages/block-library/src/image/index.php#L31

Unfortunately core doesn't have the same logic to assign the updated binding metadata back to the block before rendering, which means the image block's logic fails. The block only receives the individual __default binding in its metadata.

How has it been fixed?

The fix in this PR is to ensure that the process_block_bindings method returns any updates to the block's binding metadata along with other computed attributes.

Prior to rendering, the block's attributes are updated with the result of this method (it's where the binding attribute values are updated for a block):

// Process the block bindings and get attributes updated with the values from the sources.
$computed_attributes = $this->process_block_bindings();
if ( ! empty( $computed_attributes ) ) {
// Merge the computed attributes with the original attributes.
$this->attributes = array_merge( $this->attributes, $computed_attributes );
}

So this will achieve the same result as the code in the Gutenberg plugin.

Steps for reproduction

  1. Add an image block to a new post and add an image to the block
  2. Preview the post and inspect the image, make a note of the image's id in the classname attribute (e.g. wp-image-123)
  3. Back in the editor, click on the image block, go to settings, and Create Pattern (make it a synced pattern)
  4. Edit the pattern by clicking 'Edit original'
  5. Click the image block and go to Advanced > Enable Overrides
  6. Enter a name for the block and enable the override
  7. Save Pattern and return to post
  8. Click on the image block in the pattern and replace with a new image
  9. Save page and view on front-end
  10. Inspect the image and check that the id in the classname was updated to the new image's id
  11. Check that the image has html additional attributes like fetchpriority (which are missing in trunk)

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.

@talldan talldan added the bug label Sep 18, 2024
@talldan talldan self-assigned this Sep 18, 2024
Copy link

github-actions bot commented Sep 18, 2024

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 talldanwp, cbravobernal, santosguillamot, mukesh27, gziolo.

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

Copy link

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.

@talldan talldan force-pushed the fix/ensure-rendered-block-receives-fully-expanded-default-bindings branch 3 times, most recently from f6cecd3 to e6d9c31 Compare September 18, 2024 11:44
@talldan
Copy link
Contributor Author

talldan commented Sep 18, 2024

There are two failing e2e tests, but they also seem to be failing in trunk.

Copy link

@SantosGuillamot SantosGuillamot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good in my testing. It seems to solve the mentioned issue, and the rest of use cases seem to work as expected. Code makes sense to me as well. I just left a small comment about where it could be placed.

I also checked that the unit test fails without the proposed solution, and it passes with it.

src/wp-includes/class-wp-block.php Outdated Show resolved Hide resolved
@talldan talldan force-pushed the fix/ensure-rendered-block-receives-fully-expanded-default-bindings branch from cd2f3f0 to 3fa50c2 Compare September 19, 2024 15:28
Comment on lines 288 to 291
/*
* Update the bindings metadata of the computed attributes.
* This ensures the block receives the expanded __default binding metadata when it renders.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/*
* Update the bindings metadata of the computed attributes.
* This ensures the block receives the expanded __default binding metadata when it renders.
*/
/*
* Update the bindings metadata of the computed attributes.
* This ensures the block receives the expanded __default binding metadata when it renders.
*/

@@ -272,31 +272,41 @@ public function test_using_symbols_in_block_bindings_value() {
}

/**
* Tests if the `__default` attribute is replaced with real attribues for
* Tests if the `__default` attribute is replaced with real attributes for
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@gziolo
Copy link
Member

gziolo commented Sep 23, 2024

That makes perfect sense to update the metadata.bindings for the __default special case together with computed values for attributes 👍🏻

@cbravobernal
Copy link
Contributor

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

@talldan talldan deleted the fix/ensure-rendered-block-receives-fully-expanded-default-bindings branch September 30, 2024 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants