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

Only run block bindings Gutenberg logic for sites using WordPress versions below 6.5 #62363

Merged
merged 5 commits into from
Jun 7, 2024

Conversation

SantosGuillamot
Copy link
Contributor

What?

Ensure the Gutenberg block bindings logic only runs on sites using a WordPress version below 6.5.

Why?

At this moment, if users have WordPress 6.5 or above, and the Gutenberg plugin installed, block bindings are processed twice. Once by core and once by Gutenberg. This seems unnecessary because the Gutenberg logic was only added for compatibility with older WordPress versions.

How?

Checking that the class WP_Block_Bindings_Registry doesn't exist before running the block bindings compact logic in Gutenberg.

Aspects to consider

Some aspects to consider with this approach:

  • This would limit ourselves to make changes to this logic in Gutenberg first. On the other hand, I believe it should be fine to work directly in core for this. Additionally, we can always use the render_block filter again in the future if we want.
  • Is there a better way to check if the bindings are being processed? I tried checking the methods, but they are private so I believe it is not possible.
  • Is it okay to hide the existing Gutenberg functions? With this change, WordPress sites above 6.5 can't use gutenberg_block_bindings_replace_html and gutenberg_process_block_bindings. However, they were meant to be internal, so I assume that shouldn't be a problem, right?

Opinions?

Testing instructions

Before
After creating a binding, check that both WP_Block->replace_html (link) and gutenberg_block_bindings_replace_html run.

After
After creating a binding, check that only WP_Block->replace_html (link) runs.

Copy link

github-actions bot commented Jun 6, 2024

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Technical Prototype, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: [Type] Enhancement, [Type] Performance, [Feature] Block bindings.

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

1 similar comment
Copy link

github-actions bot commented Jun 6, 2024

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Technical Prototype, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: [Type] Enhancement, [Type] Performance, [Feature] Block bindings.

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

Copy link

github-actions bot commented Jun 6, 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.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org>

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

@SantosGuillamot SantosGuillamot force-pushed the update/only-run-bindings-logic-for-old-wp-versions branch from 8a37087 to 9c468b4 Compare June 6, 2024 09:38
Copy link

github-actions bot commented Jun 6, 2024

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/compat/wordpress-6.5/blocks.php

@cbravobernal
Copy link
Contributor

This would limit ourselves to make changes to this logic in Gutenberg first. On the other hand, I believe it should be fine to work directly in core for this.

Be careful with this, working directly in Core is not the fastest approach, and could be committed/tested in production environments only a few days before the next Beta. If render_block filter allows to add new iterations in the future, then is ok, that way performance checks with Gutenberg will be more accurate.

Is there a better way to check if the bindings are being processed?

IMHO checking that the class exists is really a common approach.

Is it okay to hide the existing Gutenberg functions?

Gutenberg is a plugin for feature development and experiments, but if we can, we should add a deprecation notice before not allowing to use them.

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 is what I see in the mode with whitespace changes excluded:

Screenshot 2024-06-07 at 10 09 26

I think it makes perfect sense. If we need to change the processing, we can always move this handling to a different compat folder related to a future WP major version and apply all necessary changes.

Is it okay to hide the existing Gutenberg functions?

Gutenberg is a plugin for feature development and experiments, but if we can, we should add a deprecation notice before not allowing to use them.

Internal Gutenberg functions prefixed with gutenberg_ were never documented and considered public API. Sometimes, we offer earlier access to public API, but then we use the final name to avoid the need for extenders to change their implementations.

@SantosGuillamot
Copy link
Contributor Author

Be careful with this, working directly in Core is not the fastest approach, and could be committed/tested in production environments only a few days before the next Beta. If render_block filter allows to add new iterations in the future, then is ok, that way performance checks with Gutenberg will be more accurate.

I agree with this part. However, as mentioned above, I believe we can always add the render_block filter in another compat folder if needed.

@SantosGuillamot
Copy link
Contributor Author

SantosGuillamot commented Jun 7, 2024

I can see that the "Verify Core Backport" workflow is failing: link.

However, I would say we don't need a backport for that. Any idea how to proceed here?

@gziolo gziolo merged commit 7fe18e9 into trunk Jun 7, 2024
59 of 60 checks passed
@gziolo gziolo deleted the update/only-run-bindings-logic-for-old-wp-versions branch June 7, 2024 08:58
@github-actions github-actions bot added this to the Gutenberg 18.6 milestone Jun 7, 2024
@gziolo
Copy link
Member

gziolo commented Jun 7, 2024

However, I would say we don't need a backport for that. Any idea how to proceed here?

I handled it 😄

@cbravobernal
Copy link
Contributor

However, I would say we don't need a backport for that. Any idea how to proceed here?

I handled it 😄

YOLO 😆

patil-vipul pushed a commit to patil-vipul/gutenberg that referenced this pull request Jun 17, 2024
…sions below 6.5 (WordPress#62363)

* Check if bindings are processed in core

* Change wrong check

* Move gutenberg_block_bindings_replace_html inside check

* Fix typo

* Remove extra line break

Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block bindings [Type] Enhancement A suggestion for improvement. [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants