-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Provide a way to manage wp_navigation posts from wp-admin #36126
Provide a way to manage wp_navigation posts from wp-admin #36126
Conversation
I think we want to hide any ability to edit the content of the navigation CPT, as manually changing navigation block markup is prone to breaking it. Also, I like the simplicity of this approach if it works, no need for new API just a Gutenberg specific fix. |
Thank you for the review. |
This looks great to me, thanks @anton-vlasenko! The only remaining bit that would be great to tidy up is the bit of text here: It'd be cool if it just said 'Navigation Menus'. I think this would just be a simple change to the strings defined in the custom post type definition: Line 402 in 40ff1e5
The only part that confused me is that the Template Parts section has 'All Template Parts' defined for this
So perhaps there's a different way to control this text that doesn't involve changing |
…t can be managed using the standard UI.
…tor doesn't correctly work with wp_navigation posts.
b3438b3
to
566c159
Compare
Oh, if there's a way to get rid of the 'Add new' button as well, that'd be great. It looks like that also opens the editor. |
Great catch. I've noticed it too. |
Great work @anton-vlasenko 👏🏻 @talldan this is what we get with this approach: Aside from renaming, not much use for everything else there:
Unless updating those changes the behavior of the block, I think this exploration will not lead us to far. It feels to me like the whole idea of the #36036 is bound to be around using a block editor to properly edit a menu, while this approach using the "post edit screen" brings only one benefit and a lot of useless baggage. |
Content editor cannot correctly edit them. 2. Add unit tests.
Rename it to "Navigation Menus".
@draganescu @talldan |
I see value in this PR (considering the changes I've just made). |
Yes, there is a way. Fixed in 39e9570 |
I think bulk deleting is valuable right now, given we know the flow for creating menus isn't perfect. And it'll be a good idea to iterate on providing a way to edit these menus via the Gutenberg plugin after 5.9. The direction should ultimately be guided by #29630 and future related issues to that. Right now it looks like that's moving towards using wp-admin for listing menus for 5.9. |
'show_in_menu' => 'themes.php', | ||
'show_in_admin_bar' => false, | ||
'show_in_rest' => true, | ||
'map_meta_cap' => true, | ||
'rest_base' => 'navigation', | ||
'rest_controller_class' => 'WP_REST_Posts_Controller', | ||
'rest_controller_class' => WP_REST_Posts_Controller::class, |
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.
Why change this?
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.
IDEs like PHPStorm can understand what class is meant here if we use the ::class
keyword. This could be useful during refactoring or if we decide to use namespaces in the far future :). And it's just convenient to click on a class name to go to its definition.
I'm not aware of any modern PHP application that uses plain strings to specify class names. It just looks a little bit archaic to me.
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 think this is good but, just thinking, ::class
returns the fully qualified class name resolution, it is not "the same" as the class name as a string, and if rest_controller_class does not expect a fully qualified class name we may get into trouble.
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.
Yes, it does return a FQCN, but since we don't use PHP namespaces in WordPress it equals to 'WP_REST_Posts_Controller'
. I've checked it.
And it's fail safe. Even If the WP_REST_Posts_Controller
class is not loaded or defined, it will still return 'WP_REST_Posts_Controller'
.
I'm not insisting that we have to use ::class
, I just see no reasons not to use 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.
What happens if we do introduce Namespaces? Well ... I assume that too many things will change for this to be a big problem?
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.
If we do introduce namespaces, the FQCN will change to include the namespace.
E.g. instead of 'WP_REST_Posts_Controller'
it will return 'WordPress\REST\Controllers\Posts'
or something similar.
But if we introduce namespaces, we will need to know FQCN to load the correct class anyway because there could be several classes with the same name, i.e., multiple Posts
classes in different namespaces.
This change shouldn't introduce any problems.
We need to enable it back because we disable it to hide the content editor windows. This is required to avoid the incorrect state of wp_navigation CPT.
return; | ||
} | ||
|
||
remove_post_type_support( $post_type, 'editor' ); |
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.
So this has to hapen b/c if the post type definition itself does not support editor
we get into the rest problem not allowing saving at all. That would be good to be added to the docblock of the function.
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 catch.
Fixed in 3bee078
lib/navigation.php
Outdated
add_action( 'edit_form_after_editor', 'gutenberg_enable_content_editor_for_navigation_post_type', 10, 1 ); | ||
|
||
/** | ||
* Fixes the label of the 'wp_navigation' admin menu entry. |
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's unclear what exactly this fixes from the comment.
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.
Thank you for the review!
I've renamed the method and updated the comment.
Fixed in baccf37
lib/navigation.php
Outdated
return; | ||
} | ||
foreach ( $submenu['themes.php'] as $key => $submenu_entry ) { | ||
if ( $post_type->labels->all_items === $submenu['themes.php'][ $key ][0] ) { |
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 think assigning $submenu['themes.php'][ $key ][0]
to a variable that tells the reader what is at index 0
would be nice.
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've renamed the variables.
Please let me know if you think it's fine.
a6abfca
phpunit/navigation-test.php
Outdated
@@ -0,0 +1,94 @@ | |||
<?php | |||
/** | |||
* This class is supposed to test the functionality of the navigation.php |
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.
is supposed to test
-> tests
,
of the navigation.php
-> of block navigation
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.
Fixed in 38dbd8e
The new name better describes the purpose of the function.
… callback function.
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 stuff, LGTM.
Thanks for this, appreciate you finding all those niché core APIs for disabling features.
Thank you! |
Description
The goal of this PR is to disable block editor for wp_navigation type posts (because it doesn't work with wp_navigation posts) and provide a way to mange wp_navigation posts from wp-admin.
Fixes #36036 (?).
How has this been tested?
Appearance -> Navigation Menus
page.Screenshots
Types of changes
New feature
Checklist:
*.native.js
files for terms that need renaming or removal).