Updated: Comment #2

Problem/Motivation

Menu link default machine names and route machine names do not match.

Proposed resolution

Option 1

Rename all menu link machine names to match their route machine names

Pros
No guesswork
Less changes to make
Cons
The route machine names aren't as intuitive

Option 2

Rename all route machine names with corresponding menu links to match the menu link machine names

Pros
No guesswork
Better naming

Cons
Not all route names have corresponding menu links, so there will be two different styles of route machine names

Option 3

Rename all route machine names to match the menu link machine names, including following the style of naming for routes with no corresponding menu link

Pros
Better naming
Consistent throughout all menu links and routes

Cons
Not all route names have corresponding menu links, so this requires some inventiveness on renaming those
All route names will be touched, increasing the size of the change

Option 4

Rename all of the menu links and routes to something "better"

Pros
Best naming!

Cons
Most work
Who decides what to name things? What standard/pattern do we set for contrib to follow?

Option 5

Do nothing, won't fix

Pros
We're done!

Cons
We have two different styles of machine names that do not match.

Remaining tasks

Pick an option.

Also to consider

The current route naming pattern is modulename.description, with only one dot in the name. Menu links do not have that distinction.

User interface changes

N/A

API changes

Changing the route machine names will affect calling code in Drupal.

Original report by @effulgentsia

Follow up to #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit. That issue introduced the concept of machine names for menu links, but used a different naming pattern for them than what is used by route names in HEAD. We should pick which naming pattern we like more, and then change either the link names or route names, so that the two match.

Files: 
CommentFileSizeAuthor
#54 Screen Shot 2014-03-24 at 14.13.48.png40.21 KBswentel
#54 Screen Shot 2014-03-24 at 14.08.55.png26.66 KBswentel
#50 2178725-50.patch48.85 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,628 pass(es).
[ View ]
#50 increment.txt2.25 KBpwolanin
#45 2178725-45.patch47.47 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,589 pass(es).
[ View ]
#45 increment.txt1.09 KBpwolanin
#43 2178725-43.patch47.17 KBpwolanin
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,418 pass(es), 7 fail(s), and 23 exception(s).
[ View ]

Comments

tim.plunkett’s picture

  $links['node.admin.structure.types'] = array(
    'link_title' => 'Content types',
    'parent' => 'system.admin.structure',
    'description' => 'Manage content types, including default status, front page promotion, comment settings, etc.',
    'route_name' => 'node.overview_types',
  );

When we last redid the route machine names, the decision was made that they had to be modulename.something_descriptive, and that there could only be one dot in the name.
I disagreed with that restriction, so I'd be more than fine updating the route machine names to match the menu link defaults.

But, not all routes have a corresponding menu link, so we'd have some grey area in renaming *all* of the routes.

One thing to consider if we do touch all the routes: Those files are roughly in order of when the route were converted, and make absolutely no sense. We should consider reordering them at the same time.

tim.plunkett’s picture

Issue summary:View changes

Updated with my take on the options we have.

dawehner’s picture

I really think that this kind of renaming tasks should be moved until we really have a big picture what we really want, which is probably after fixing the conversions. You know, my opinion is is basically: make it in a way, that it is not overcomplex due to our addition of rules, but also don't assume that developers are stupid.

effulgentsia’s picture

Is there an option to rename both to something decent? For example, for #1, something like node.type.list?

tim.plunkett’s picture

Issue summary:View changes

Sure

Gábor Hojtsy’s picture

Woah, and I thought this may be an easy issue to look at :D It is hard to decipher how/why the menu machine names are easier to understand vs. the route names? Some examples would be great for A/B comparisons.

Sutharsan’s picture

Current menu items with their routes:

