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: Add canUpdateBlockBindings editor setting #7258

Conversation

SantosGuillamot
Copy link

@SantosGuillamot SantosGuillamot commented Aug 28, 2024

Needed for WordPress/gutenberg#64570

Adds a canUpdateBlockBindings editor setting that will be read to decide if the user should be able to create and modify bindings through the UI. By default, only admin users can do it, but it can be overridden with block_editor_settings_all filter.

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


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.

Copy link

Trac Ticket Missing

This pull request is missing a link to a Trac ticket. For a contribution to be considered, there must be a corresponding ticket in Trac.

To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. More information about contributing to WordPress on GitHub can be found in the Core Handbook.

Copy link

github-actions bot commented Aug 28, 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 santosguillamot, gziolo, jorbin, noisysocks, matveb, cbravobernal, youknowriad, mamaduka, timothyblynjacobs, peterwilsoncc, drivingralle.

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.

@cbravobernal cbravobernal self-assigned this Aug 28, 2024
@@ -665,6 +665,8 @@ function get_block_editor_settings( array $custom_settings, $block_editor_contex
}
}

$editor_settings['canUpdateBlockBindings'] = current_user_can( 'manage_options' );
Copy link
Member

Choose a reason for hiding this comment

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

What’s the rationale for using manage_options here? This permission mostly controls access to setting pages in the admin interface.

Copy link
Author

@SantosGuillamot SantosGuillamot Aug 29, 2024

Choose a reason for hiding this comment

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

I just selected one of the admin capabilities according to this docs: https://wordpress.org/documentation/article/roles-and-capabilities/. manage_options seemed the most similar use case. But maybe it can be checked another way, maybe using wp_get_current_user()->roles. Not sure if that'd be better.

Copy link
Member

@gziolo gziolo Aug 29, 2024

Choose a reason for hiding this comment

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

I asked on WordPress Slack (also here) whether this would work.

Copy link
Member

Choose a reason for hiding this comment

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

  1. Please don't check against a role. The roles can differ between sites and also aren't always 100% equal in a site. For example, some sites add a role that could be thought of as in between editor and admin that might be able to manage options but not users. Some sites give contributors the ability to upload media. I've also seen sites that rename roles to fit their organization structure. Capabilities are always the correct thing to check.

  2. Why does this need to be a higher capability than edit_post for that specific post? While I absolutely see the benefit of a separate cap and I think a manage_bindings that maps to edit_post with the specific post_id included would allow the locking down of it that is necessary for some sites.

Copy link
Member

@gziolo gziolo Aug 29, 2024

Choose a reason for hiding this comment

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

While I absolutely see the benefit of a separate cap and I think a manage_bindings that maps to edit_post with the specific post_id included would allow the locking down of it that is necessary for some sites.

The biggest challenge is with site editor settings as on the client, the user might interact with many templates, template parts, etc, so we would have to check that capability through REST API somehow if we want to contextualize that based on post id. I don't know too much about that are of WordPress, but happy to discuss possibilities.

Copy link
Author

@SantosGuillamot SantosGuillamot Sep 5, 2024

Choose a reason for hiding this comment

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

I have pushed this commit to see how mapping a capability could look like. I mapped it to manage_options, but I can change it to whatever we decide.

I believe there are these options:

  1. Map manage_block_bindings to admin capability like manage_options and use that to define the canUpdateBlockBindings editor setting: This would allow only admin users by default managing bindings, but admin caps don't seem too related.
  2. Map manage_block_bindings to editor capability like edit_others_posts and use that to define the editor setting: This would allow an editor to directly manage bindings. It seems it isn't clear yet if this should be the case, though.
  3. Map manage_block_bindings to editor capability like edit_others_posts, but set canUpdateBlockBindings to false by default, and add a preference in the editor to enable/disable it: This would allow us to keep the bindings UI disabled by default unless an admin explicitly sets the preference.

For all the use cases, site owners can always change the behavior by modifying the capability or the editor setting:

