Implement a PHPCS sniff to check for redeclaration of functions and classes

[anton-vlasenko]

What?

This pull request introduces a new PHPCS sniff that checks for redeclaration of functions and classes.

Why?

1. Please refer to this coding standard to understand why this sniff is necessary: https://github.com/WordPress/gutenberg/blob/trunk/lib/README.md#wrap-functions-and-classes-with--function_exists-and--class_exists
2. Since Gutenberg is part of Core (as explained in this post: https://make.wordpress.org/core/2022/08/18/wordpress-development-setup/), its coding standards should align with WordPress coding standards. However, this does not necessarily mean that this sniff must be included in the WordPress-Core's ruleset.xml.

Below are some additional details that can help understand the purpose of this PR:
Gutenberg developers should be cautious when defining functions and classes to ensure that they are not already defined in Core. Failure to do so has led to fatal errors in the past.

The new sniff will identify cases where developers forget to use ! function_exists and ! class_exists when creating new functions and classes. This will enhance the quality of the code and minimize conflicts with other plugins and themes.

By wrapping functions and classes with ! function_exists and ! class_exists, these conflicts can be avoided, and fatal errors can be prevented.

Testing Instructions

1. Verify that the CI jobs pass (ignore the failing job as it's not related to this PR).
2. There is already a unit test that checks whether this sniff works as expected.

[anton-vlasenko]

I do not have high hopes that this pull request will be accepted. But I would be happy to receive any feedback. Thanks.

[jrfnl]

@anton-vlasenko While I appreciate what you are trying to do here, in my opinion this doesn't belong in WPCS.

Using ! function_exists() and ! class_exists() around all class/function declarations - as this sniff tries to enforce - has been discussed before and is an anti-pattern which should actively be _dis_couraged, not _en_couraged.

Re-defining functions and classes from WP Core, safe from the known list of pluggable functions, is also an anti-pattern and should be discouraged at all costs.

I understand that this is a problem in the Gutenberg repo as the code from that repo gets merged into Core, but adding a sniff which is only relevant to one specific plugin to the WPCS repo, especially a sniff which encourages the opposite of best practices, is something I cannot get behind.

Maybe Gutenberg should have a separate repo for Gutenberg specific sniffs ? (which would include WPCS and add this sniff)

Also: doesn't the Gutenberg repo have integration tests which run against all supported WP versions ? That setup should catch all these issues anyway and if it doesn't, that test set up needs improving.

Note: I have not reviewed the code in the sniff. Just at first glance, that would need quite some work too.

[anton-vlasenko]

Thank you for providing feedback on this PR, @jrfnl.
I appreciate your concerns regarding adding this sniff to the WPCS repo and your opinions on using ! function_exists() and ! class_exists().

I agree that the use of these statements may not be the best approach in all cases. However, in the context of the Gutenberg repo and its integration with Core, it is necessary to ensure that there are no conflicts with existing Core functions and classes.

Maybe Gutenberg should have a separate repo for Gutenberg specific sniffs ? (which would include WPCS and add this sniff)

Yes, I agree with that. It seems like a logical step.

In regards to the integration tests that run against all supported WP versions in the Gutenberg repo, there are Jest tests that test certain frontend functionality. However, some of these tests are unreliable, and it is not always possible to determine if they fail for legitimate reasons or if it is just an issue with the test itself.

Note: I have not reviewed the code in the sniff. Just at first glance, that would need quite some work too.

In my defense, I can say that this is my first PHPCS sniff. 🤣 I would appreciate any feedback.
However, I think this pull request could be closed since it makes sense to have a separate repository for the Gutenberg coding standards.

[jrfnl]

Thank you for providing feedback on this PR, @jrfnl. I appreciate your concerns regarding adding this sniff to the WPCS repo and your opinions on using ! function_exists() and ! class_exists().

I agree that the use of these statements may not be the best approach in all cases. However, in the context of the Gutenberg repo and its integration with Core, it is necessary to ensure that there are no conflicts with existing Core functions and classes.

Not sure if @GaryJones contacted you about this, but he made a suggestion about using namespaces (or a namespace) in the GB project, which would make the GB native versions namespaced, while the versions of functions/classes in WP Core would be in the global namespace.
Sounds to me something which could well be a solution for your problem and could make moving code from GB to Core simpler too, though I imagine this will need extensive discussion (in a ticket in the GB repo, not here).

In regards to the integration tests that run against all supported WP versions in the Gutenberg repo, there are Jest tests that test certain frontend functionality. However, some of these tests are unreliable, and it is not always possible to determine if they fail for legitimate reasons or if it is just an issue with the test itself.

I can't say much about the tests without looking at them in detail, but that sounds like something which can use expanding and improvement as well if that setup doesn't catch these issues. Another ticket to be opened in the GB repo ?

In my defense, I can say that this is my first PHPCS sniff. 🤣 I would appreciate any feedback. However, I think this pull request could be closed since it makes sense to have a separate repository for the Gutenberg coding standards.

I fully appreciate that and it's a good first effort. Having said that, for something which we will not accept in this repo, spending significant time to review the sniff and coach you on it, does not seem like the most sensible way for me to spend my time.

[anton-vlasenko]

Not sure if @GaryJones contacted you about this, but he made a suggestion about using namespaces (or a namespace) in the GB project, which would make the GB native versions namespaced, while the versions of functions/classes in WP Core would be in the global namespace.

No, @GaryJones hasn't contacted me. I don't know if using namespaces would work in all cases, but it sounds interesting.
I'm curious to see where that discussion would go if such a GB ticket existed. I haven't been able to find any GB tickets proposing the use of namespaces.

I can't say much about the tests without looking at them in detail, but that sounds like something which can use expanding and improvement as well if that setup doesn't catch these issues. Another ticket to be opened in the GB repo ?

As far as I know, there is ongoing work to improve JavaScript frontend tests in order to eliminate the problem of unreliability (a.k.a. "flaky" tests), but there is currently no complete solution to this issue.

I fully appreciate that and it's a good first effort. Having said that, for something which we will not accept in this repo, spending significant time to review the sniff and coach you on it, does not seem like the most sensible way for me to spend my time.

Sorry, my wording was not exactly precise. I didn't mean to ask you for code review of this particular PR.
I appreciate your time and your replies. Thanks!