Menu item Route
block.admin.structure block.admin_display
action.admin.actions action.admin
aggregator.admin_overview aggregator.admin_overview
aggregator aggregator.page_last
aggregator.sources aggregator.sources
ban.admin.config.people ban.admin_page
book.admin.outlines book.admin
book book.render
comment.admin.content comment.admin
config.admin.management config.sync
config_translation.admin.config-translation config_translation.mapper_list
contact.admin.categories contact.category_list
contact.site_page contact.site_page
custom_block.add_page custom_block.add_page
dblog.admin.reports.dblog dblog.overview
dblog.admin.reports.page-not-found dblog.page_not_found
dblog.admin.reports.access-denied dblog.access_denied
dblog.admin.reports.search dblog.search
entity.admin.structure.display_modes entity.display_mode
entity.admin.structure.display_modes.view entity.view_mode_list
entity.admin.structure.display_modes.form entity.form_mode_list
field_ui.admin.reports.fields field_ui.list
filter.tips filter.tips_all
filter.admin.formats filter.admin_overview
forum forum.index
forum.admin.overview forum.overview
help.main help.main
image.admin.media.image-styles image.style_list
language.admin.language.admin_overview language.admin_overview
language.admin.language.content_settings_page language.content_settings_page
locale.admin.config.regional.translate locale.translate_page
locale.admin.reports.translations locale.translate_status
menu.admin.overview menu.overview_page
node.admin.content node.content_overview
node.admin.structure.types node.overview_types
node.add node.add_page
path.admin.overview path.admin_overview
picture.admin.config.picturemapping picture.mapping_page
search search.view
search.admin.settings search.settings
shortcut.admin.config.user-interface.shortcut shortcut.set_admin
simpletest.admin.config.development.testing simpletest.test_form
statistics.admin.config.system.statistics statistics.settings
system.admin system.admin
system.admin.structure system.admin_structure
system.admin.appearance system.themes_page
system.admin.modules system.modules_list
system.admin.config system.admin_config
system.admin.config.media system.admin_config_media
system.admin.config.media.file-system system.file_system_settings
system.admin.config.media.image-toolkit system.image_toolkit_settings
system.admin.config.services system.admin_config_services
system.admin.config.services.rss-publishing system.rss_feeds_settings
system.admin.config.development system.admin_config_development
system.admin.config.development.maintenance system.site_maintenance_mode
system.admin.config.development.performance system.performance_settings
system.admin.config.development.logging system.logging_settings
system.admin.config.regional system.admin_config_regional
system.admin.config.regional.settings system.regional_settings
system.admin.config.regional.date-time system.date_format_list
system.admin.config.search system.admin_config_search
system.admin.config.system system.admin_config_system
system.admin.config.system.site-information system.site_information_settings
system.admin.config.system.cron system.cron_settings
system.admin.config.user-interface system.admin_config_ui
system.admin.config.workflow system.admin_config_workflow
system.admin.config.content system.admin_config_content
system.admin.reports system.admin_reports
system.admin.reports.status system.status
taxonomy.admin.structure.vocabulary taxonomy.vocabulary_list
tracker tracker.page
update.admin.reports.updates update.status
user user.page
user.logout user.logout
user.admin.people user.admin_account
user.admin.config.people user.admin_index
user.admin.config.people.accounts user.account_settings
views_ui.admin.structure.views views_ui.list
views_ui.admin.reports.views-plugins views_ui.reports_plugins

[edit] change of leading text.

Gábor Hojtsy’s picture

Well, it is hard to say based on these which are categorically better :) Some menu items are better, some routes are better :D

webchick’s picture

I am also missing a cluebat I guess on why the menu link names are preferable. They are hard-coding the default path into the machine name, a path which by design is intended to be de-coupled from the route. Additionally, there are going to be more route names than menu link names, so using the menu link name again doesn't make sense to me.

We spent time making all of the route names consistent, why can we not just use them in two places?

Sutharsan’s picture

StatusFileSize
new6.54 KB

I support the previous decision to use the module_name.some_description format as discussed in #1981368: Convert all routes to 'module_name.foo_bar' naming convention. Without having in-depth knowledge of views_ui module (as an example) I had no problem understanding the meaning of the route or recognising a corresponding page or form from only reading the current (single dot) route names.

For consistency I would vote for option 1, to change the menu link machine names to match the route names. A single pattern to learn and therefore better DX.

I made a dump of all static route names (taken from *.routing.yml files). I found 330 routes, and in #7 I found 80 menu links. Renaming routes would not be my priority, be if we do I have a few suggestion:

  • Do not use "admin_" prefix. It is probably a result of the (former) path_as_route_name strategy.
    system.admin_compact_page -> system.admin_compact
      system.admin_config -> system.config
      block.admin_block_delete -> block.delete
     
  • Avoid "_page" or "_form" suffix. The route type ("_content", "_form") already provides this information.
    menu.overview_page -> menu.overview
      image.effect_add_form -> image.effect_add
      simpletest.result_form -> simpletest.result
     
  • Avoid "_list", use "_overview" instead.
    system.date_format_list -> system.date_format_overview
      custom_block.type_list -> custom_block.type_overview
     
