Navigation: Move the renderer class to the main navigation file #57979
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 and
tellthemachines
as code owners
|
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 |
|
This sounds like a good plan. There's little value in it existing elsewhere. |
youknowriad
reviewed
Jan 18, 2024
| /** | ||
| * Helper functions used to render the navigation block. | ||
| */ | ||
| class WP_Navigation_Block_Renderer { |
There was a problem hiding this comment.
Is it possible to avoid a class and just have functions? The reason is more pragmatic than anything. We have code in place that auto rename functions in the "block-library" php files when building the Gutenberg plugin to avoid conflicts with core when the same files get included in Core.
I believe (to confirm) we don't have the same behavior for classes. So either we remove the class instead of regular functions or we update the webpack config that rename functions to also prefix classnames.
There was a problem hiding this comment.
The reason for using a class is that we can keep keep the public functions to a minimum, which helps to avoid creating a load of functions we have to maintain for BC.
There was a problem hiding this comment.
I'm not opinionated here personally, just stating that as it's stand it's going to create issues when this file lands in Core.
There was a problem hiding this comment.
I don't feel strongly either. There are already many public functions so I'm not sure making these public loads worse!
There was a problem hiding this comment.
Have we investigated how hard it would be to add the auto-renaming functionality? As Riad says we're unopinionated about the code so we should probably aim to do what's right for the code and make the release process conform to that.
That said, if we stick with class then any help looking into auto-renaming would be great @scruffian 🙏 😄
There was a problem hiding this comment.
Good idea, I'll look into it.
|
So I've just attempted a package update on Core and the following is one of the first errors I got. Let's make sure to land this PR before Wednesday. |
3 tasks
|
@scruffian I think we need to include this fix as part of any changes here. |
|
I added a commit which moves the class into its own file, and then updated the script to:
|
|
Size Change: 0 B Total Size: 1.69 MB ℹ️ View Unchanged
|
draganescu
reviewed
Jan 23, 2024
tools/webpack/blocks.js
Outdated
| @@ -35,6 +35,8 @@ const prefixFunctions = [ | |||
| 'wp_get_global_settings', | |||
| ]; | |||
|
|
|||
| const suffixClasses = [ 'WP_Navigation_Block_Renderer' ]; | |||
There was a problem hiding this comment.
oh another list to maintain?
There was a problem hiding this comment.
Why do we need to maintain a list of classes. Can't we auto-discover them using a regex like we do for functions?
There was a problem hiding this comment.
I don't consider this a blocker for this PR but if we can avoid the list, it would be an improvement.
There was a problem hiding this comment.
Agreed. Ship this and then follow up to make this dynamic (unless we can easily do the dynamic bit here).
|
The copy script in WordPress copies |
youknowriad
reviewed
Jan 23, 2024
|
Ok, so there's something wrong in this PR (subtle). When built, the class moves to There's a subtle issue here, if say two different blocks define a |
|
Flaky tests detected in 9938394. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7641244027
|
youknowriad
force-pushed
the
move/navigation-renderer
branch
from
c88f8be
to
3528cca
Compare
…ke advantage of the automatic backporting
youknowriad
force-pushed
the
move/navigation-renderer
branch
from
3528cca
to
d759d0f
Compare
youknowriad
approved these changes
Jan 24, 2024
|
Thanks for the fix @youknowriad this looks good to me as well now. |
6 tasks
|
Great to see this in 👏 |
This was referenced Jan 25, 2024
Merged
|
✅ I updated the PHP Sync Tracking Issue for WP 6.5 to note this PR does not require a backport for WP 6.5. |
scruffian
added
the
Backport to WP Minor Release
scruffian
added a commit
that referenced
this pull request
Jan 30, 2024
* 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
added a commit
that referenced
this pull request
Jan 30, 2024
…n't clash with the one backported to core (#58429) * Navigation: Move the renderer class to the main navigation file (#57979) * 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]> * Use the right renderer class * Fix php unit tests (after variations api change) (#58090) --------- Co-authored-by: Riad Benguella <[email protected]> Co-authored-by: Ella <[email protected]>
youknowriad
removed
the
Backport to WP Minor Release
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?
This moves the
WP_Navigation_Block_Rendererback into the rendering file for the Navigation blockWhy?
Based on the feedback in https://core.trac.wordpress.org/ticket/59867#comment:4, I think it might be a good idea to move the
WP_Navigation_Block_Rendererclass back into the same file as the rest of the navigation block rendering logic, so that it gets backported automatically and code isn't spread across different packages.How?
Just move the class. I had to remove the
gutenbergprefix from some function calls.Testing Instructions
Check that the navigation block still renders on the frontend