Modifying the capability

function restrict_manage_block_bindings_cap( $caps, $cap ) {
	if ( 'manage_block_bindings' === $cap ) {
		return array( 'do_not_allow' );
	}
	return $caps;
}

add_filter( 'map_meta_cap', 'restrict_manage_block_bindings_cap', 10, 4 );

Modifying the editor setting

function restrict_editor_setting( $editor_settings ) {
	$editor_settings['canUpdateBlockBindings'] = false;
	return $editor_settings;
}

add_filter( 'block_editor_settings_all', 'restrict_editor_setting', 10 );

Personally, I'd go with option 1 or 3 as they seem more restrictive while flexible. And it would give us some time to receive more feedback on the expected behavior. I'd love to know your thoughts on the different possibilities.

Copy link
Member

Choose a reason for hiding this comment

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

Option (2) is what I'd expect.

Restricting to admins as a way of slowly rolling out the feature doesn't really make sense to me. Editors should be able to do everything that an admin can do bar manage other users.

If we want to slow down the roll out of the feature so that we have more time for feedback and testing then we should do that in more traditional ways: an experimental flag, keeping the feature only in the Gutenberg plugin, requiring the theme to opt-in, etc.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for approaching it through capabilities. This is not that different from toggling meta fields on and creating new ones in the classic editor context.

This is aside from any work to improve the interface should it not test well with users.

Copy link
Member

Choose a reason for hiding this comment

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

This is not that different from toggling meta fields on and creating new ones in the classic editor context.

In that case, the edit_post_meta capability is the right one.

That said, I think @noisysocks is correct and if the UI/UX needs refinement, hiding it behind a capability check isn't the right away to encourage it.

Copy link
Author

Choose a reason for hiding this comment

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

In that case, the edit_post_meta capability is the right one

I am not sure about this. Although post meta is the most relevant case for block bindings, the idea is that block attributes can be connected to other sources, not just post meta. Additionally, I believe connecting to post meta and editing post meta are different things.

For those reasons, I was suggesting using something more general as edit_posts or edit_other_posts.

@peterwilsoncc
Copy link
Contributor

I've posted a comment on the associated issue Core-61945 (comment) -- tl;dr is that I think this is solving the wrong problem given the earlier comment that the interface is too technical.

@peterwilsoncc
Copy link
Contributor

peterwilsoncc commented Sep 11, 2024

Sorry folks, can I get some help on testing this. I followed the instructions in the GB PR but without success.

These are the steps I took:

  1. Set up:
  2. Log in as an admin
  3. Build GB
  4. Add new page
  5. Insert paragraph, unable to see attributes
  6. Insert second paragraph, still no luck
  7. Activate Jetpack after seeing a comment on the GB PR
  8. Add an image, no attributes.

I'm sure it's a problem at my end, can someone let me know what I am missing.

@cbravobernal
Copy link
Contributor

Sorry folks, can I get some help on testing this. I followed the instructions in the GB PR but without success.

These are the steps I took:

1. Set up:
   
   * WP 6.6.2
   * Gutenberg trunk @ [WordPress/gutenberg@8a74d31](https://github.com/WordPress/gutenberg/commit/8a74d31f28e4d5ddc6f21be1bc2134e675d2913f)

2. Log in as an admin

3. Build GB

4. Add new page

5. Insert paragraph, unable to see attributes

6. Insert second paragraph, still no luck

7. Activate Jetpack after seeing a comment on the GB PR

8. Add an image, no attributes.

I'm sure it's a problem at my end, can someone let me know what I am missing.

You may need to register a meta data. Like this:

function add_bindings() {
register_meta(
		'post',
		'all_templates_key',
		array(
			'show_in_rest'      => true,
			'single'            => true,
			'type'              => 'string',
			'default'           => 'all templates default value',
		)
	);
}

add_action( 'init', 'add_bindings');

If the meta is for a post it will appear in a post. If the meta is for a page, then you need to register with the page key.

Jetpack was adding a private custom field, that's the reason why it made the selector to appear before. We fix that, that's the reason why it didn't work for you anymore.

Here is a small video demo:

Screen.Recording.2024-09-12.at.11.52.31.mov

@peterwilsoncc
Copy link
Contributor

Thanks @cbravobernal I knew I must have been missing something so I appreciate the help.

@gziolo
Copy link
Member

gziolo commented Sep 13, 2024

I am circling back to the Dev Chat, September 11, 2024 that I couldn't attend. I want to share more context.

My main concern here is that the approach is to hide the UI from users with low permissions. I don’t feel that this is a great approach to handling a UI that is considered too technical, as I don’t think there is anything to suggest that an administrator will understand what an author does not.

So I’m of the view the interface ought to be improved and made less technical before it’s shipped in core.

Block Bindings UI for connecting sources with block attributes will be ready with optimized user experience on time for WP 6.7. We had some minor concerns initially about how to present post meta options in the UI in the sidebar and for editable fields, but we did several iterations and plan to extend post meta to offer human-friendly labels to show in the editor. I think that was the primary concern we had (related report) as seeing post meta keys like themeslug_book_rating will never be the most efficient for processing by human brain (despite being helpful for debugging in some cases).

I’d be fine with just updating this to use caps. The interface doesn’t strike me as being too technical. Can put it in the Advanced tab if we’re worried…

…The short of it is that I’m okay with fixing the cap issue (add a new cap, don’t check against a role) and shipping in 6.7 or leaving it in the plugin for more testing. Up to the team working on it. We have until beta 1 to decide.

The goal of this ticket/PR was to define the best strategy for handling the feature's exposure. We want to choose the best defaults that can be further optimized by the site owners. The panel for managing bindings would be only confusing when presented to all users, in particular those that only care about editing data. We also anticipated that on some sites, it will be disabled for nearly everyone as it is considered an advanced feature for site building. For example, when the intention is to expose block variations or patterns with bound attributes, the consumer should have a streamlined experience when editing values. However, the person editing the pattern’s design in the editor should be able to connect these attributes with binding sources. That’s basically what we want to offer with proper permissions, and that’s why we contemplated a custom capability.

@Drivingralle
Copy link

I think a custom capability is the best way. With that projects can decide what role can do the work. No matter what the core decides.
I personal would limit this feature to the role of admin and editors. For my understanding an editor is a role for handling content but not structure.

@SantosGuillamot
Copy link
Author

Given that the UI is being improved in other pull requests to make it more user-friendly, I agree we could focus this ticket/PR on discussing the best strategy to decide when to show or not the UI to create and modify bindings.

I still believe that adding a manage_block_bindings capability makes sense and, according to the feedback, it seems that mapping it to other capabilities like edit_posts or edit_other_posts could be enough. That way, it'd be available for admins and editors by default, but site owners can override this behavior if wanted.

@peterwilsoncc
Copy link
Contributor

I think meta capabilities is the way to go.

Similar to post types, we probably want multiple caps for editing own, editing others, etc. This will allow developers maximum flexibility.

To account for different permissions with CPTs, we'd probably want them to require the post ID, eg current_user_can( 'edit_block_binding', $post_id ) mapping to $post_type->caps->edit_others_posts. The edit_post meta capability will provide a guide.

Naming things is hard but typically meta capabilities are singular while primitive capabilities are plural.

@SantosGuillamot
Copy link
Author

Thanks for the guidance 🙂

To account for different permissions with CPTs, we'd probably want them to require the post ID

The main issue with this is that bindings can be edited in the site editor in templates, where a post ID doesn't exist. I checked and it seems in the site editor we are checking the edit_theme_options capability: link.

Would it make sense to fallback to that one when there is no post ID and we are in the context of the site editor? I made a commit trying to show what I mean: 24951bc

@@ -665,6 +665,8 @@ function get_block_editor_settings( array $custom_settings, $block_editor_contex
}
}

