WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 9 months ago

#39077 new defect (bug)

Navigation menu items should be defined as being hierarchical

Reported by: schlessera Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Menus Keywords: has-patch needs-testing dev-feedback
Focuses: Cc:
PR Number:

Description

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).

Attachments (1)

39077.1.diff (471 bytes) - added by schlessera 3 years ago.

Download all attachments as: .zip

Change History (14)

@schlessera
3 years ago

#1 @helen
3 years ago

  • Version changed from trunk to 3.0

#2 @welcher
3 years ago

  • Keywords has-patch needs-unit-tests added

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

#3 @schlessera
2 years ago

@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?

#4 follow-up: @schlessera
2 years ago

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.

#5 in reply to: ↑ 4 @welcher
2 years ago

  • Keywords needs-unit-tests removed

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.

#6 @welcher
2 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 4.9

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


2 years ago

#8 @netweb
2 years ago

  • Keywords needs-testing added; commit removed
  • Milestone changed from 4.9 to Future Release

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

#9 @netweb
2 years ago

  • Keywords dev-feedback added

#10 @SergeyBiryukov
2 years ago

  • Milestone changed from Future Release to 5.0

#11 @schlessera
2 years ago

@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.

#12 @peterwilsoncc
12 months ago

  • Milestone changed from 5.0 to 5.1

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

#13 @pento
9 months ago

  • Milestone changed from 5.1 to Future Release

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.

Note: See TracTickets for help on using tickets.