Navigation menu items should be defined as being hierarchical

[schlessera]

Menu items are hierarchical in nature, as each menu item can be attached as a child to a parent item. This is what allows one to build submenus at differing levels.

These hierarchical relationships are persisted into the database using the post_parent column to attach parent IDs to child IDs.

However, when the nav_menu_item is registered during the bootstrapping process, it is defined as being hierarchical => false, which is conceptually wrong. It just happens to be irrelevant, because the user interface for menus is a custom implementation that considerably differs from standard post list tables.

As these nav_menu_item elements might need to be iterated over through other means than the menu UI, they should be correctly represented as being hierarchical in nature (and thus making use of the post_parent database table column).

[welcher]

Patch is pretty straight forward. Let's add some tests.

[schlessera]

@welcher I'm not sure what specific tests you have in mind.

What could be tested is: "a nav_menu_item's hierarchical property should return true", but that would just test whether the register_post_type() correctly assigns that property (which is already tested in several other places, directly or indirectly).

Any specific test you would like to see?

[schlessera]

Patch still applies cleanly.

@welcher Any update on what tests you'd expect to see? The ticket is blocked in needs-unit-tests workflow currently.

[welcher]

Replying to schlessera:

@welcher Any update on what tests you'd expect to see? The ticket is blocked in needs-unit-tests workflow currently.

Sorry for being the blocker on this! I think you are correct that there are no tests for this that aren't covered in other areas. As long as the currents tests pass, I'd be happy.

[netweb]

The hierarchical parameter of register_post_type() is not clear cut as the verbiage might suggest:

Via ​https://codex.wordpress.org/Function_Reference/register_post_type

"hierarchical Note: this parameter was intended for Pages. Be careful when choosing it for your custom post type - if you are planning to have very many entries (say - over 2-3 thousand), you will run into load time issues. With this parameter set to true WordPress will fetch all IDs of that particular post type on each administration page load for your post type. Servers with limited memory resources may also be challenged by this parameter being set to true."

Punting as testing for this should occur whilst not in beta, I also believe there other concerns relating to the use of hierarchical post types that needs to be researched here

[schlessera]

@netweb For the nav_menu_item post type, there are no post type administration screens. The menu system uses its own custom user interface, which already makes use of the post_parent column in the database.

As far as I can tell, there's no screen actually checking for the hierarchical parameter for that post type right now. However, the REST API would need it to be correctly set, so that it knows to look for the post_parent column as well.

[peterwilsoncc]

Switching milestone due to the focus on the new editor (Gutenberg) for WordPress 5.0.

[pento]

wp_get_nav_menu_items() calls get_posts(), which uses WP_Query. WP_Query::get_posts() changes its behaviour depending upon whether a post type is hierarchical or not.

I'd like to see some more testing to show if this has an effect on how the query runs.