$editor_settings['canUpdateBlockBindings'] = current_user_can( 'edit_block_binding', $block_editor_context->post->ID );
Copy link
Author

Choose a reason for hiding this comment

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

This would need a safety check in case $block_editor_context->post doesn't exist.

Copy link
Member

@gziolo gziolo Sep 17, 2024

Choose a reason for hiding this comment

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

I don't think it's going to be reliable to use post ID. It will only work correctly for the existing post in the post editor. It is impossible to correctly detect the post id for the site editor as everything happens dynamically on the client.

What I mean, exposing it here as a setting calculated on the server has limitations.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it seemed complicated when I was looking through the code and use cases. That's why, if we want to check against a post ID, I was adding a fallback to edit_theme_options capability: https://github.com/WordPress/wordpress-develop/pull/7258/files#diff-7b99cb99a8105f5254c9c282a8cb9eed9d9b48f2d974544ce6919fe124bd0427R813

That way, we don't try to detect the post id, if it isn't provided in the context and we are in the site editor, we map the edit_block_binding capability to edit_theme_options.

Having said that, maybe it is just easier to not pass a post ID and map it directly to other capabilities.

Copy link
Member

Choose a reason for hiding this comment

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

This would need a safety check in case $block_editor_context->post doesn't exist.

Confirmed:

Copy link
Author

Choose a reason for hiding this comment

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

This should be solved in this commit: a6a8d21

@noisysocks
Copy link
Member

Hey @SantosGuillamot. JavaScript packages were updated in r59072 so you might wish to rebase this for easier testing. The deadline to commit this backport is 6.7 Beta 1 which is scheduled for 1 October.

@SantosGuillamot SantosGuillamot force-pushed the add/editor-settings-to-update-bindings branch from f4f31c6 to ae3b1be Compare September 20, 2024 07:41
@SantosGuillamot
Copy link
Author

Rebased done 🙂 I believe I already have an idea of what needs to be done once we agree on the path forward. Let's see if we can agree on the mapping capabilities.

