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

HTML API: Backport updates from Core during the 6.4 release cycle #55703

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Oct 30, 2023

What?

Backports updates from Core in the HTML API

  • Remove the Gutenberg_HTML_Tag_Processor_6_3 class which no code was relying on. Code should use the _6_4 class instead.
  • Stop calling next_tag() during get_updated_html() to avoid breaking subclass logic that reimplements that method.
  • Clarify compatability loading strategy in Gutenberg's compat layer.
  • Removes lingering unit tests that live in Core and do not need to be duplicated in Gutenberg.

Why?

Keeps code in sync between the repos. Gutenberg's code should mirror that in Core and not run into trouble when the code diverges.

How?

Pulls in code that has already been reviewed and merged into Core.

Testing Instructions

Ensure that all tests pass, that the backport looks like it was performed appropriately.

There should be no visual or functional changes in this patch.

@dmsnell dmsnell added Backport from WordPress Core Pull request that needs to be backported to a Gutenberg release from WordPress Core [Feature] HTML API An API for updating HTML attributes in markup labels Oct 30, 2023
@WordPress WordPress deleted a comment from github-actions bot Oct 30, 2023
@dmsnell dmsnell force-pushed the html-api/backport-updates-from-core branch 2 times, most recently from 1010920 to 60abd78 Compare October 30, 2023 15:33
@github-actions
Copy link

github-actions bot commented Oct 30, 2023

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.4/html-api/class-gutenberg-html-tag-processor-6-4.php
❔ lib/load.php

@dmsnell dmsnell requested a review from ockham October 30, 2023 17:04
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this file? It's duplicating lib/compat/wordpress-6.4/html-api/class-wp-html-processor.php, except for the class name and class_exists check.

Copy link
Member Author

Choose a reason for hiding this comment

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

it should also be subclassing WP_HTML_Tag_Processor_6_4 as noted in load.php so it's necessary to avoid bugs introduced during the development process of 6.4

Copy link
Contributor

@ockham ockham Oct 31, 2023

Choose a reason for hiding this comment

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

it should also be subclassing WP_HTML_Tag_Processor_6_4

Ohh right, I missed that! Apologies.

as noted in load.php so it's necessary to avoid bugs introduced during the development process of 6.4

This part I don't fully grok 🤔 It would make sense to me if we'd need this to subclass WP_HTML_Tag_Processor_6_4 (or Gutenberg_, rather 😬) in order to guarantee that we're using a version of the Tag processor that has all the fixes (and potentially API changes) in place that are available in 6.4 -- in case we're running WP 6.3 🤔

Anyway, even so, I'd lean towards having GB's WP_HTML_Processor subclass Gutenberg_HTML_Tag_Processor_6_4, and indeed dropping Gutenberg_HTML_Processor_6_4.

My rationale is about as follows:
GB's compat layer is supposed to polyfill functionality available in WP 6.4 but not in 6.3. One example is GB code that relies on WP_HTML_Processor, which is why we provide it as part of the compat layer.

Since WP_HTML_Processor is only being introduced by WP 6.4, we can keep the GB polyfill in sync whenever it's updated in Core (e.g. in a 6.4.x point release), so I don't think we'll ever need a Gutenberg_HTML_Processor_6_4 🤔 Any future features that won't go into 6.4 should instead go into a prospective Gutenberg_HTML_Processor_6_5.

lib/load.php Outdated
Comment on lines 104 to 113
/*
* There was a bug in WordPress `trunk` during the 6.4 development cycle and that bug was
* introduced and fixed in the Tag Processor. Therefore, in case Gutenberg is running on
* a version of WordPress with the bug, it should run its own copy with the fix.
*
* This include needs to come after the dependencies above in case Gutenberg is running
* on WordPress 6.3 or older. The helper classes need to be loaded first.
*/
require __DIR__ . '/compat/wordpress-6.4/html-api/class-gutenberg-html-processor-6-4.php';

Copy link
Contributor

Choose a reason for hiding this comment

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

See https://github.com/WordPress/gutenberg/pull/55703/files#r1376626781 -- the file seems essentially identical to /compat/wordpress-6.4/html-api/class-wp-html-processor.php.

If it was only WP trunk that was affected by that bug at some point -- but the bug has since been fixed -- I don't think we need to provide a workaround here; I don't think we typically make that sort of guarantee when people can simply update to a later trunk. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

see above reply. given that we had a -6-3 version of the Tag Processor and given that we're going to continue to develop the HTML Processor, I feel like it's inevitable that we need this, and quickly so.

so if you think it's important to remove it I'm fine with that, but also if we remove it we'll probably need to add it in a week or whenever we have our first post-6-4 bug fix to the HTML Processor. happy to defer that until we need it, but again, I know this covers bugs that are possible right now for folks running Gutenberg between 6.3 and 6.4.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think I'd prefer removing Gutenberg_HTML_Processor_6_4 (and having GB's WP_HTML_Processor subclass Gutenberg_HTML_Tag_Processor_6_4 instead 😬 ), see my other comment.

If I'm not mistaken, we likely won't ever need a Gutenberg_HTML_Processor_6_4, as explained in that other comment 🤔 tl;dr:

  • Any bugfixes to the HTML processor we'll also want in Core's WP_HTML_Processor in the 6.4 branch (for 6.4.x point releases), so we should be able to keep that file in sync with GB's file of the same name.
  • Any new features for that HTML Processor that won't go into Core's 6.4 branch should go into Core's trunk, to be included in 6.5, and thus into Gutenberg_HTML_Processor_6_5.

@dmsnell dmsnell force-pushed the html-api/backport-updates-from-core branch 2 times, most recently from 2c419fa to 0b395c8 Compare October 30, 2023 20:18
…5703)

 - Remove the `Gutenberg_HTML_Tag_Processor_6_3` class which no code was
   relying on.
 - Stop calling `next_tag()` during `get_updated_html()` to avoid breaking
   subclass logic that reimplements that method.
 - Clarify compatability loading strategy in Gutenberg's compat layer.
 - Removes lingering unit tests that live in Core and do not need to
   be duplicated in Gutenberg.

Co-authored-by: Bernie Reiter <[email protected]>
@dmsnell dmsnell force-pushed the html-api/backport-updates-from-core branch from 8a65b5e to c44e1b4 Compare October 31, 2023 12:57
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Thank you!

@dmsnell dmsnell merged commit c44e1b4 into trunk Oct 31, 2023
49 checks passed
@dmsnell dmsnell deleted the html-api/backport-updates-from-core branch October 31, 2023 13:47
@github-actions github-actions bot added this to the Gutenberg 17.0 milestone Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport from WordPress Core Pull request that needs to be backported to a Gutenberg release from WordPress Core [Feature] HTML API An API for updating HTML attributes in markup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants