Problem/Motivation

Currently it is not possible to document what slugs can be used in a route path. Also, there's a lot of one off code to get entities working.

Proposed resolution

Plugins provide a simple and quite powerful solution. They can be much simpler than paramconverters as the manager picks which one to use and they only need to do one thing instead of figuring out which parameters they need to act on.

We can easily generalize everything already done for entities: since the name of the arguments are supposed to be the name of the slugs, we just map the name of the arguments to plugins. This is done using the existing reflection code. For making sure we have the right class, I added a parameter_class to the annotation which specifies what class this parameter should be. If an argument doesn't map directly to the name (the {node1} case) then it can just define the plugin_id in options/parameters, exactly like before, an an example will be provided below with a dynamic plugin id.

Entities are handled by an entity plugin and a plugin deriver familiar from REST and Typed Data (in fact the code is 90% same to the REST deriver). So the "official" slug name is like {entity:node} but we have (simple) code to use {node}: besides mapping {foo} to the plugin id foo, we try mapping to entity:foo as well. This is the only entity specific code. Since we have parameter_class there is no confusion between the two. If you have a plugin called node and it is not an entity, it will just work and not be confused with entity:node (because entity:node has parameter_class NodeInterface and this node plugin has something else).

We allow for dynamic plugin IDs: if you have /foo/{bar}/{baz} then 

options:
  parameters:
    baz:
      plugin_id: something:{bar}

Exactly like how {entity_type} was handled before but for everything.

Remaining tasks

I am getting a notice about auto_placeholder_conditions. Also need to figure out what to do with paramconverter.configentity_admin . Perhaps config entities need to be smarter for admin routes.

User interface changes

API changes

We can keep backwards compatibility just mark paramconverters deprecated slated for removal for D9.

Data model changes

CommentFileSizeAuthor
#7 2558937_7.patch42.08 KBchx
#3 2558937_3.patch36.1 KBchx
#2 2558937_2.patch40.5 KBchx
plugins.patch19.92 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx created an issue. See original summary.

chx’s picture

chx CreditAttribution: chx commented
FileSize
2558937_2.patch40.5 KB
chx’s picture

chx CreditAttribution: chx commented
Issue summary: View changes
FileSize
2558937_3.patch36.1 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three for Drupal Association commented

I don't know what I should write as a respond here, I try to be pratical.

The issue title should be named the following: "The available slugs aren't documented" ... if this is the major critique you apply here.
Which solution is chosen is then a detail, but we should focus on solving problems and not limit our bind in the first place.
Just in general regarding tagged services vs. plugins, we are using tagged services in a lot of places related to routing, so the issue summary should document the advantages of the specail cases.

Afaik this would solve #2470926: Don't load all the paramconverters on every request., the cache entry for all plugins might be not entirely small and we have to run more code in total.

Also, there's a lot of one off code to get entities working.

While this is right, this is caused simply by the fact that we have some complex magic going on. The patch keeps basically the same complex logic (look into the controller ...),
so I try to understand how we manage to get rid of so much code.

chx’s picture

chx CreditAttribution: chx commented

We are getting rid of so much code because we reuse everything written for entities for generic. It works, mostly.

chx’s picture

chx CreditAttribution: chx commented

Also, it's not just docs but also the power and simplicity this brings.

chx’s picture

chx CreditAttribution: chx commented
Issue summary: View changes
Status: Active » Needs review
FileSize
2558937_7.patch42.08 KB

So a little entity code is necessary to put the controllers on for _entity_view etc. But that's it. Also, it's build time now instead of runtime.

Status: Needs review » Needs work

The last submitted patch, 7: 2558937_7.patch, failed testing.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Needs review

Yeah, I know it doesn't work but the architecture et al needs review still.

chx’s picture

chx CreditAttribution: chx commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three for Drupal Association commented

Well, you know, I try to be as objective as possible:
These are the changes I see from a really high level on this patch:

-class EntityRouteEnhancer implements RouteEnhancerInterface {
-class EntityResolverManager {
-class EntityConverter implements ParamConverterInterface {

+class PluginRouteAlterSubscriber implements EventSubscriberInterface {
+class PluginConversionEnhancer implements RouteEnhancerInterface {
+class Entity extends PluginBase implements RoutePluginInterface, ContainerFactoryPluginInterface {
+class EntityDeriver extends DeriverBase implements ContainerDeriverInterface {
+class RouteManager extends DefaultPluginManager {

so I ask myself, is this really simpler? I know its unfair, because I guess we could remove ParamconverterManager as well, but just saying that the overall architecture is more complex with that patch.

chx’s picture

chx CreditAttribution: chx commented

> is this really simpler?

The patch currently removes more line of code than it adds despite ParamConverterManager is not removed. That's one objective metric but a lousy one so let's see:

  1. Simpler to learn: you already need to know plugins, so many things are plugins.
  2. Less code to write per slug: we do away with applies and just go by plugin id. This seems to be enough.
  3. Code wise it costs HEAD nothing: it's just EntityResolverManager rewritten into a plugin manager. And so yes, it's simpler: Instead of ParamConverterManager and EntityResolverManager you just have RouteManager. EntityResolverManager was 231 LoC, the RouteManager is 188.
  4. The plugin id (via derivatives) carries information, like entity type. EntityConverter needs to figure out the entity type. The entity plugin simply knows it via its own id.
  5. Speaking of EntityConverter, it contains one-off, entity specific code to handle the entity type from the route. This is moved into the plugin manager and now any plugin id can contain any slug which is replaced by a scalar. There's no need for the plugins to handle this.

Also note that some refactoring could make this even less code because the three EntityDerivers in core (HEAD has two, this is the third) contains a lot of common code.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented

I am getting a notice about auto_placeholder_conditions.

This is probably because you're still using an outdated services.yml file. Try a truly fresh reinstall of D8, with that file deleted. (You should've been seeing this notice for weeks if this suspicion is correct.)

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented

@Wim Leers I just got that same bunch of notices about auto_placeholder_conditions. And you are totally right about my services.yml being out of date. Thanks!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Component: base system » routing system

Moving to routing

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Closed (won't fix)