[meta] Finalize the menu links (and other user-entered paths) system

Added an issue for peter

I gave this a lot of thought after reading the outcome of the recent meeting and I think I have a viable alternative.

Let's start with the biggest (or single) shortcoming of the current link field: it has to denormalize a user-entered path into a route name and route paramateres because the new routing system is faster at generating links based on those two properties. The problem with this denormalization is now quite well documented in the issue summary, the /blog path use case.

TL;DR Rewrite the 'link' field type so it doesn't store any information about the route or path anymore but reference another object where those details are stored. In this way, there is a single place (e.g. db table) that needs to be updated when a route name is updated on router rebuild.

Some fundamental points of this proposal, all in the context of the current issue summary:

• A menu item is basically composed of a link (internal or external) and a hierarchy.

  The 90% / 10% separation between "Select from your existing content" and "Enter a path (internal/external)" is probably a good estimate for site visitor facing menus, but what about site admin facing menus like "Administration"? That one is also editable in Menu UI and if we force every item there to be a path reference, wouldn't that make it quite slow to render?

• A shortcut is just an internal link.

  Because shortcuts 99% of the time go to admin paths, resolve these to a route, so links stay working even if a module changes from admin/foo to admin/bar. Store a route reference.

  This is not true. Admin paths can also be provided by views (like many content entity listings in D8) that could be switched to a panel for whatever reason, just like the /blog case for menu links. So a shortcut's link has exactly the same needs as a menu item's link, without the external part.

• Link field serves many use cases, so we can't predict what people will want.

  The link field has the same use case as menu items: link to an internal or external path.

Given the points above, I still think that the base data model should not be dictated by the UI (the 90% existing content / 10% path separation) but architected around a proper link field implementation which can be used by menu items, shortcuts and every other project-specific entity type that needs it.

So here's a concrete proposal for how we can fix the issues with the current link field and satisfy all use cases:

1) create a new entity type, let's call it 'cached_path' for now, with these properties/fields: 'route_name', 'route_parameters', 'options', 'path' (which *always* stores the exact user-entered path) and maybe 'external'.

2) scratch the current code of the link field and rewrite it like this:

• extend the EntityReference field type with two additional properties: 'title' and 'target_entity_object' (or something similar)
• hard-code the 'target_type' field storage setting to the 'cached_path' entity type
• use most of the code from the current link widget for the MVP, which is just a textfield that accepts a path as input
• the widget will create (or update) the referenced 'cached_path' entity
• the 'title' property allows a user to enter an optional title for this link, but it's only optional if we determine that the path is internal in a validation step
• the 'target_entity_object' property will store data like <entity_type>:<entity_id> if we determine that the path is a) internal and b) it links to an entity, which helps with querying menu items that reference existing content

Some other random thoughts:

- the only reason to expose 'cached_path' as an entity type is to be able to re-use as much existing code in HEAD as possible, like the ER field and maybe the KeyValue entity storage; it will have to be as "slim" as possible, probably not a content entity and just extend the base Entity class directly in order to skip the overhead of typed data
- having it as an entity type is definitely not the central point of this proposal, we just need the link field type to reference "something"

I realize most people are already sick and tired of this problem space but since I already put a lot of time into designing this system before the "new plan for menu links" came up last year, I think it may still be useful to analyze all possible options :)

Edit: fixed a missing [code] tag.

@amateescu - I don't think your proposal addresses as completely the cases address in the issue summary. I don't think it makes sense to try to force all these into one solution when they actually behave differently - I'm sorry you weren't on the call where we talked through each one.

Edit: removed.

I agree that the UI part is really tricky to get right, but if we do the separation at the base level aren't we limiting the UI choices? A field type can have a widget with two inputs but two field types can not have a single widget in case we find a solution to reference both existing content and paths in a single input.

Maybe my previous comment was too much to take in at once, so I'll try to be more clear.

1. Because shortcuts 99% of the time go to admin paths, resolve these to a route, so links stay working even if a module changes from admin/foo to admin/bar. Store a route reference.

   Do we agree that shortcuts can also link to paths for which the route name can change, like admin/content, so storing a route reference will not work?

2. Link field serves many use cases, so we can't predict what people will want.

   Can it link to something that a menu item can not?

Do we agree that shortcuts can also link to paths for which the route name can change, like admin/content, so storing a route reference will not work?

Yeah, we have to talk about those ones in more detail, no question. That is a valid point, but you also have to see, that those at least don't break things for the site visitors.

I agree that the UI part is really tricky to get right, but if we do the separation at the base level aren't we limiting the UI choices? A field type can have a widget with two inputs but two field types can not have a single widget in case we find a solution to reference both existing content and paths in a single input.

Well, I think if you really want that to have it, we could develop a field type which contains both a (D)ER field and a path at the same time,
but again, as chx wrote, the issue summary says, etc. just a path is not enough.

Link field serves many use cases, so we can't predict what people will want.

IMHO menu items are conceptually a part of Drupal, while link items could be used to just store paths, for example as backend for some sort of REST stuff.

That is a valid point, but you also have to see, that [shortcuts] at least don't break things for the site visitors.

Unless you want to use them as a "bookmark this page" feature, which IMO is a perfectly valid use case for them.

Well, I think if you really want that to have it, we could develop a field type which contains both a (D)ER field and a path at the same time

Which is exactly the field type architecture proposed in #7 for a proper link field? :) Except the (D)ER part is just the ''target_entity_object'' property which stores a <entity_type>:<entity_id> string and the 'path' is a reference to the 'cached_path' object which can be updated on router rebuild.

@catch said in #2407587-23: Allow multiple target entity types in the entity reference field

the core use-case only requires basic EntityFieldQuery (find the menu links referencing X entity) and entity loading (load the entity referenced by Y menu link)

which is supported by the proposal above.

IMHO menu items are conceptually a part of Drupal, while link items could be used to just store paths, for example as backend for some sort of REST stuff.

The could part is a bit ambiguous in your answer. Maybe I need to rephrase the question: If we want to support linking to internal paths/routes/whatever in the link field, doesn't that mean we need the exact same linking capabilities that menu items have?

OK after reviewing amateescu's proposal and discussing a fair bit in irc, here's my current thoughts:

Things that seem obvious

- the fundamentals of storing an entity reference or a literal path but nothing else remains the same with both proposals.

- we accepted on the call that shortcut has to make a decision on route vs. path (because 99% of shortcuts are added from the page, not a form), and either way will break in some cases. It's arguable which case will break more - depends on what gets added. I think it's reasonable for shortcut module to be refactored to use the link field regardless of whatever else happens.

- if menu links are going to store a literal path, then using the link field for the literal path storage seems reasonable unless there were massive obstacles to doing so.

- I don't think we agreed two entity types, one entity type with two bundles, or a single bundle for menu links on the call at all (though Peter seemed to think we had? but I definitely hadn't at least). My preference is for a single bundle.

Things I think more discussion/am not sure about personally

1. Whether or not the shortcut move to link field is critical or not.

2. I'm not at all sure whether it's a good idea for the link field to add entity reference support. To be honest, that sounds like a field collection with a DER + link field. I'm not suggesting we add field collection to core however.

3. I can see the use case for link or entity ref generically - i.e. we know we have that with menu links, and it could be used for something like a 'further reading' field that allows both external and internal references side by side. Whether that should be in a single field type or a combi-field is the sticking point since the use cases for the separate types seem more numerous than for the combination.

4. It's a side issue, but we've not discussed for either menu link or shortcut is whether query arguments should or could be respected. If we're storing literal path input then as a user I'd expect that to persist (i.e. a link to the RTBC queue). Do we need a separate issue for that?

Really contentious and also not sure about personally

1. Two fields, one DER and one link. Some additional logic (where? entity form?) makes the UI smoother and ensures they're mutually exclusive. This could be used as a pattern by other modules but isn't 100% re-usable as is. However DER and link field are clearly reusable independently and there's use cases for both separately.

2. One field that combines the storage model of link and DER, and the logic to keep those mutually exclusive would be handled at the formatter/field level. This could be re-used elsewhere but there are limited use cases for that field type compared to the two separately.

1. Two fields, one DER and one link...
2. One field that combines the storage model of link and DER...

I think both of those options from #14 assume that some kind of DER field type gets added to core in #2407587: Allow multiple target entity types in the entity reference field. If that happens, then great, but I think there's also some good arguments on the issue against it. In the event that we "won't fix" that issue, I'd like to suggest a 3rd option to add to the above:

3. Have a link field item store "path" (what LinkItem::propertyDefinitions() currently calls 'url', except it's not always a full URL). And invent a new scheme: 'entity'. So, if the user types 'blog', store 'blog'. If the user types 'http://example.com', store that. If the user selects node 1 from some kind of ER selection plugin, then store entity://node/1. And if the user selects the frontpage view, store entity://view/frontpage.

I'm opposed to trying to make one field that combines the storage model of link and DER (in other words, a multi-column storage). We would be digging ourselves into a huge hole there, and this task will not get done.

Let's talk about storage more than UI - that's really what we need to finalize:

• Link Field:
  Just store the user entered path in a text field, resolve at runtime.
• Menu Link Content: options
  1. 2 entity types, 1 with DER base field, 1 with a path base field. This is the cleanest in terms of plugin derivative discovery logic (we could have a wizard that lets you create one or the other fro the same form).
  2. 1 entity type, 1 bundle, with both a DER and path base field, some tweak to the entity form and validation to make them mutually exclusive. The plugin derivative discovery would be a bit of a hack here, but probably something we could live with.
  3. 1 entity type, 2 bundles, hide one of DER or path field based on bundle. This seems too ugly to consider seriously.
• Shortcut: options
  1. Just store the user entered path, resolve at runtime (same as Link field)
  2. Just resolve the user-entered path to route and params, store those (doesn't support external link, unique storage?)
  3. Have both a DER field and a path field (same storage as menu link options 2 & 3), resolve at run time
  4. Have both a DER field and a path field (same storage as menu link options 2 & 3), plus also store the entity route

IMPORTANT NOTE: this list does not include all possible options, but is the list of options that I think we could feasibly accomplish in the near term (i.e. so this issue is not the last one blocking the upgrade path).

I'm not at all sure whether it's a good idea for the link field to add entity reference support.

With #15.3, we'd at least have that at the storage level. And even with the link field's default widget, someone would be able to type 'entity://node/1'. Contrib could then decide if it wants to offer up a better widget that integrates with ER selection plugins.

Having more discussion in IRC with catch and larolan (and webchick until she had to go fly) - coming to some agreement that possibly the approach of 15.3 could be used for a single link field that we apply to all 3 use cases.

The link field would have 4 columns: title, description, uri (or path), and options.

1st patch would just be a text input for the uri/path

follow-up patches would make the widget fancier so it would have a DER-like entity picker plus a field for user-entered path.

For menu links we'd add an extra boolean base field not exposed in the UI to flag those with non-entity paths to be re-resolved on menu rebuild.

Just discussed entity:// in irc with larowlan and pwolanin.

For discovery we need to exclude entity links when updating route/parameters in the menu tree.

Two ways to get around this:

- NOT LIKE('entity://%')
- Add a boolean field to menu links, no widget, and do the same logic on save.

Need to add description column (or otherwise handle description translation) on the link field, right now while it supports options/attributes there's no UI for them.

entity:// means we can completely decouple the data model work from the UI - i.e. the very worst case is you get a text field like now in the menu UI, and that's enough to make everything else work.

Overall that feels encouraging.

FWIW I also think #15.3 is the most promising solution. @eff++

Not sure if a child issue for #19 has been opened yet, so I opened one here: #2411143: Change LinkItem schema to store URIs rather than URLs/paths/routes. Feel free to close that as a dupe if it is.

#15.3++
If this has been agreed on, I'd be happy to work on the entity stream wrapper.

Created #2411333: Create standard logic to handle a entity: URI scheme

@effulgentsia++
I really like the idea to decouple the data model from the UI.
As discussed with catch its not yet clear how to use that exactly for menu links, given that we still, at least in my thinking world, store route names in the tree.
But I'm confused we can figure out, how exactly we want to deal with it.

Thanks everyone for keeping an open mind here!

I like @effulgentsia's proposal too, it's quite similar to #7 and only differs in two aspects:

1. it loses the indirection of referencing a different object (the "cached_path" stuff) and with it the ability to have an optimized storage for the 10% path reference case on link fields and shortcuts, but given the sanity of having the same storage model and expectations everywhere, which was exactly the purpose of my proposal, I can live with that :)
2. it folds the extra "target_entity_object" column/property into the URI property, thus requiring a query that will be a bit more expensive: NOT LIKE('entity://%') on the 'uri' column vs. IS NULL on the 'target_entity_object' column. I can't say if this will mean a lot in terms of performance so I'm not going to push it further than this comment.

Since this meta also touches the UI parts, here's my idea for the final widget design:

• keep it as a single (autocomplete-like) textfield
• when the user types something that starts with a '/' or the final string (probably copy-pasted) starts with 'http://', no autocompletion will be provided and just store what was typed - this is the path reference case
• when the user types something that doesn't start with a '/', provide (D)ER autocompletion for a list of entity types configurable at the widget level

@almaudoh - I'm not sure this makes sense as a stream wrapper at all. Those are for files. This is just a scheme like base:// we are already using.

@pwolanin yeah. After I opened that issue and thought about how to write the patch, I figured it didn't need to be a streamwrapper. So then that means that issue may have to be rescoped.

Added another critical which I think this blocks

And another major that I think this blocks: #2292159: EditorLinkDialog should validate URLs, and autocomplete like the Link widget.

A good example of route name change that coused css issue #2413723: Toolbar icon for "People" item broken

Based on discussion with pwolanin, effulgentsia, xjm, Tim Plunkett, dawehner and mpdonadio, we decided to:

1. use entity:, not entity://. Reasons:
   1. it's wrong to use // in the URI, because it requires an authority to be defined (and a path). The authority must contain a hostname. Omitting the double slash removes the need to specify an authority and hence hostname. We only want the path part; there is no sensible hostname that we can define; the Drupal site is the authority, it is the host, hence we only want a part.
      (See http://en.wikipedia.org/wiki/URI_scheme#Generic_syntax for details.)
   2. Not only is it a violation of the spec for URIs and hence a theoretical error, it also has practical consequences: it prevents us from using parse_url().
2. analogously introduce user-path:, not user-path://; the same reasons as above apply.
3. analogously update base:// to base:; the same reasons as above apply. Changing this causes absolutely minimal disruption.
4. not update public:// and private:// to remove the slashes, because PHP's stream wrappers must use double-slashed schemes: http://php.net/manual/en/wrappers.php. On top of that, it'd be much more disruptive too.

We also determined that we don't need a description property on LinkItem: see #2413029-16: Change LinkItem schema to also store a description.

Filed #2416763: Convert Url::fromUri() base:// scheme to base:.

Added another subissue.

listed two issues this blocks in the summary.

One of them is not needed anymore.

Posted #2417459: Provide internal API for special schemes and thin public wrappers for user input and non-routed URLs.

.

Given that, I'd propose demoting this meta issue to a "major" one. I think it still has value in laying out the problem space, and in tracking various dependent issues, but I don't think getting every single one of these sub-issues done (apart from the two already marked critical) is required for us to ship D8.

I think we could postpone it once we have a minimal UI with autocompletion support, as this is the first time, the initial brittleness issue is completly tackled.
After that I think you are right, moving it to major and link to further improves is a good idea.`

maybe something from the recent menu work caused this: #2423153: Add menu from the editing page doesn't save the changes

During the NJ sprint, some of us discussed URL generation when paths are tokenized (such as in Views). I don't know if some other issue was made for that, but I didn't find one, so opened this one: #2426399: FieldPluginBase::renderAsLink() loses language prefix for tokenized paths. Not adding it as a child of this, since the issue title of this is "menu links", and that's a link in Views, but just commenting here to notify followers of this issue.

The CR in https://www.drupal.org/node/2417421 never got published, so I did that now.

However, it's a bit incomplete and some of the information is incorrect. E.g., user-path: a.k.a. internal: does not replace base:; it just serves a different usecase.

is the only remaining task here to correct the change record?
or are we keeping this open for all the remaining open child issues? (are they all officially "children" or some of the related referenced by issues also?)
maybe we should explicitly list the ones this is open for in the summary?

I think the meta should stay open as there are numerous outstanding sub-issues, some of which are major. I think anything that's a child that's not postponed to 8.1.x is relevant still.

Added a few issues that were missing the parent as well.

The remaining child issues (and some of the original ones) cover more than just menu links, so retitling this meta. Leaving it in the "menu system" component, because I don't know where else to assign it.

More title tweaking.

What about URL aliases? The field to enter the existing path could also be an autocomplete ...
I'm the maintainer of MPAC and with adding autocomplete to the shortcut and menu link forms the module is nearly obsolete now ;)

@StBorchert:

#2418017: Implement autocomplete UI for the link widget
#2292159: EditorLinkDialog should validate URLs, and autocomplete like the Link widget

@hass: uhm, I've seen these issues before, but how do they affect the form for URL aliases (PathFormBase)?

@stBorchert
Interested idea.
#2430593: Store {url_alias}.source and .alias with a leading / is partially related but doesn't talk yet about the UI.

These issues make MPAC obsolete I think. They are adding the autocomplete to the path fields what MPAC does, isn't it?

Added as child issue #2501691: Change content-types, comment-types, and block-types URLs

#60 said:

I think the meta should stay open as there are numerous outstanding sub-issues, some of which are major. I think anything that's a child that's not postponed to 8.1.x is relevant still.

8.0.0 has been out for almost two months now. We can't do any more "finalizing" work.

Does that mean we should close this? Most child issues have been fixed. I looked at the remaining ones, and closed a few more, unpostponed a few, bumped several to 8.1 and left a few that can be fixed without breaking BC open against 8.0.x.

We've got open issues for things we still want to/should do. This issue is linked from many other issues, so we can easily find back this issue to read our original thinking again.