*/
if ( ! isset( $args[0] ) ) {
$screen = get_current_screen();
if ( ! isset( $screen->id ) || 'site-editor' !== $screen->id ) {
Copy link
Member

Choose a reason for hiding this comment

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

If the post id gets passed as an argument, then probably it's easier to pass also the name of the editor:

Or even the entire block editor context.

Copy link
Author

Choose a reason for hiding this comment

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

I've used the entire block editor context as suggested in this commit: a6a8d21

@SantosGuillamot
Copy link
Author

I've addressed the changes suggested and added a unit test to check the capabilities are mapped properly. Let's see if the unit tests pass.

With these changes, this is how it should work:

  • If it is in the context of a post, and post->ID exists, it maps to edit_post.
  • If it is in the context of a template, and post->IDDOES NOT exist, it maps to edit_theme_options.

Let me know if that makes sense, or if I should change anything.

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.

I would be in favor of starting with that strategy.

Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
@@ -665,6 +665,8 @@ function get_block_editor_settings( array $custom_settings, $block_editor_contex
}
}

$editor_settings['canUpdateBlockBindings'] = current_user_can( 'edit_block_binding', $block_editor_context );
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a new setting and not something you access through coreData canUser or something?

Copy link
Author

Choose a reason for hiding this comment

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

The main reason we started using a new setting was to allow users to disable this behavior with a PHP filter if wanted. And it seems there was a precedence for this with canLockBlocks: link.

It's true that user capabilities can also be modified through a filter, so if we are able to access edit_block_binding in the editor somehow, we wouldn't need an editor setting.

I couldn't find a way to do so, but I must say I'm not familiar with user capabilities.

I'm happy to explore other paths. Is it possible to access these user capabilities? From what I understood, canUser checks whether the user can perform the given action on the given REST resource. But that's not exactly what we need here.

Another possibility to replicate the capabilities mapping in JS, but I am not sure it makes sense and I believe users wouldn't be able to override this behavior easily.

Any thoughts are more than welcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are a lot of precedents for block editor settings indeed. But IMO, we should start moving away from them like I explained on my recent blog posts about Gutenberg practices.

In this case, it's a capability check and we should just have a unique way to fetch capabilities from the client. I think @Mamaduka might know better whether that's already possible or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity, I'm not saying that users shouldn't override the capability in php, I'm saying that the frontend should access the capability through the capability APIs and not block editor settings.

Copy link
Author

Choose a reason for hiding this comment

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

I'm saying that the frontend should access the capability through the capability APIs and not block editor settings.

If we can make it work this way, I agree it seems the best path forward. I will take another look, but I couldn't find a way to do that, at least with the existing APIs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the difference with the global settings is that this endpoint is user specific. So maybe a "user settings" endpoint.

Copy link
Contributor

Choose a reason for hiding this comment

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

Writing that, I wonder if that's just the user endpoint though :P

Copy link
Member

Choose a reason for hiding this comment

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

If I'm not wrong, that endpoint has never made it into Core.

Yeah looks like it's still GB only. We should probably merge that, I can't remember any specific reasons why we held off on it.

I'm personally convinced this is not user preferences though.

I'm not too fussed if it's a "preference" or a "setting". But I do think it's not a capability.

I think we need a consistent global settings endpoint in Core.

Along the lines of wp-block-editor/v1/settings?

I know we used the "root" endpoint for that sometimes. Maybe we should just continue using that

I think this is less of an index type of thing, since it is so spceific to Block Editor UI, and not the general capabiliites of the site.

I guess the difference with the global settings is that this endpoint is user specific. So maybe a "user settings" endpoint. Writing that, I wonder if that's just the user endpoint though :P

I don't think it would be odd, IMO, for wp-block-editor/v1/settings to take into account the user when building the settings. It's already something that I imagine extenders are using when filtering.

Copy link
Contributor

@youknowriad youknowriad Sep 27, 2024

Choose a reason for hiding this comment

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

I don't think it would be odd, IMO, for wp-block-editor/v1/settings to take into account the user when building the settings. It's already something that I imagine extenders are using when filtering.

Just to be clear, I think a block editor settings endpoint would work here. But I wonder if it's a bit shortsighted in the sense that we'll have a need for similar settings (user settings) outside the block editor. (For instance, a setting to prevent users from creating custom WP-Admin views or a setting to prevent users from using "filters" in the different admin pages, or random things like that)...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah we should start exploring that in more detail.

@youknowriad
Copy link
Contributor

What's clear here is that the right solution here is still unclear and there are still diverging opinions. If It helps unblock this PR, I wouldn't mind personally this is landing as a block editor setting.

Now, my personal opinion is that whether this applies to the block editor only or not is irrelevant, we want to control the ability of the user to perform a task or not. So for me, this is a user capability (in the large sense) and the ideal for me is that we should have a unique way in core-data to access user capabilities. (I'm less opinionated about the backend and REST API, how user capabilities are managed there is not something I have expertise on)

@cbravobernal
Copy link
Contributor

cbravobernal commented Sep 30, 2024

@cbravobernal
Copy link
Contributor

cbravobernal commented Sep 30, 2024

There has been some discussion about whether the ability to update block bindings in the editor should be a setting/preference or a capability.

This ability should be filterable by external developers, which suggests the editor setting approach. However, it is not a user setting, as the user cannot choose whether or not to update block bindings—it is only enabled for admins or filtered by third-party extensions.

Overuse of editor settings is not recommended by Gutenberg practices, so adding a capability could work. However, it has not been merged yet.

Considering there is no clear right solution, and the editor setting is theoretically a valid approach, I have committed it so it doesn't block the adoption of block bindings. Additionally, it is a helpful feature for site admins, as shown, for example, in this comment:
WordPress/gutenberg#64570 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.