Problem/Motivation

The way that the various *links.yml files are named make it hard to easily tell what a module implements, and require module developers to know special names for each subsystem. Conceptually, these are all "links."

Proposed resolution

Rename the files to:

*.links.action
*.links.contextual
*.links.task

This makes the naming more consistent and as an added bonus makes the files appear grouped together in a directory listing.

*.links.menu is slated to be renamed also but it will be handled in #2303605: Rename *.menu_links.yml to *.links.menu.yml files to improve DX.

Remaining tasks

Do that.

User interface changes

N/A

API changes

File names change.

Original report by @webchick

Gábor created and tweeted around this diagram:

It's lovely that we now have this documented. But holy complicated, Batman. :\ That's up to 5 different files I need to open and read in order to figure out what a module is doing. In the past I only had to look at hook_menu() and it was all there.

I totally get the benefits of splitting routing from links. I don't understand what we gain though by being this granular the types of links themselves. I think to most people it's just "links."

Proposal would be just keep:

*.menu_links.yml

And then indent the YAML definitions by an appropriate key, for example, book.menu_links.yml would be:

links:
  book.admin:
    title: Books
    description: 'Manage your site''s book outlines.'
    parent: system.admin_structure
    route_name: book.admin
  book.render:
    title: Books
    route_name: book.render
    hidden: 1

local_tasks:
  book.admin:
    route_name: book.admin
    title: 'List'
    base_route: book.admin
  book.settings:
    route_name: book.settings
    title: 'Settings'
    base_route: book.admin
    weight: 100
  book.outline:
    route_name: book.outline
    base_route: node.view
    title: Outline
    weight: 2

What say you?

Files: 
CommentFileSizeAuthor
#25 2291137-25.patch23.91 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,227 pass(es). View
#25 interdiff-23-25.txt1.51 KBcilefen
#23 2291137-23.patch33.12 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,193 pass(es). View
#23 interdiff-21-23.txt3.65 KBcilefen
#21 2291137-21.patch29.23 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,230 pass(es). View
#21 interdiff-19-21.txt1.01 KBcilefen
#19 2291137-19.patch22.57 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,199 pass(es). View
#19 interdiff-17-19.txt454 bytescilefen
#17 2291137-17.patch14.35 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,211 pass(es). View
#17 interdiff-15-17.txt907 bytescilefen
#15 2291137-15.patch10.54 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,211 pass(es). View
#15 interdiff-12-15.txt683 bytescilefen
#12 2291137-12.patch9.73 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,167 pass(es), 44 fail(s), and 0 exception(s). View
Bq0JZ5bIMAA7-Ex.png-large.png190.85 KBwebchick

Comments

chx’s picture

xjm’s picture

I don't think that having one file versus four files is really much of a gain. Either it's long and hard to read, or you open four things. To me it makes sense to have them separated conceptually.

alexpott’s picture

Well I guess this might depend on how many menu links, local tasks, local actions, or contextual links a module is providing. If you're providing one of each then four files might feel like overkill. But on the other hand these "links" are not the same thing and they have different data structures. Also the lack of a file makes it easy to discover when a module does not provide something.

effulgentsia’s picture

Well I guess this might depend on how many menu links, local tasks, local actions, or contextual links a module is providing. If you're providing one of each then four files might feel like overkill.

Just looking through core modules, looks to me like a lot provide 2 or 3.

But on the other hand these "links" are not the same thing

They're both similar and different. They're all links. But they have different UX purposes and therefore get rendered in different places with different hierarchy organization.

and they have different data structures.

