Skip to content
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

Merged
merged 16 commits into from
Nov 5, 2021
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 59 additions & 2 deletions lib/navigation.php
Original file line number Diff line number Diff line change
Expand Up @@ -418,13 +418,13 @@ function gutenberg_register_navigation_post_type() {
'description' => __( 'Navigation menus.', 'gutenberg' ),
'public' => false,
'has_archive' => false,
'show_ui' => false,
'show_ui' => true,
'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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this?

Copy link
Contributor Author

@anton-vlasenko anton-vlasenko Nov 4, 2021

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

'supports' => array(
'title',
'editor',
Expand All @@ -435,3 +435,60 @@ function gutenberg_register_navigation_post_type() {
register_post_type( 'wp_navigation', $args );
}
add_action( 'init', 'gutenberg_register_navigation_post_type' );

/**
* Disable block editor for wp_navigation type posts so they can be managed via the UI.
*
* @param bool $value Whether the CPT supports block editor or not.
* @param string $post_type Post type.
*
* @return bool
*/
function gutenberg_disable_block_editor_for_navigation_post_type( $value, $post_type ) {
if ( 'wp_navigation' === $post_type ) {
return false;
}

return $value;
}

add_filter( 'use_block_editor_for_post_type', 'gutenberg_disable_block_editor_for_navigation_post_type', 10, 2 );

/**
* This callback disables content editor for wp_navigation type posts.
* Content editor cannot handle wp_navigation type posts correctly.
*
* @param WP_Post $post An instance of WP_Post class.
*/
function gutenberg_disable_content_editor_for_navigation_post_type( $post ) {
$post_type = get_post_type( $post );
if ( 'wp_navigation' !== $post_type ) {
return;
}

remove_post_type_support( $post_type, 'editor' );
Copy link
Contributor

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.

Copy link
Contributor Author

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

}

add_action( 'edit_form_after_title', 'gutenberg_disable_content_editor_for_navigation_post_type', 10, 1 );

/**
* Fixes the label of the 'wp_navigation' admin menu entry.
Copy link
Contributor

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.

Copy link
Contributor Author

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

*/
function gutenberg_fix_navigation_items_admin_menu_entry() {
global $submenu;
if ( ! isset( $submenu['themes.php'] ) ) {
return;
}
$post_type = get_post_type_object( 'wp_navigation' );
if ( ! $post_type ) {
return;
}
foreach ( $submenu['themes.php'] as $key => $submenu_entry ) {
if ( $post_type->labels->all_items === $submenu['themes.php'][ $key ][0] ) {
Copy link
Contributor

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.

Copy link
Contributor Author

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

$submenu['themes.php'][ $key ][0] = $post_type->labels->menu_name; // phpcs:ignore WordPress.WP.GlobalVariablesOverride
return;
}
}
}

add_action( 'admin_menu', 'gutenberg_fix_navigation_items_admin_menu_entry' );
62 changes: 62 additions & 0 deletions phpunit/navigation-test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
<?php
/**
* This class is supposed to test the functionality of the navigation.php
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 38dbd8e

*
* @package Gutenberg
*/

class WP_Navigation_Test extends WP_UnitTestCase {
const NAVIGATION_POST_TYPE = 'wp_navigation';
const NON_NAVIGATION_POST_TYPE = 'wp_non_navigation';

public function tearDown() {
add_post_type_support( static::NAVIGATION_POST_TYPE, 'editor' );
}

public function test_it_doesnt_disable_block_editor_for_non_navigation_post_types() {
$filtered_result = gutenberg_disable_block_editor_for_navigation_post_type( true, static::NON_NAVIGATION_POST_TYPE );
$this->assertTrue( $filtered_result );
}

public function test_it_disables_block_editor_for_navigation_post_types() {
$filtered_result = gutenberg_disable_block_editor_for_navigation_post_type( true, static::NAVIGATION_POST_TYPE );
$this->assertFalse( $filtered_result );
}

public function test_it_doesnt_disable_content_editor_for_non_navigation_type_posts() {
$post = $this->create_non_navigation_post();
$this->assertTrue( $this->supports_block_editor() );

gutenberg_disable_content_editor_for_navigation_post_type( $post );

$this->assertTrue( $this->supports_block_editor() );
}

public function test_it_disables_content_editor_for_navigation_type_posts() {
$post = $this->create_navigation_post();
$this->assertTrue( $this->supports_block_editor() );

gutenberg_disable_content_editor_for_navigation_post_type( $post );

$this->assertFalse( $this->supports_block_editor() );
}

private function create_post( $type ) {
$post = new WP_Post( new StdClass() );
$post->post_type = $type;
$post->filter = 'raw';
return $post;
}

private function create_non_navigation_post() {
return $this->create_post( static::NON_NAVIGATION_POST_TYPE );
}

private function create_navigation_post() {
return $this->create_post( static::NAVIGATION_POST_TYPE );
}

private function supports_block_editor() {
return post_type_supports( static::NAVIGATION_POST_TYPE, 'editor' );
}
}