effulgentsia’s picture

In #1981368-23: Convert all routes to 'module_name.foo_bar' naming convention, webchick said:

Yeah, we could always discuss re-introducing "sub-namespaces" later, and come up with some guidelines on when we do/do not do that.

Is now an appropriate time to do that, and is this an appropriate issue in which to do it? If so, I'd recommend for entity types and plugin types (i.e., well defined nouns in Drupal's domain language), to use "." as separator. For example, an "image style" is an entity type and an "image effect" is a plugin type, so because of that, those are nouns with well defined meaning in image.module. So instead of image.style_delete and image.effect_delete, I recommend image.style.delete and image.effect.delete.

Re #10, +1 on dropping "admin_", "_page", and "_form". However:

Avoid "_list", use "_overview" instead.

Why? "List" is the term we already use in the entity system, so why use a different term in the route name? I'd suggest the opposite: that for any route that is an entity list but that currently uses "overview" in its name, that we change it to "list". So per #4, we change node.overview_types to node.type.list, since it's a list of node types. I think we have some overview pages that aren't entity lists, so those can possibly remain "overview".

effulgentsia’s picture

Note that #11 might be moot if/when we do #2150747: Switch back to URI templates in entity annotations, since that might pull all entity operation routes out of *.routing.yml. But the fate of that issue is still uncertain, so not sure if we want to proceed here without renaming routes, or with trying to rename them to something we like, so that if that issue doesn't happen, we still have route and link names that we like.

webchick’s picture

To me, this issue should simply do what it says: make the link and route names the match. This is a critical, beta blocking DX problem with the current router/menu link system separation. And since there are far more route names than there are menu links, we should use the route names. Done.

Then, if we want to further bikeshed what we should name these machine names, we can do that in a non-critical, non-beta blocking "nice to have" issue.

tim.plunkett’s picture

+1 for #13.

Though we should be honest that if we pass up the chance to rename routes now, it probably won't happen later (since that would be a BC break).

sun’s picture

I agree with everything @effulgentsia said in #11. Especially on fixing the DX of route names. Unlike @webchick, I think that's an inherent part of the DX problem.

However:

The mere attempt of renaming all of these IDs/names and manually trying to keep them synchronized is a flawed idea, IMHO.

Humans can do this, but humans are guaranteed to fail on every data set that is larger than 3 items. That's what machines are for.

In short, if "make them match and keep them synchronized" is the goal (+1), then #2178723-5: [meta-2] Finalize module-facing API of declaring menu links is the proper solution.


#2150747: Switch back to URI templates in entity annotations will only make the problem harder, regardless of which direction we proceed with, since the effect of that issue inherently means that you will need to dig out the (magic) route names out of entity plugin annotations.

effulgentsia’s picture

I'm ok with limiting the scope of this issue to #13, letting #2150747: Switch back to URI templates in entity annotations deal with entity operations and corresponding route names, and #2178723: [meta-2] Finalize module-facing API of declaring menu links deal with future proofing consistency. Though there is the risk that those two issues won't solve things in a way that makes everyone happy, I don't think it's worth holding this issue up on that.

catch’s picture

Yes I also like #13 and #2178723: [meta-2] Finalize module-facing API of declaring menu links seems like a reasonable place to figure out the rest.

pwolanin’s picture

re: #13. It's impossible to use the route name for all cases - e.g. a sub-tab.

So, is it better DX to have the machine name USUALLY match the route but sometimes be different?

I understand that looking these up can be a pain if they are distinct, but I'm a bit worried and frustrated that we are going to be back to the hook_menu scenario where most devs had NFI what was actually going on becuase many things used the same name/data.

effulgentsia’s picture

Status:Active» Needs review
StatusFileSize
new521 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,183 pass(es).
[ View ]

It's impossible to use the route name for all cases - e.g. a sub-tab. So, is it better DX to have the machine name USUALLY match the route but sometimes be different?

