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 API: Refactor logic into Block Bindings class and singleton pattern #57742

Merged
merged 16 commits into from
Jan 17, 2024

Conversation

artemiomorales
Copy link
Contributor

@artemiomorales artemiomorales commented Jan 10, 2024

What?

This refactors the experimental Block Bindings API, first introduced in #57249, into a singleton pattern as recommended in this discussion. Please also see related tracking issue #53300.

Why?

We want to ensure the Block Bindings API is in line with WordPress standards as we look to prepare this for WP 6.5.

How?

It creates a new Block Bindings class and singleton pattern, modeled on the implementation of the Modules API.

Testing Instructions

Please follow the same testing instructions from #57249 and ensure that everything still works as expected — this is the same functionality, just refactored.

Copy link

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/experimental/block-bindings/block-bindings.php
❔ lib/experimental/block-bindings/index.php
❔ lib/experimental/block-bindings/sources/pattern.php
❔ lib/experimental/block-bindings/sources/post-meta.php
❔ lib/experimental/blocks.php
❔ lib/experimental/block-bindings/class-wp-block-bindings.php

Base automatically changed from add/block-bindings-php-registration-mechanisms to trunk January 11, 2024 14:17
@artemiomorales artemiomorales force-pushed the update/improve-block-bindings-api branch from 2b724d6 to 27cfb49 Compare January 12, 2024 04:07
Copy link
Contributor Author

@artemiomorales artemiomorales left a comment

Choose a reason for hiding this comment

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

Some open questions I have for reviewers:

  1. Where is the best place to put the class-wp-block-bindings.php and block-bindings.php files?
  2. Should we have the Block Bindings logic spread out between these two files, or should we consolidate this into a single file and follow the singleton pattern established in Block Pattern Categories or Block Styles instead? (Right now this is modeled after the Modules API)
  3. How do the function signatures and docblocks look? Now is a good time to improve all of that.

Everything seems to be working as far as I can tell, and I think this is good enough to start a conversation and see if I'm in the right ballpark. Any thoughts appreciated! 🙏

