Avoid using early returns to prevent classname duplication #58500
Conversation
youknowriad
added
the
[Type] Code Quality
youknowriad
requested review from
mcsf,
getdave,
dmsnell,
hellofromtonya and
cbravobernal
youknowriad
requested review from
TimothyBJacobs,
spacedmonkey,
geriux and
anton-vlasenko
as code owners
| @@ -32,10 +32,6 @@ | |||
| * @since 6.3.0 | |||
| */ | |||
|
|
|||
| if ( class_exists( 'WP_Duotone_Gutenberg' ) ) { | |||
There was a problem hiding this comment.
These have Gutenberg prefixes so there's not need to guard. They are not meant to be included with this name in core.
|
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/README.md ❔ lib/class-wp-duotone-gutenberg.php ❔ lib/class-wp-rest-global-styles-controller-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.4/fonts/font-face/class-wp-font-face-resolver.php ❔ lib/compat/wordpress-6.4/fonts/font-face/class-wp-font-face.php ❔ lib/compat/wordpress-6.4/html-api/class-wp-html-active-formatting-elements.php ❔ lib/compat/wordpress-6.4/html-api/class-wp-html-open-elements.php ❔ lib/compat/wordpress-6.4/html-api/class-wp-html-processor-state.php ❔ lib/compat/wordpress-6.4/html-api/class-wp-html-processor.php ❔ lib/compat/wordpress-6.4/html-api/class-wp-html-token.php ❔ lib/compat/wordpress-6.4/html-api/class-wp-html-unsupported-exception.php ❔ lib/compat/wordpress-6.5/block-bindings/class-wp-block-bindings-registry.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/fonts/font-face/bc-layer/class-wp-fonts-provider-local.php ❔ lib/experimental/fonts/font-face/bc-layer/class-wp-fonts-provider.php ❔ lib/experimental/fonts/font-face/bc-layer/class-wp-fonts-resolver.php ❔ lib/experimental/fonts/font-face/bc-layer/class-wp-fonts-utils.php ❔ lib/experimental/fonts/font-face/bc-layer/class-wp-fonts.php ❔ lib/experimental/fonts/font-face/bc-layer/class-wp-web-fonts.php ❔ lib/experimental/fonts/font-face/bc-layer/class-wp-webfonts-provider-local.php ❔ lib/experimental/fonts/font-face/bc-layer/class-wp-webfonts-provider.php ❔ lib/experimental/fonts/font-face/bc-layer/class-wp-webfonts-utils.php ❔ lib/experimental/fonts/font-face/bc-layer/class-wp-webfonts.php ❔ lib/experimental/fonts/font-library/class-wp-font-collection.php ❔ lib/experimental/fonts/font-library/class-wp-font-library.php ❔ lib/experimental/fonts/font-library/class-wp-font-utils.php ❔ lib/experimental/fonts/font-library/class-wp-rest-font-collections-controller.php ❔ lib/experimental/fonts/font-library/class-wp-rest-font-faces-controller.php ❔ lib/experimental/fonts/font-library/class-wp-rest-font-families-controller.php |
|
I'm working on this PR. |
anton-vlasenko
force-pushed
the
update/guard-classnames-properly
branch
from
cb325a9 to
bc2faec
Compare
|
Flaky tests detected in cb325a9dc1362d244f4fb54cbbfcac1544e22707. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7726613441
|
| $incorrect_guarded_error_message = sprintf( | ||
| 'The class "%s" is not properly guarded against redeclaration. Please ensure the entire class body is wrapped within an "if ( ! class_exists( \'%s\' )) {" statement.', | ||
| $className, | ||
| $className | ||
| ); | ||
|
|
||
| $phpcsFile->addError( $incorrect_guarded_error_message, $classToken, 'ClassIncorrectlyGuardedAgainstRedeclaration' ); |
|
I think the phpcs array alignment rule is probably buggy. It forced me to do this commit cdba242 to make it pass but I believe that commit shouldn't be needed. |
|
LGTM as well |
cbravobernal
added
the
Backport to Gutenberg RC
|
Folks, an issue is open for discussing different approaches to resolving this #58467. The approach taken in this PR is Approach 1, but the discussion is leaning towards Approach 2 that moves the guard into the |
|
@hellofromtonya Thanks for chiming in. I want to add more context here. We already had an issue with one of the package update PRs in this release 6.5 and since there's a Gutenberg release happening today, we wanted this issue fixed for all the current usage included on that Gutenberg release. That way, when we make newer backports in the next days (like font library), we won't have conflicts with the latest Gutenberg release. Now, for the future, I don't mind which approach we take personally. |
Co-authored-by: Anton Vlasenko <vlasenko.anton@gmail.com>
|
Cherry picked for |
cbravobernal
removed
the
Backport to Gutenberg RC
What?
As we've seen recently class_exists + early returns is not enough to prevent against classes duplication properly. It actually depends on the php version and sometimes PHP scans the classes before running the code which results in php fatal errors.
This PR updates our code to avoid these.
Why?
We're merging more and more code into Core and this will help us prevent fatal errors.
How?
I just inverted the condition, I wanted to update our PHPCS rule to not consider the early return as a correct fix but failed to do so. I'll leave that for @anton-vlasenko either here or as a follow-up.
Testing Instructions
There's no functional change in this PR, also consider checking the diff with the "whitespace" option enabled.