Is that question relevant to this issue? From what I can tell, it's ok for a menu link machine name to be the same as a local task id. To test that, here's a patch that changes system.admin.appearance to system.themes_page even though system.themes_page also exists in system.local_tasks.yml. Doing so doesn't appear to create any conflict from what I can tell in local testing.

So I think what you're pointing out is just an issue for *.local_tasks.yml, where we have both system.theme_settings and system.theme_settings_global as local task ids that both use the system.theme_settings route. Pre-existing condition in HEAD, but yes, I do think that is better DX than to rename all local task ids to never match their route names.

pwolanin’s picture

Yes, it's fine if they match - it won't cause a conflict.

The point I'm making is that it basically gets us back to D7 in terms of possible dev confusion, and I'm worried we've wasted a lot of time to go in a circle.

sun’s picture

To test that, here's a patch that changes system.admin.appearance to system.themes_page even though system.themes_page also exists in system.local_tasks.yml.

This is where you lost me... But at this point, I'm really just trolling to underline #15.

Gábor Hojtsy’s picture

I *think* @pwolanin argues the same names would be confusing in debugging when you see values and you don't know if they refer to the route, the menu item or the tab.

pwolanin’s picture

@Gábor Hojtsy - yes, debugging or even just reading the code. Perhaps we can find some convention of a prefix or suffix that at least avoids them all being the same string?

effulgentsia’s picture

The point I'm making is that it basically gets us back to D7 in terms of possible dev confusion, and I'm worried we've wasted a lot of time to go in a circle.

In D7, I think the confusion was mostly due to the key of the array returned by hook_menu() being simultaneously a route identifier, a menu link identifier, and a local task identifier. Not that the same string value was used for all 3, but that all 3 concepts were literally wrapped up in the same hook, and our other menu.inc functions didn't separate out the concepts in a very clear way.

Now that the 3 concepts are very nicely separated, I don't see where the confusion would come from in having the same identifiers. For example, just like I don't think there's any confusion in book.settings being both the name of a config object/file and the name of a route, I also don't think there's any confusion in it also being the name of a local task and/or a menu link.

Is there any specific code snippet or debugging example you can come up with to demonstrate where it's not clear what kind of object you're identifying?

Sutharsan’s picture

While looking for good or bad DX effects of making route and menu link names match, I discovered this positive effect. Take the #19 example and this scenario: When reading the code below, I ask myself what is the route of this "parent system.admin.reports"?

  $links['dblog.admin.reports.dblog'] = array(
    'link_title' => 'Recent log messages',
    'parent' => 'system.admin.reports',
    'description' => 'View events that have recently been logged.',
    'route_name' => 'dblog.overview',
    'weight' => -1,
  );

Currently it requires two search actions to answer the question. 1. search for "system.admin.reports" and find the menu link that uses this as a key (11 results in 6 files). 2. Take the route name of this link ("system.admin_reports") and search for its definition (5 results in 3 files). The result is found in a *.routing.yml file since route definitions are in files like that. When route and menu link names match the parent route definition is found just by a single search and the route is found in a *.routing.yml file.

effulgentsia’s picture

I'd still like to see this in before beta, but not sure a beta needs to be blocked on "just naming".

effulgentsia’s picture

Perhaps we can find some convention of a prefix or suffix that at least avoids them all being the same string?

I'm not necessarily opposed to prefixing all menu link machine names with menu_link., but that would be different than what we do in every other part of Drupal, where IDs are only unique within their namespace. So I think it only makes sense if we're confident that the potential for developer confusion in seeing a bare ID without clarity on whether it's a link or a route is greater than the potential for confusion in applying a custom ID pattern in this one place. So, any feedback on #24?

catch’s picture

If we're going to make an effort to start supporting head2head upgrade paths at beta,then this needs to be a blocker rather than target.

We store references to route names in a lot of places, so I don't fancy us trying to upgrade all of those to match the new names.

pwolanin’s picture

StatusFileSize
new12.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,031 pass(es), 51 fail(s), and 2,241 exception(s).
[ View ]

Here's a start on system module, but I didn't get them all. crowdcg is going to take a pass at finding more.

crowdcg’s picture

Assigned:Unassigned» crowdcg

Working on cleaning these up.

Status:Needs review» Needs work

The last submitted patch, 29: 2178725-29.patch, failed testing.

crowdcg’s picture

