Implement a lint rule that ensures that functions/classes with no "gutenberg_" prefix are properly guarded

[anton-vlasenko]

What problem does this address?

Currently, it's possible to define a PHP function (class) with any prefix.
This repeatedly resulted in fatal errors due to duplicate function names between Core and Gutenberg.

What is your proposed solution?

A new lint rule should be implemented.
It should check that PHP functions/classes with no (G|g)utenberg_ prefix are properly guarded with function_exists() or class_exists().

[anton-vlasenko]

I'm working on implementing this rule.

[anton-vlasenko]

@noisysocks
@adamziel

I'm looking for feedback before implementing the solution for this issue.

Let me briefly summarize it:
According to this doc, PHP functions/classes that start with a prefix other than (G|g)utenberg_ should be wrapped in function_exists() or class_exists() (i.e., "guarded") to avoid naming collisions with Core.

Does writing a new phpcs sniff that checks whether the functions/classes are properly guarded seem valid to you?

@adamziel, I know you've worked on a script that checks prefixes in Gutenberg functions.
Is there anything you can share on this matter?
Could some of your code be reused to solve the issue?
Also, could your script be modified so that implementing a new wpcs sniff wouldn't be needed?

[adamziel]

@anton-vlasenko thanks for the ping!

I'm looking for feedback before implementing the solution for this issue.

The linter rule is a great idea which would save everyone a ton of headache!

It also makes me think – can we find 3 or 4 last instances of this problem to learn if any other measures would help here? For example, do we need any other linter rules other than one checking for the gutenberg_ prefix? Would a small change in the CI configuration make something flash red in case we run into the fatal errors again?

I know you've worked on a script that checks prefixes in Gutenberg functions.

That ended up being just an idea, I've never gotten around to building a script :-( I only built some automations for releasing new WordPress versions, which now makes me think – wouldn't it be great if the release leads knew away about the naming conflicts the moment they're introduced? I'm thinking about some kind of smoke automated test to install the latest Gutenberg plugin and visit a few most important pages, perhaps even in the CI for PRs somehow marked as core merge.

Also cc @gziolo

[anton-vlasenko]

I haven't forgotten about this issue. But I apologise it took me so long to reply, @adamziel.

I'm thinking about some kind of smoke automated test to install the latest Gutenberg plugin and visit a few most important pages, perhaps even in the CI for PRs somehow marked as core merge.

It also makes me think – can we find 3 or 4 last instances of this problem to learn if any other measures would help here? For example, do we need any other linter rules other than one checking for the gutenberg_ prefix? Would a small change in the CI configuration make something flash red in case we run into the fatal errors again?

I believe that the e2e tests should have failed in that situation.
That's what these e2e tests do: they install Gutenberg and click on certain buttons and links.

The problem is that such issues can remain unnoticed for a long time, which could cause many websites to go down. This can happen because a function or class with a conflicting name might be backported to Core unbeknownst to the Gutenberg CI jobs.

Consequently, Gutenberg CI jobs will not provide advance warnings to developers.
Only when the error occurs will Gutenberg CI jobs fail.

So, in my opinion, this should be prevented in advance with the help of a PHPCS sniff that would enforce function_exist and class_exists checks.

The linter rule is a great idea which would save everyone a ton of headache!

Thank you!
I added such a rule, but my pull request was understandably rejected because the sniff I implemented is specific to Gutenberg.
I would like to create a new GitHub repository for this sniff since my pull request cannot be merged into the WordPress Coding Standards repository.
What is your opinion on this?

[adamziel]

A separate repo would do it! The price to pay would be a new chore to update it periodically. I like the namespace suggestion in the discussion under your PR, though, maybe that’s all we need? Is there a sniff to require using a namespace in all files?

[anton-vlasenko]

A separate repo would do it! The price to pay would be a new chore to update it periodically.

Thank you for the reply, @adamziel. I agree with that.
I could take responsibility for updating that repository.

I like the namespace suggestion in the discussion under your PR, though, maybe that’s all we need?

Namespaces might work.
However, there might be an issue with using namespaces.
Currently, there is no way to enforce them, similar to guarding functions and classes.
If a developer forgets to guard a function or a class, naming conflicts and fatal errors are inevitable.
The same could happen with namespaces. A developer can simply forget to add a namespace to a file.
Therefore, a PHPCS sniff that enforces either a (function|class)_exists() check or namespaces could be beneficial.

I've submitted a PR that adds such a sniff (and added you as a reviewer).
Also, I created a new repository for tracking Gutenberg-specfic sniffs.
I'd be grateful if you could share your feedback on that matter, @adamziel.
Thank you.

[ockham]

FWIW, there's a Trac ticket for this in Core: https://core.trac.wordpress.org/ticket/56794 😊

[ockham]

BTW to name one specific scenario:

Dynamic block code (in packages/block-library/src/*/index.php) may reference gutenberg_-prefixed functions during development, especially if they're defined in lib/block-supports. However, when those blocks are merged to Core, we want to avoid that prefix, and use wp_ instead.

Since the dynamic block code is actually copied over to Core from the @wordpress/block-library package, we have a solution to change the code to use the wp_ prefix even in Gutenberg's code base, and to add the function name to a list of functions whose prefixes will be rewritten back to gutenberg_ by Webpack when blocks are built for Gutenberg.

Here's an example of the above scenario: #51989

[anton-vlasenko]

BTW to name one specific scenario:
Dynamic block code (in packages/block-library/src/*/index.php) may reference gutenberg_-prefixed functions

Thank you for flagging this issue, @ockham !
I've created a follow-up GH issue to track it: #52769