WordPress.org

Make WordPress Core

Opened 12 months ago

Last modified 4 weeks ago

#39077 new defect (bug)

Navigation menu items should be defined as being hierarchical

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

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 12 months ago.

Download all attachments as: .zip

Change History (12)

@schlessera
12 months ago

#1 @helen
12 months ago

  • Version changed from trunk to 3.0

#2 @welcher
12 months ago

  • Keywords has-patch needs-unit-tests added

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

#3 @schlessera
3 months 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
4 weeks 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
4 weeks 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
4 weeks 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.


4 weeks ago

#8 @netweb
4 weeks 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
4 weeks ago

  • Keywords dev-feedback added

#10 @SergeyBiryukov
4 weeks ago

  • Milestone changed from Future Release to 5.0

#11 @schlessera
4 weeks 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.

Note: See TracTickets for help on using tickets.