StatusFileSize
new32.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2178725-32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new679.55 KB

Incomplete patch building on pwolanin's in #29. There still are a bunch of updates to be made, which I will make tomorrow.

pwolanin’s picture

Status:Needs work» Needs review

let's see what still fails?

Status:Needs review» Needs work

The last submitted patch, 32: 2178725-32.patch, failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new32.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,021 pass(es), 52 fail(s), and 2,245 exception(s).
[ View ]

Hmm, funny - picture module was removed from core?

This is the same as #32, but omitting picture module change.

Berdir’s picture

Status:Needs review» Needs work

The last submitted patch, 35: 2178725-35.patch, failed testing.

crowdcg’s picture

Assigned:crowdcg» Unassigned
Status:Needs work» Needs review
StatusFileSize
new45.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,309 pass(es), 49 fail(s), and 643 exception(s).
[ View ]
new30.99 KB

Finished up this task...I think.

Of note, based on a discussion with pwolanin, I removed the hook_menu_link_defaults section in custom_block.module as these are local tasks. Also, in menu_test.module I left these as is, but updated $items to be $links for consistencies sake.

Status:Needs review» Needs work

The last submitted patch, 38: 2178725-38.patch, failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new2.61 KB
new46.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,293 pass(es), 39 fail(s), and 23 exception(s).
[ View ]

Fixes a per-existing mis-named hook in comment module (needs a test?)

Also cleans up a few missed conversions.

Interestingly - if the specified parent machine name doesn't exist, you get unpleasant SQL errors. Maybe we should throw an exception, rather than try to save this invalid data. See linee ~ 1686 in menu.inc below "Handle any children we didn't find starting from top-level links.".

Berdir’s picture

Status:Needs review» Needs work

The last submitted patch, 40: 2178725-40.patch, failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new1.13 KB
new47.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,418 pass(es), 7 fail(s), and 23 exception(s).
[ View ]

Missed small changes needed in for the changed machine name in user_menu_link_presave() and user_translated_menu_link_alter()

Status:Needs review» Needs work

The last submitted patch, 43: 2178725-43.patch, failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new1.09 KB
new47.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,589 pass(es).
[ View ]

Fixes one parent missed in a test hook, and adds some code comments there.

pwolanin’s picture

45: 2178725-45.patch queued for re-testing.

pwolanin’s picture

Issue summary:View changes
Wim Leers’s picture

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

I verified each change in this patch manually, and it all checks out. RTBC!

+++ b/core/modules/block/custom_block/custom_block.module
@@ -38,24 +38,6 @@ function custom_block_help($path, $arg) {
-function custom_block_menu_link_defaults() {

This is pretty much the only "special" thing in the patch. These are a local action and task respectively now. So this removal is good :)

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

Let's document this coding standard in hook_menu_link_defaults() in system.api.php

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new2.25 KB
new48.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,628 pass(es).
[ View ]

Here's with added documentation in system.api.php and one wording change suggested by Wim in person.

Wim Leers’s picture

Status:Needs review» Reviewed & tested by the community

Doc changes look good to me! :)

  • Commit fa51d2e on 8.x by alexpott:
    Issue #2178725 by pwolanin, crowdcg, effulgentsia, Sutharsan: Make all...
alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed fa51d2e and pushed to 8.x. Thanks!

It would nice to create a followup to add a coding standards test to test that core follows this recommendation.

swentel’s picture

Status:Fixed» Needs work
StatusFileSize
new26.66 KB
new40.21 KB

If you enable content translation and language module, the links do no show up on 'admin/config'

it should look like this

Maybe there are other links missing, haven't checked them all.

swentel’s picture

Status:Needs work» Fixed

Ok, false alarm, sorry about that. Devel threw an error and then everything stopped.

swentel’s picture

Talked with Alex and we should probably make sure when an exception is caught, all other valid menu links/routes are still ok though.

mondrake’s picture

Same here as #54, any time I install a module I get the following watchdog entry:

Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'menu_name' cannot be null: INSERT INTO {menu_links} (menu_name) VALUES (:db_insert_placeholder_0); Array ( [:db_insert_placeholder_0] => ) in Drupal\menu_link\MenuLinkStorageController->save() (line 107 of /[...]/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php).

Fresh install.

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.