Navigation: Update the navigaiton renderer class name so that it doesn't clash with the one backported to core #58429
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
scruffian
requested review from
spacedmonkey,
talldan,
gziolo,
ntwb,
nerrad,
ajitbohra,
ryanwelcher and
tellthemachines
as code owners
scruffian
added
[Type] Bug
youknowriad
reviewed
Jan 30, 2024
| /** | ||
| * Helper functions used to render the navigation block. | ||
| */ | ||
| class WP_Navigation_Block_Renderer_Gutenberg { |
There was a problem hiding this comment.
Should we keep the name as is and include the webpack change instead?
There was a problem hiding this comment.
I thought this was simpler
There was a problem hiding this comment.
As soon as we backport this file to WP core, then it will throw a fatal error because of the class redeclaration. That's why I think that following the recommendation from Riad makes perfect sense.
There was a problem hiding this comment.
Ok I'll do that :)
There was a problem hiding this comment.
This file won't be backported to Core though
There was a problem hiding this comment.
It isn't targeting trunk, interesting :)
|
I'm trying to reproduce the bug first, and I'm failing so far. I destroyed my wp-env and recreated it to get the latest WP trunk and used release/17.5 branch here but can't see the error. What am I missing, how to trigger the error. |
* Navigation: Move the renderer class to the main navigation file to take advantage of the automatic backporting * Update the build script to also copy over class files * prefix with gutenberg * Add a Gutenberg suffix to class files when they are built * add gutenberg prefix to functions * move the built block files to their own dir * Putting back the render class into the same file as the navigation block * Update the rendered post rebase * Fix php unit tests --------- Co-authored-by: Riad Benguella <[email protected]>
scruffian
force-pushed
the
fix/navigaiton-renderer-name-collision
branch
from
633abbf
to
1ab1d76
Compare
|
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/load.php ❔ phpunit/blocks/render-block-navigation-test.php ❔ phpunit/class-wp-navigation-block-renderer-test.php |
|
Oh it looks like the "early return" we had in place only works in some php versions but not all So in fact, we could have solved this by just moving the class code within the "if" condition and inverting the condition. But this fix works too. |
|
Size Change: 0 B Total Size: 1.69 MB ℹ️ View Unchanged
|
youknowriad
approved these changes
Jan 30, 2024
scruffian
force-pushed
the
fix/navigaiton-renderer-name-collision
branch
from
d04b2a4
to
2ca2c44
Compare
So this is considered an anti-pattern then? Can anyone provide a docs reference which shows which version of PHP started to support this pattern? |
scruffian
removed
the
Backport to Gutenberg Minor Release
|
Is the problem really a change in how <?php
var_dump( class_exists( 'Foo' ) );
echo "before return\n";
return;
echo "after return\n";
class Foo {}This outputs: Confirming that the function is already defined even though the line Edit: running PHP 8.3.2 |
|
@mcsf, I've tried this with PHP 7.3 and 8.0 and got the same result. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Cherry picks #57979.
Also cherry picks #58090 to fix the tests
Why?
The previous version of Gutenberg had this classs in the compat folder, so when it was backported to core and run alongside the 17.5 version of Gutenberg the class was declared twice resulting in a fatal error.
Testing Instructions