Their data structures are both similar and different. They all share the same semantics of title, description, route_name, weight, and class. But related to hierarchy, they differ with respect to parent, parent_id (though that's silly, it should be renamed to parent), base_route, group, and appears_on.

Therefore, in order to communicate/represent the similarities and differences, what about keeping them as separate files, but renaming them to:

  • *.links.actions
  • *.links.contextual
  • *.links.menu
  • *.links.tasks

?

tim.plunkett’s picture

I'm hugely -1 on combining these.

However, I'm not averse to renaming them. I never liked "local tasks" and "local actions" anyway, since they're all just links.

Another options is this:

  • *.actions.links.yml
  • *.contextual.links.yml
  • *.menu.links.yml
  • *.tasks.links.yml

vim *.links.yml

tstoeckler’s picture

I'm not against combining them, but between #4 and #5 I'm very much for #4. vi *.links.yml is nice, but so is vi mymodule.links.*. And #4 has the benefit of neatly showing the files next to each other in the the file browser.

Gábor Hojtsy’s picture

The 'subtle' differences are even more spelled out when you want to create dynamic items of hook_menu() successors. Opened this issue to summarize the differences and similarities: #2291773: Creating dynamic hook_menu() successor items use different APIs maybe we want to do something about this, maybe not :)

dawehner’s picture

Having them in separate files certainly helps people to remember that there are differences. Otherwise they really easy might just copy and paste other bits and wonder why they don't work.

*.links.actions
*.links.contextual
*.links.menu
*.links.tasks

I also prefer this variant but it would be great to keep it either singular or plural so:

*.links.action
*.links.contextual
*.links.menu
*.links.task

On top of that having them in one file would make scanning of them a bit worse on the performance side, because as the
subsystems are decoupled all of them would have to read the merged file anyway. Not sure whether this though actually
matters given that it is cached but yeah a full router + menu rebuild is always potentially problematic.

catch’s picture

We discussed one file before, I have no idea which issue it was, from what I remember the pros and cons were similar (single file for overview vs. making the separation between systems clear and reducing parsing overhead). Another thing to consider is that (at least) contextual module is optional, so if we have information in one file we're having to parse YAML that will never be used on some sites. There's also the ability to completely ignore the other types altogether if you have a module that just adds a single settings admin page or similar.

So, I'd also like to keep single files, but I like dawehner's second variant in #8.

webchick’s picture

Title: Proposal: Consolidate all of the various *links.yml files » Proposal: Rename all of the various *links.yml files so they appear together in directory listings
Issue summary: View changes
Issue tags: +API change

#8 would definitely be a big improvement over the status quo. #3, #4, and #9 lay out a sensible reasons why we probably don't want to do what I said. Changing the title and updating the issue summary accordingly.

cilefen’s picture

This is not clear to me: Is the intent that these would actually be named with the YAML file extension?:

*.links.action.yml
*.links.contextual.yml
*.links.menu.yml
*.links.task.yml
cilefen’s picture

Status: Active » Needs review
FileSize
9.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,167 pass(es), 44 fail(s), and 0 exception(s). View

I am trying local_tasks here. This will certainly fail the bots.

cilefen’s picture

Status: Needs review » Needs work

The last submitted patch, 12: 2291137-12.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
683 bytes
10.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,211 pass(es). View

Updated the local tasks test base. We still needs the other link types.

cilefen’s picture

Status: Needs review » Needs work
cilefen’s picture

Status: Needs work » Needs review
FileSize
907 bytes
14.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,211 pass(es). View

links.contextual. I am not dealing with mentions in comments yet.

cilefen’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record updates
cilefen’s picture

Status: Needs work » Needs review
FileSize
454 bytes
22.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,199 pass(es). View

links.menu

Todo: links.action, comment mentions, and change records.

cilefen’s picture

Status: Needs review » Needs work
cilefen’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
29.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,230 pass(es). View

links.action

Todo: comment mentions and change records.

dawehner’s picture

This looks really good so far!

It maybe makes sense to postpone the links.menu_link changes, given that this will conflict with the menu link patches, like #2301239: MenuLinkNG part1 (no UI or conversions): plugins (static + MenuLinkContent) + MenuLinkManager + MenuTreeStorage

cilefen’s picture

FileSize
3.65 KB
33.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,193 pass(es). View

Found a few of the comments, but may have missed a few.

Todo: change records. Postpone all or part on #2301239: MenuLinkNG part1 (no UI or conversions): plugins (static + MenuLinkContent) + MenuLinkManager + MenuTreeStorage if necessary, depending on the impact on that issue.

dawehner’s picture

I would be fine with temporarily moving just the three types.

cilefen’s picture

FileSize
1.51 KB
23.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,227 pass(es). View

This patch is meant to address only:

*.links.action
*.links.contextual
*.links.task

*.links.menu is being postponed.

Todo:

cilefen’s picture

cilefen’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

The change notice coverage looks fine as well as the patch.

tim.plunkett’s picture

Title: Proposal: Rename all of the various *links.yml files so they appear together in directory listings » Rename all of the various *links.yml files so they appear together in directory listings

+1

sun’s picture

Title: Rename all of the various *links.yml files so they appear together in directory listings » Rename various *links.yml files to improve DX

FWIW, contextual (module) appearing in that list implies that contrib modules are supposed to advance on this pattern by using their own suffix.

At the same time, the suffixes "action", "task", and "menu" do not correlate to a module, but are sorta "reserved" by Drupal core.

I assume that is intended, but worth to point out.

cilefen’s picture

@sun Contrib modules must use the new names. Maybe we could be clearer on that in the change record.

cilefen’s picture

@sun Oh sorry I misunderstood your comment.

neclimdul’s picture

Sorry for finding this so late in the discussion. To me, this implies some connection between these system that are pretty disparate. Putting links on the page is just something we do because we write HTML. Ditto on routing.yml, everything is going to point to that, because that's the core of our system.

I guess I just want to make sure we are sure we're not doing this based on the history of hook_menu and our biases as experienced Drupal developers rather then actual clarity.

effulgentsia’s picture

To me, this implies some connection between these system that are pretty disparate.

If we consider our other module-level YAML files, we have .info.yml, .routing.yml, .services.yml, .libraries.yml, and maybe eventually .events.yml. Those are much more disparate concepts than what variants of link groups are to each other. So I think them staying separate files is a good enough acknowledgement of their disparateness, but putting them all under a common "links" namespace is a good way to group them conceptually.

FWIW, contextual (module) appearing in that list implies that contrib modules are supposed to advance on this pattern by using their own suffix. At the same time, the suffixes "action", "task", and "menu" do not correlate to a module, but are sorta "reserved" by Drupal core.

Well, I think menu is also well named, as that relates to the \Drupal\Core\Menu component. I suppose action and task aren't properly namespaced and are just being grandfathered in: similar problem we have elsewhere in core (e.g., a date_format entity type). Since the lack of component or module namespace for 'local_tasks' and 'local_actions' is a preexisting condition of HEAD, I don't think this patch makes the problem any worse.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Okay let's get this in whilst the patch applies and we've got no new files.

Committed f2b6aa7 and pushed to 8.x. Thanks!

sun’s picture

Thanks! This is definitely an improvement to status quo and improves DX.

I would, however, recommend to leave the door open for further improvements. To improve consistency of definitions across files (cf. #4), but also, to discuss and consider a potential merge of e.g. actions and tasks into a single file, in separate issues.

  • alexpott committed f2b6aa7 on 8.x
    Issue #2291137 by cilefen | webchick: Rename various *links.yml files to...
alexpott’s picture

Gábor Hojtsy’s picture

The https://www.drupal.org/node/2122231 page is a good starting point, since that search does not seem to yield any results.

dawehner’s picture

Hopefully all pages are covered

lussoluca’s picture

https://www.drupal.org/node/2302893 mention that also *.menu_links.yml has been renamed to *.links.menu.yml but this seems not true.

cilefen’s picture

Issue summary: View changes

@lussoluca You are correct about that.

cilefen’s picture

cilefen’s picture

Is it proper form to update superseded change records?

https://www.drupal.org/node/2302893 references old change records that may need updating if that is true.

Gábor Hojtsy’s picture

Updated the D8 code in potx translation parser: #2305555: All the links files got renamed in Drupal 8

effulgentsia’s picture

Title: Rename various *links.yml files to improve DX » [Needs change record updates] Rename various *links.yml files to improve DX
Status: Fixed » Needs work

Is it proper form to update superseded change records?

Yes.

cilefen’s picture

Updated the prior change records. Can someone give them a quick review, update the title of this issue, and mark this "fixed"?

The prior notices are linked from this one:
https://www.drupal.org/node/2302893

cilefen’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Fixed

These updates looks perfect.

cilefen’s picture

Title: [Needs change record updates] Rename various *links.yml files to improve DX » Rename various *links.yml files to improve DX

Status: Fixed » Closed (fixed)

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