-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Forbid using certain functions and classes #57738
Forbid using certain functions and classes #57738
Conversation
Flaky tests detected in 0c8ef69. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7717914499
|
The PR fails CI checks because certain functions are not permitted within the block-library packages. Clarification is needed on whether to ignore or refactor these functions. The list of problematic functions/classes:
|
I'd appreciate your feedback on that, @gziolo. |
The sniff works great detecting all listed violations 👍 Some of these code paths are behind the check ensuring they only execute in the Gutenberg context so folks can explicitly disable violations for these lines. It looks like other calls will fail in WordPress core like these unconditionally calling Overall, I find this new rule very helpful as it will remind folks to default to syntax available in WordPress core. |
f032c82
to
0277771
Compare
@gziolo I really appreciate your feedback. An example of such statement would be: // Refactoring required: this function must be guarded as it is available only in Gutenberg, not in Core. I plan to create another GitHub issue specifically to address the refactoring of these occurrences after this PR is merged. If the PR looks good to you, both in terms of code and functionality, may I ask for your approval? 🙏 |
wp_register_script_module( | ||
'@wordpress/block-library/file', | ||
defined( 'IS_GUTENBERG_PLUGIN' ) && IS_GUTENBERG_PLUGIN ? gutenberg_url( '/build/interactivity/file.min.js' ) : includes_url( 'blocks/file/view.min.js' ), | ||
$module_url ?? includes_url( 'blocks/file/view.min.js' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this should work in PHP 7.0+. Just unsure if Gutenberg still has to support 5.6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7 or higher according to the plugin directory. There's a whole discussion around whether WP "officially" supports the null coalescing operator or not but I think at this point we should be fine to start using it 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can always initialize the $module_url
before IS_GUTENBERG_PLUGIN
check to avoid the conditional here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7 or higher according to the plugin directory. There's a whole discussion around whether WP "officially" supports the null coalescing operator or not but I think at this point we should be fine to start using it 😅
The null coalescing operator has not yet been accepted into the WordPress Coding Standards. There is a Trac ticket open for it.
Until this ticket or a proposal for it (and other PHP 7.0 syntax) is accepted into Core, please don't use it (yet) for code that will be merged into Core. Why? It will be get removed once it comes to Core.
So to ease the sync process between GB and Core, I'd advise to not use it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the null coalescing operator was OK to use in Gutenberg considering its extensive usage throughout the Gutenberg codebase: https://github.com/search?q=repo%3AWordPress%2Fgutenberg+path%3A*.php+language%3APHP+%3F%3F&type=code&ref=advsearch.
But I didn't check if it's actually allowed.
I agree with @hellofromtonya, let's not use ??
until it's officially accepted in Core (and appreciate the link to the Trac discussion).
Fixed in 1b61304a422ff9d8bd1f1bbdf30215f3b1ff75ce.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can always initialize the
$module_url
beforeIS_GUTENBERG_PLUGIN
check to avoid the conditional here.
I didn't want to initialize it before the IS_GUTENBERG_PLUGIN
check for performance-related reasons. But yes, that was an option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, this is looking pretty good! I think we should try and merge this, see how it works and then iterate if needed. Also it looks like most of the refactoring to be done is in experimental blocks that aren't going into 6.5 so our codebase seems to be in a good place overall 😄
$conditions = array_reverse( $tokens[ $stack_pointer ]['conditions'], true ); | ||
|
||
// Matches the "if ( defined( 'IS_GUTENBERG_PLUGIN' ) && IS_GUTENBERG_PLUGIN" string. | ||
$regexp = '/if\s*\(\s*defined\(\s*(\'|")IS_GUTENBERG_PLUGIN(\'|")\s*\)\s*&&\s*IS_GUTENBERG_PLUGIN/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if there would ever be a reason to make this check configurable? Checking for IS_GUTENBERG_PLUGIN
makes sense in block_library or any other PHP files that may be auto-generated in core from the npm packages. I guess this kind of logic wouldn't be needed at all if we were using the sniff in plugin-specific PHP files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sniff is currently only checking the /packages/block-library
folder: https://github.com/WordPress/gutenberg/pull/57738/files#diff-05ae9cddcaec1e845771a7db224961439f83ef5939ec67d3a48744cb34d7e58bR157
So, it doesn't check files in other folders.
I hope I've understood your question correctly.
@@ -980,6 +980,8 @@ function block_core_navigation_get_fallback_blocks() { | |||
if ( class_exists( 'WP_Navigation_Fallback' ) ) { | |||
$navigation_post = WP_Navigation_Fallback::get_fallback(); | |||
} else { | |||
// Refactoring required: this class must be guarded as it is available only in Gutenberg, and not in Core. | |||
// phpcs:ignore Gutenberg.CodeAnalysis.ForbiddenFunctionsAndClasses.ForbiddenClassUsage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only file that has the phpcs:ignore
comment included that is going to sync to WP core. Will it work correctly without the rule defined? I simply don't know how PHPCS works so the answer will be simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will not generate any errors if the ignored rule is not defined.
However, I've moved these to the phpcs.xml.dist
file in 255e3ed to avoid cluttering the code with phpcs:ignore
statements. I hope that these files will be refactored soon, allowing for the removal of the exclude-pattern
statements.
phpcs.xml.dist
Outdated
<element value="(G|g)utenberg.*"/> | ||
</property> | ||
<property name="forbidden_classes" type="array"> | ||
<element value="(G|g)utenberg.*"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: technically speaking, it should be enough to use the following:
<element value="(G|g)utenberg.*"/> | |
</property> | |
<property name="forbidden_classes" type="array"> | |
<element value="(G|g)utenberg.*"/> | |
<element value="gutenberg.*"/> | |
</property> | |
<property name="forbidden_classes" type="array"> | |
<element value="Gutenberg.*"/> |
Although, maybe I'm missing some special cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right.
However, to be on the safer side, I would prefer to allow the first letter to be either 'G' or 'g'.
it's just my personal preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great start. I left a couple of nitpicks to consider, but none of them are blockers. This should be a big help for the Gutenberg syncing in WordPress Core, as so far it's been always on the editor release leads to catch these code paths.
@youknowriad and @getdave, anything we could miss based on the experience in the WP 6.5 release so far?
Great work here 👍 This is going to help us a lot. |
// Refactoring required: this class must be guarded as it is available only in Gutenberg, and not in Core. | ||
// phpcs:ignore Gutenberg.CodeAnalysis.ForbiddenFunctionsAndClasses.ForbiddenClassUsage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tjcafferkey @ockham Did we introduce the called to the Gutenberg_
prefixed version of this class in the recent work on hooks in the Nav block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm removing this in #58369 I don't see where this class is defined at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. This will make a big difference. Merge and iterate 👍
Would it help if someone (me?) made an Issue to track resolving the outstanding cases you've had to ignore
here? Would be good to get these resolved.
d185d8f
to
1b61304
Compare
…f functions/classes.
1b61304
to
8249699
Compare
…void cluttering the code with `phpcs:ignore` statements.
@getdave Awesome. Yes, please go ahead and create a GitHub issue to track resolving the outstanding cases. |
Thank you all! |
What?
This PR introduces the
ForbiddenFunctionsAndClasses
sniff.It's designed to enforce usage restrictions on specific classes and functions within the Gutenberg project.
While declarations of these classes/functions are permitted, their actual usage is restricted based on defined regular expressions.
Fixes #54745.
Why?
The necessity of this PR arises from the need to avoid using any Gutenberg functions and classes in the block-library packages.
This is because these packages are backported to Core and must not include Gutenberg related functions and classes.
How?
The implementation involves defining regular expressions for restricted classes and functions.
The
ForbiddenFunctionsAndClassesSniff
class hooks into the PHP CodeSniffer process, scanning the code for instances where these restricted classes or functions are used.Testing Instructions
test/php/gutenberg-coding-standards/
.composer update
.composer run check-all
.Testing Instructions for Keyboard
Screenshots or screencast