@artemiomorales artemiomorales changed the title Move bindings logic to Block Bindings class Block Bindings API: Refactor logic into Block Bindings class and use singleton pattern Jan 12, 2024
@artemiomorales artemiomorales changed the title Block Bindings API: Refactor logic into Block Bindings class and use singleton pattern Block Bindings API: Refactor logic into Block Bindings class and singleton pattern Jan 12, 2024
@artemiomorales artemiomorales added [Feature] Block API API that allows to express the block paradigm. [Type] New API New API to be used by plugin developers or package users. [Feature] Custom Fields Anything related to the custom fields project - connecting block attributes and dynamic values labels Jan 12, 2024
@artemiomorales artemiomorales marked this pull request as ready for review January 12, 2024 04:39
Comment on lines 173 to 175
public function get_allowed_blocks() {
return $this->allowed_blocks;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just thinking about a future where this mechanism may change (it becomes an opt-in or opt-out, or we support so many core blocks that it becomes easier to list the ones that aren't supported), and ways that the method could be future proofed.

Maybe changing it to something like get_is_block_type_supported( $block_type ) could be a way to future proof it. That would prevent leaking the allowed_blocks array into public code and allow us to change the way it's implemented while keeping the interface the same.

Copy link
Member

Choose a reason for hiding this comment

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

I definitely agree with the rationale. What do you think about going even further and making it an implementation detail of the function that takes care of computing the value of the attributes that have a corresponding custom source binding? If there are no attribute changes then there is nothing much to process in the block content's HTML.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think what Dan suggests makes total sense. The allowed_blocks property is already private so if we use a function like get_is_block_type_supported( $block_type ) instead it should be enough to "future proof" it.

I'm not sure I follow your suggestion @gziolo though. Could you elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we go with this, we should also update the wrapper function in:

if ( ! function_exists( 'wp_block_bindings_get_allowed_blocks' ) ) {
function wp_block_bindings_get_allowed_blocks() {
return wp_block_bindings()->get_allowed_blocks();
}
}

Copy link
Member

@gziolo gziolo Jan 15, 2024

Choose a reason for hiding this comment

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

What I meant is that with more refactoring, you can make the list of allowed blocks and implementation details of process_block_bindings. The only other place where the same list is currently needed is:

foreach ( $block_bindings_allowed_blocks as $block_name => $attributes ) {
	add_filter( 'render_block_' . $block_name, 'process_block_bindings', 20, 3 );
}

However, I don't see any reason why it couldn't become:

add_filter( 'render_block', 'process_block_bindings', 20, 3 );

The same check would happen in the filter instead before it starts any processing:

function process_block_bindings( $block_content, $attributes, $block_instance ) {
	$allowed_blocks = array(
		'core/paragraph' => array( 'content' ),
		'core/heading'   => array( 'content' ),
		'core/image'     => array( 'url', 'title', 'alt' ),
		'core/button'    => array( 'url', 'text' ),
	);
 	if ( empty( $attributes['metadata']['bindings'] ) ||  ! isset(  $allowed_blocks[ $block_instance->name ] ) ) {
 		// stop processing
	}
 	// continue processing
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Greg! Yeah, that makes sense to me. 👍

There's no need to expose wp_block_bindings_get_allowed_blocks() as public API so we can remove it as well as remove the internal $this->get_allowed_blocks() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I moved the $allowed_blocks property to the filter function and removed the loop 👍
Perhaps the motivation of the original code was to prevent running the filter function on every block on the page?

In any case, I think what I've revised here addresses the issue. Let me know what you think!

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This PR is heading in a good direction. Let's address the feedback raised by @talldan.

We should also start looking at adding unit tests for the public API so we can finalize its shape. It feels like another PR might be better for that as we would focus more on that seperately.

Copy link
Contributor

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

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

Where is the best place to put the class-wp-block-bindings.php and block-bindings.php files?

I think that for now the location is fine but we will want to move the files out of the experimental folder when the Block
Bindings are not an experiment anymore. And once we merge with Core, we'll have to move the files again anyway to someplace that is appropriate for Core.

Should we have the Block Bindings logic spread out between these two files, or should we consolidate this into a single file and follow the singleton pattern established in Block Pattern Categories or Block Styles instead? (Right now this is modeled after the Modules API)

The Modules have the same requirement as Block Bindings (have a singleton) so the structure seems good to me.

How do the function signatures and docblocks look? Now is a good time to improve all of that.

I'm happy with them but open to more feedback!

*
* Support for custom fields in blocks.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We should update this since Block Bindings are not only about custom fields
(e.g. Partially Synced Patterns).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I updated the description here: aa566cc

How does that look?

Copy link
Member

@kevin940726 kevin940726 left a comment

Choose a reason for hiding this comment

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

Looks good as a refactor (except for an error mentioned below)! We can discuss and solve other issues in follow-ups IMO.


// Allowed blocks that support block bindings.
// TODO: Look for a mechanism to opt-in for this. Maybe adding a property to block attributes?
$allowed_blocks = array(
Copy link
Member

Choose a reason for hiding this comment

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

We have a similar check in block-supports/pattern.php as seen here: https://github.com/WordPress/gutenberg/pull/57789/files#r1452055620. Do you think we should duplicate this array there? Or should we move this into a sharable JSON config so that it can also be imported by JS? Not sure if it's worth the effort though since it's supposed to be a temporary solution.

Copy link
Member

Choose a reason for hiding this comment

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

I think question number one is whether these two lists should always match. Eventually, it should be possible to use block bindings with all blocks and I expect to see a method to opt in for that. The prerequisite is the full support in HTML API to replace HTML tags in the saved content.

Copy link
Contributor Author

@artemiomorales artemiomorales Jan 16, 2024

Choose a reason for hiding this comment

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

Hmm, where would we put the JSON config? My initial reaction is that it's not worth the effort because this is a temporary solution. The temporary solution may be in place for a while, though.

If we do add a JSON config, that means we'd need to make sure support for every block works for both regular blocks and patterns moving forward. To @gziolo's point, that would require more coordination and perhaps may be less productive at this stage.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I was basically tossing out three different solutions above 😅:

  1. Duplicate the $allowed_blocks config in block-supports/pattern.php.
  2. Move the constant to a sharable PHP file that can be imported by both PHP scripts.
  3. Go a bit further and move the constant to a sharable JSON file that can also be imported by client-side JS.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think duplicating it is ok for now.

I've put together a PR that does that - Update pattern overrides to use a hard coded support array

It should ensure pattern overrides don't break when this PR is merged (I think this PR is currently a little bit behind trunk).

}

add_filter( 'render_block', process_block_bindings, 20, 3 );
Copy link
Member

Choose a reason for hiding this comment

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

This throws an error when I tested this:

Undefined constant "process_block_bindings"

Should this be:

Suggested change
add_filter( 'render_block', process_block_bindings, 20, 3 );
add_filter( 'render_block', 'process_block_bindings', 20, 3 );

instead?

Copy link
Member

Choose a reason for hiding this comment

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

That definitely should be a string. In addition, it's going to be Gutenberg specific helper method so it should be prefixed with gutenberg_. In the WordPress core we will be able to tap into internal classes instead of using the hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I changed it to a string and renamed it in e6473a7

So strange that I don't see the error on my machine though 🤔 Seems to work as intended regardless.

Anyway, it's been fixed 🙏

Copy link
Contributor

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

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

Tested it 👌

Code-wise looks good to me too.

@michalczaplinski michalczaplinski merged commit 81c3c96 into trunk Jan 17, 2024
56 checks passed
@michalczaplinski michalczaplinski deleted the update/improve-block-bindings-api branch January 17, 2024 12:22
@github-actions github-actions bot added this to the Gutenberg 17.6 milestone Jan 17, 2024
@getdave getdave added the Needs PHP backport Needs PHP backport to Core label Jan 22, 2024
@getdave
Copy link
Contributor

getdave commented Jan 22, 2024

I believe this is being backported in https://core.trac.wordpress.org/ticket/60282 / WordPress/wordpress-develop#5888.

Thank you for working on this.

Once the code is merged into WP Core please could you

  • check off the line items in the PHP Sync Tracking Issue
  • remove the Needs PHP backport label
  • add the Backported to WP Core label

Thank you in advance 🙇

@michalczaplinski michalczaplinski added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Needs PHP backport Needs PHP backport to Core labels Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Feature] Block API API that allows to express the block paradigm. [Feature] Custom Fields Anything related to the custom fields project - connecting block attributes and dynamic values [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants