Enforce checks against redeclaration for functions and classes #52696

anton-vlasenko · 2023-07-17T15:54:37Z

What?

This pull request introduces checks that prevent redeclaration of Gutenberg classes and functions. More information about this can be found here: https://github.com/WordPress/gutenberg/blob/trunk/lib/README.md#wrap-functions-and-classes-with--function_exists-and--class_exists
Fixes #44151

Why?

Currently, Gutenberg allows to define PHP functions (classes) with any prefix.
This repeatedly resulted in fatal errors due to naming conflicts between Core and Gutenberg.

How?

To avoid these conflicts, this pull request adds a new Gutenberg.CodeAnalysis.GuardedFunctionAndClassNames sniff to phpcs.xml.dist.
This sniff verifies that the functions and classes are properly guarded.

Testing Instructions

Review the code and make sure that CI jobs pass.

github-actions · 2023-07-18T16:34:29Z

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/class-wp-duotone-gutenberg.php
❔ lib/class-wp-theme-json-data-gutenberg.php
❔ lib/class-wp-theme-json-gutenberg.php
❔ lib/class-wp-theme-json-resolver-gutenberg.php
❔ lib/compat/wordpress-6.2/html-api/class-wp-html-attribute-token.php
❔ lib/compat/wordpress-6.2/html-api/class-wp-html-span.php
❔ lib/compat/wordpress-6.2/html-api/class-wp-html-tag-processor.php
❔ lib/compat/wordpress-6.2/html-api/class-wp-html-text-replacement.php
❔ lib/compat/wordpress-6.2/rest-api.php
❔ lib/compat/wordpress-6.3/kses.php
❔ lib/compat/wordpress-6.3/rest-api.php
❔ lib/compat/wordpress-6.3/theme-previews.php
❔ lib/experimental/class--wp-editors.php
❔ lib/experimental/class-wp-rest-block-editor-settings-controller.php
❔ lib/experimental/class-wp-rest-customizer-nonces.php
❔ lib/experimental/interactivity-api/class-wp-directive-context.php
❔ lib/experimental/interactivity-api/class-wp-directive-processor.php
❔ lib/experimental/interactivity-api/class-wp-interactivity-store.php
❔ lib/experimental/interactivity-api/store.php
❔ lib/experimental/kses.php
❔ lib/experimental/rest-api.php
❔ lib/experiments-page.php

anton-vlasenko · 2023-07-18T16:43:14Z

I propose committing the code of the package to the lib/experimental folder. Creating a new composer package (and a GitHub repository) can be done at a later stage.

ramonjd · 2023-07-19T03:18:04Z

This is awesome, thanks @anton-vlasenko

The rules are highlighting target code:

Confirmed the functionsWhiteList works as expected.

Running phpcs --report-full --report-checkstyle=./.cache/phpcs-report.xml also flags the error. The PHP coding standards CI check runs the same command.

--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 79 | ERROR | The "allow_filter_in_styles()" function should be guarded against redeclaration.
    |       | (Gutenberg.CodeAnalysis.GuardedFunctionAndClassNames.FunctionNotGuardedAgainstRedeclaration)
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Whatever it's worth, it LGTM. Folks might have further advice/comments.

tellthemachines

Thanks for this!

Could we do something similar to check for occurrences of functions with gutenberg_ prefixes inside the block-library package? It seems every release we have to go back and rename a few 😅 (see for example #51978) so might be worth automating that too.

tellthemachines · 2023-07-19T04:35:09Z

lib/compat/wordpress-6.3/kses.php

+	 * @param bool   $allow_css Whether the CSS is allowed.
+	 * @param string $css_test_string The CSS to test.
+	 */
+	function allow_grid_functions_in_styles( $allow_css, $css_test_string ) {


This one doesn't exist in core, because we patched kses directly. I guess it could be prefixed with gutenberg instead and then it wouldn't need the function_exists check? (there may be other functions in lib/ to which the same applies)

@tellthemachines Hmm, I'm not as knowledgeable as you in the lib/compat stuff.
So, I'm asking myself, could renaming allow_filter_in_styles() to gutenberg_allow_grid_functions_in_styles() break backward compatibility?

Honestly, I would prefer if this is resolved within the scope of another PR, as I'm tryting to keep this PR focused on enforcing CI checks against redeclaraions.

I guess it could be prefixed with gutenberg instead and then it wouldn't need the function_exists check?

Been wondering about that too. Imho as a "golden rule of thumb" it would probably be safer to always do function_exists()/class_exists() in the plugin's code, no matter what! Scrapping WP betas because of old versions of Gutenberg having conflicting function/class names is no fun.

So perhaps absolutely all functions and classes in /lib should always be wrapped in a _exists, no matter what. This could eventually "teach" some contributors about the relation WP <=> plugin too :)

That's a good point! I don't think renaming would break back compat because this function only exists in the plugin, and there are no matches other than gutenberg in the wp directory. But yeah, it doesn't really matter and if leaving it as is can have an educational value, I'm happy with that!

anton-vlasenko · 2023-07-19T17:45:19Z

Thank you for looking into this PR, @tellthemachines!

Could we do something similar to check for occurrences of functions with gutenberg_ prefixes inside the block-library package? It seems every release we have to go back and rename a few 😅 (see for example #51978) so might be worth automating that too.

Yes, we could do it, but it will require implementing a new phpcs sniff because PrefixAllGlobals is not designed to work in that way.

That's because PrefixAllGlobals allows checking for "valid" prefixes, but it doesn't allow blacklisting arbitrary prefixes. The list of blacklisted prefixes is hardcoded and cannot be changed.

I've created a new GitHub issue to track this specific problem, and I would be happy to work on it after this PR is approved and merged. Would that be an acceptable solution, @tellthemachines?

If so, could you please approve this PR? 🙏

anton-vlasenko · 2023-07-19T18:09:10Z

This is awesome, thanks @anton-vlasenko

Thank you for testing this PR, @ramonjd! I appreciate it.
If that PR seems fine to you, could you please approve it? 🙏

azaozz

Thinking this looks very good! Just one question: The lib/experimental/packages/gutenberg/gutenberg-coding-standards/ path/directory is new, right? Sorry if this has been discussed somewhere else but wouldn't it be better for /gutenberg-coding-standards to be in /test or even better /test/php? Reason: /lib/experimental is for "production" code (production for the plugin), /test is for code that checks and fixes things and can change without affecting the production code.

Other than that this seems ready.

tellthemachines

LGTM!

Regarding the folders this would make sense in test/php instead of lib/experimental, but if the plan is to make it its own package/repo as mentioned here I guess it doesn't matter much 😄

See #52696 (review)

Let's be more strict to porevent potential issues.

github-actions · 2023-07-20T14:31:31Z

Flaky tests detected in 4457480.
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/5611946522
📝 Reported issues:

[Flaky Test] should toggle "lightbox" in saved attributes #52350 in /test/e2e/specs/editor/blocks/image.spec.js

anton-vlasenko · 2023-07-20T14:38:20Z

Thank you very much for your comments and approving the PR, @tellthemachines and @azaozz !
I really appreciate that.

I've moved the composer package to test/php and made the checks stricter by removing exceptions for WP_.+Gutenberg classes.
For example, WP_Duotone_Gutenberg now needs to be guarded since it doesn't have the Gutenberg_ prefix.
The reason for this change is that I'm afraid someone will forget to remove the _Gutenberg postfix and backport the class in that form.
If anyone complains, this exception can be added back anytime. Anyway, this is on me.

…ding-strategy * origin/trunk: (59 commits) Promisify action creator return type for WP data dispatch (#52530) [RNMobile] Add WP hook for registering non-core blocks (#52791) removes check for active preview device type to enable the fixed toolbar preference (#52770) Enforce checks against redeclaration for functions and classes (#52696) update appearance tools, (#52785) Behaviors: Extend Global Styles API to read/write behaviors config. (#52370) HeaderToolbar - Update inserterMethod meta data (#52735) add options for debugging php unit tests (#52778) Docs: Interactivity API > Getting Started Guide - minor adjustments (#52786) Footnotes: Use static closures when not using '' (#52781) Improve slug generation & matching in request utils (#52414) Open "docs" folder for the Interactivity API package and Getting Started Guide (#52462) Global Styles: Don't use named arguments for 'sprintf' (#52782) E2E utils - Update locator to hide the keyboard on iOS to pick the first element, on iPad two buttons are available and the second one makes the floating keyboard to show up (#52771) Patterns: Reinstate template parts mode spec (#52780) chore(release): publish Update changelog files Patterns: Fix empty general template parts category (#52747) Add id to pattern inserted notice to stop multiple notices stacking (#52746) Site Editor: Fix site link accessibility issues (#52744) ...

tellthemachines · 2023-07-21T01:08:16Z

For example, WP_Duotone_Gutenberg now needs to be guarded since it doesn't have the Gutenberg_ prefix.
The reason for this change is that I'm afraid someone will forget to remove the _Gutenberg postfix and backport the class in that form.

Thanks, that's a sensible approach!

dmsnell · 2023-07-22T00:46:18Z

thanks @anton-vlasenko - this should help quite a bit.

after all I've looked into the issue, I'm still left confused about when exactly it pops up, and what exactly we should be doing to fix it. glad at least now some automation flags it.

was it difficult to get this put together? asking because I'm curious about other code checks and if it was straightforward, maybe there are other opportunities to piggy-back on the WPCS code.

anton-vlasenko · 2023-07-25T11:20:28Z

@dmsnell

thanks @anton-vlasenko - this should help quite a bit.

Thanks!

was it difficult to get this put together?

Writing a PHPCS sniff wasn't very hard, especially when I realized that most of the functionality needed to parse tokens is already available. However, creating a composer package with the necessary file structure and ensuring that everything works was more challenging.

anton-vlasenko mentioned this pull request Jul 17, 2023

Enforce checks against redeclaration for functions and classes #50042

Closed

anton-vlasenko marked this pull request as ready for review July 17, 2023 15:59

anton-vlasenko requested review from TimothyBJacobs, spacedmonkey and geriux as code owners July 17, 2023 15:59

anton-vlasenko added the [Type] Code Quality Issues or PRs that relate to code quality label Jul 18, 2023

anton-vlasenko requested review from ockham, adamziel, ramonjd, hellofromtonya, azaozz, tellthemachines and dmsnell July 18, 2023 16:43

tellthemachines reviewed Jul 19, 2023

View reviewed changes

anton-vlasenko mentioned this pull request Jul 19, 2023

Enforce valid function names in packages/block-library/*/src/index.php files #52769

Closed

anton-vlasenko requested a review from tellthemachines July 19, 2023 18:09

azaozz approved these changes Jul 19, 2023

View reviewed changes

tellthemachines approved these changes Jul 20, 2023

View reviewed changes

anton-vlasenko added 6 commits July 20, 2023 15:49

Add Gutenberg coding standards package.

6cccfb4

Fix contributors.

81b0852

Fix package names.

6ebaf07

Remove unneccessary section.

887fab8

Add Gutenberg coding standards package as a composer dependency.

d91104d

Add myself as a code owner of the coding standards package.

3e7fa24

anton-vlasenko added 12 commits July 20, 2023 15:53

Guard functions and classes.

eee0473

Guard functions and classes.

f9c8746

Guard functions and classes.

4f50af3

Guard functions and classes.

4681443

Guard functions and classes.

73c6aae

Guard functions and classes.

a9d3806

Guard functions and classes.

f3da181

Guard functions and classes.

686d3e0

Revert code style changes back.

51e7ebe

Fix cs errors.

ad92156

Move the package to test/php.

42ec0e9

See #52696 (review)

Don't whitelist "WP.+Gutenberg" classes.

27dc135

Let's be more strict to porevent potential issues.

anton-vlasenko force-pushed the try/gutenberg-coding-standards branch from a1f68c1 to 27dc135 Compare July 20, 2023 13:54

anton-vlasenko added 2 commits July 20, 2023 15:56

Update code owners.

a088af5

Fix indents.

4457480

anton-vlasenko merged commit 3615722 into trunk Jul 20, 2023

anton-vlasenko deleted the try/gutenberg-coding-standards branch July 20, 2023 14:48

github-actions bot added this to the Gutenberg 16.4 milestone Jul 20, 2023

anton-vlasenko mentioned this pull request Jul 21, 2023

Fonts Library: Backend #52704

Merged

oandregal mentioned this pull request Aug 14, 2023

Try: bundle WP_REST_Global_Styles_Controller class instead of inheriting per WordPress version #53618

Merged

anton-vlasenko mentioned this pull request Aug 21, 2023

[PHP Tests] Increase parity with WordPress Core #43333

Open

12 tasks

mikachan mentioned this pull request Sep 5, 2023

Plugin: Synchronize PHP changes for WordPress 6.4 release #54177

Closed

86 tasks

mikachan added the Needs PHP backport Needs PHP backport to Core label Sep 5, 2023

SiobhyB removed the Needs PHP backport Needs PHP backport to Core label Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce checks against redeclaration for functions and classes #52696

Enforce checks against redeclaration for functions and classes #52696

anton-vlasenko commented Jul 17, 2023 •

edited

Loading

github-actions bot commented Jul 18, 2023 •

edited

Loading

anton-vlasenko commented Jul 18, 2023

ramonjd commented Jul 19, 2023

tellthemachines left a comment

tellthemachines Jul 19, 2023

anton-vlasenko Jul 19, 2023 •

edited

Loading

azaozz Jul 19, 2023 •

edited

Loading

tellthemachines Jul 20, 2023

anton-vlasenko commented Jul 19, 2023 •

edited

Loading

anton-vlasenko commented Jul 19, 2023 •

edited

Loading

azaozz left a comment •

edited

Loading

tellthemachines left a comment

github-actions bot commented Jul 20, 2023

anton-vlasenko commented Jul 20, 2023 •

edited

Loading

tellthemachines commented Jul 21, 2023

dmsnell commented Jul 22, 2023 •

edited

Loading

anton-vlasenko commented Jul 25, 2023

Enforce checks against redeclaration for functions and classes #52696

Enforce checks against redeclaration for functions and classes #52696

Conversation

anton-vlasenko commented Jul 17, 2023 • edited Loading

What?

Why?

How?

Testing Instructions

github-actions bot commented Jul 18, 2023 • edited Loading

anton-vlasenko commented Jul 18, 2023

ramonjd commented Jul 19, 2023

tellthemachines left a comment

Choose a reason for hiding this comment

tellthemachines Jul 19, 2023

Choose a reason for hiding this comment

anton-vlasenko Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

azaozz Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

tellthemachines Jul 20, 2023

Choose a reason for hiding this comment

anton-vlasenko commented Jul 19, 2023 • edited Loading

anton-vlasenko commented Jul 19, 2023 • edited Loading

azaozz left a comment • edited Loading

Choose a reason for hiding this comment

tellthemachines left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 20, 2023

anton-vlasenko commented Jul 20, 2023 • edited Loading

tellthemachines commented Jul 21, 2023

dmsnell commented Jul 22, 2023 • edited Loading

anton-vlasenko commented Jul 25, 2023

anton-vlasenko commented Jul 17, 2023 •

edited

Loading

github-actions bot commented Jul 18, 2023 •

edited

Loading

anton-vlasenko Jul 19, 2023 •

edited

Loading

azaozz Jul 19, 2023 •

edited

Loading

anton-vlasenko commented Jul 19, 2023 •

edited

Loading

anton-vlasenko commented Jul 19, 2023 •

edited

Loading

azaozz left a comment •

edited

Loading

anton-vlasenko commented Jul 20, 2023 •

edited

Loading

dmsnell commented Jul 22, 2023 •

edited

Loading