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: Move into 6.2 Compat Folder since inclusion in Core #47749

Merged
merged 2 commits into from
Feb 7, 2023

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Feb 3, 2023

Status

Is there any guidance on moving PHP support into lib/compat? I ran around like a self-driving taxi without its sensors trying to mimic what I saw, the worst kind of imitation. This is probably all wrong.

What?

Moves the HTML Tag Processor out of experimental and into combat/wordpress-6.2

Why?

The Tag Processor was merged into 6.2. It needs to load only when not already present from WordPress itself.

How?

Copies files from the merged Core code into lib/compat/wordpress-6.2 and updates the load.php file to conditionally load the compatibility class.

Testing Instructions

Since code in Gutenberg relies on the Tag Processor tests should start failing if something goes wrong. Given that this is merged in Core, ensure that the files copied over are the appropriate files and do not differ from what's in Core. Ensure that they load when running on pre-6.2 versions of WordPress.

Since the merge of the Tag Processor into Core in [55203][55203]
it's no longer needed in Gutenberg in the experimental directory.
In this patch we're moving a version of the Tag Processor which
was updated during the Core merge into the `lib/compat` directory
so that we can continue to provide it where necessary while
avoiding conflicts that might otherwise arise by leaving it
where it was.

[55203]: https://core.trac.wordpress.org/changeset/55203
@dmsnell dmsnell force-pushed the html-api/move-to-compat-layer branch from 0ce3a5c to 9e8d9ba Compare February 3, 2023 20:46
@dmsnell dmsnell marked this pull request as ready for review February 3, 2023 20:47
@noisysocks
Copy link
Member

This looks right to me!

Looks like the lint errors are due to not passing 'gutenberg' as the second argument to __.

Are there any changes that were made to the tag processor in Core before being committed that should be copied back into the plugin?

@gziolo
Copy link
Member

gziolo commented Feb 6, 2023

Looks like the lint errors are due to not passing 'gutenberg' as the second argument to __.

GitHub CLI is oversensitive in this case. We need to apply the same exceptions as we already use for core blocks that accept translations without the second argument.

This place:

gutenberg/phpcs.xml.dist

Lines 28 to 30 in 862fe67

<rule ref="WordPress.WP.I18n.MissingArgDomainDefault">
<exclude-pattern>packages/block-library/src/*</exclude-pattern>
</rule>

In fact, I don't think there are many cases where we want the second argument to be gutenberg so maybe we should flip the check and list only files where we want to see the second argument.

@dmsnell
Copy link
Member Author

dmsnell commented Feb 6, 2023

Are there any changes that were made to the tag processor in Core before being committed that should be copied back into the plugin?

@noisysocks the code in this PR/branch diverges significantly from its merge base and those changes are coming back from Core. If it looks like there's nothing different it's likely because GitHub recently decided to hide the most important parts of PR diffs so they could prune out some DB calls 🙃 try clicking on "Load Diff" and you will see it.

@dmsnell
Copy link
Member Author

dmsnell commented Feb 6, 2023

@gziolo @noisysocks it'd be nice to get this in soon before it starts causing conflicts with Core, but do we need to resolve the linter issues here? is there a clear directive on what needs to happen before merging? I don't understand if I should be appeasing the linter or filing an exception or fixing the linter's rules.

`WordPress.WP.I18n.MissingArgDomainDefault` should not trigger for code that polyfills features present in WordPress core.
@gziolo
Copy link
Member

gziolo commented Feb 7, 2023

@gziolo @noisysocks it'd be nice to get this in soon before it starts causing conflicts with Core, but do we need to resolve the linter issues here? is there a clear directive on what needs to happen before merging? I don't understand if I should be appeasing the linter or filing an exception or fixing the linter's rules.

I'm trying 6765520 to see if that silences the linting "issues" that are false positives. Again, there should be no second param for the translations used in the code that polyfills functionality present in WordPress core. If that causes trouble, I will try to improve it on my local machine 😅

@gziolo gziolo added Backport from WordPress Core Pull request that needs to be backported to a Gutenberg release from WordPress Core [Feature] Block API API that allows to express the block paradigm. labels Feb 7, 2023
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.

Everything looks good.

@gziolo gziolo enabled auto-merge (squash) February 7, 2023 09:14
@gziolo gziolo merged commit e7f096e into trunk Feb 7, 2023
@gziolo gziolo deleted the html-api/move-to-compat-layer branch February 7, 2023 09:30
@github-actions github-actions bot added this to the Gutenberg 15.2 milestone Feb 7, 2023
@github-actions
Copy link

github-actions bot commented Feb 7, 2023

Flaky tests detected in 6765520.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4112194725
📝 Reported issues:

@juanmaguitar juanmaguitar added the [Type] Code Quality Issues or PRs that relate to code quality label Feb 15, 2023
@juanmaguitar juanmaguitar mentioned this pull request Feb 15, 2023
33 tasks
ockham added a commit to WordPress/block-interactivity-experiments that referenced this pull request Feb 27, 2023
During preparation for the WP 6.2 release, the HTML Tag Processor files inside the Gutenberg plugin were [moved](WordPress/gutenberg#47749) from `lib/experimental/html/` to `lib/compat/wordpress-6.2/html-api/`.

This PR updates the import locations accordingly, and sort of "centralizes" them, so we only reference the external dependency in one file (and can potentially later to point to e.g. `lib/compat/wordpress-6.3/html-api/class-gutenberg-html-tag-processor-6-3.php`, which has some features added after the WP 6.2 Feature Freeze, if needed).
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] Block API API that allows to express the block paradigm. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants