Updated: Comment #2

Problem/Motivation

The content of $module.routing.yml is has a high WTF/line measure since it tries to use a raw symfony format for all the keys and forces us to use a leading "_" for no reason and to group or name keys according to the pure symfony parsing.

Among the worse parts is trying to use a non-permission access check. Even with permissions I have to remember it's under "requirements" not "access".

Proposed resolution

Provide an alternate parsing/translation of Drupal-specific keys that make it delightful to accomplish common developer tasks in terms of defning how the route content, access, etc are handled.

node.add_page:
  path: '/node/add'
  defaults:
    _title: 'Add page'
    _content: '\Drupal\node\Controller\NodeController::addPage'
  requirements:
    _permission: 'administer content types'
    _node_add_access: 'node'
node.revision_show:
  path: '/node/{node}/revisions/{node_revision}/view'
  defaults:
    _title: 'Revisions'
    _content: '\Drupal\node\Controller\NodeController::revisionShow'
  requirements:
    _access_node_revision: 'view'

Could be something more like: this, where e.g. access services are named directly rather than having to grep for the tag and matching that to a service.

node.add_page:
  path: '/node/add'
  properties:
    title: 'Add page'
    content: '\Drupal\node\Controller\NodeController::addPage'
    access:
        - 'administer content types'
        - access_check.node.add: 'node'
node.revision_show:
  path: '/node/{node}/revisions/{node_revision}/view'
  properties:
    title: 'Revisions'
    content: '\Drupal\node\Controller\NodeController::revisionShow'
    access:
       - access_check.node.revision: 'view'

Remaining tasks

Define the desired format
Make sure we can parse either this Druaplized version or the exisintg raw symfony format

User interface changes

none

API changes

None- should still be compatible with existing API

https://drupal.org/community-initiatives/drupal-core/d8dx

Comments

alexpott’s picture

Title: Make YAML registration of routes have delightful a DX » Make YAML registration of routes have a delightful DX
Issue tags: +DX (Developer Experience), +d8dx, +WSSCI Conversion

Tagging

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

This for sure makes it way easier to learn what the different entries actually mean, especially removing the magic underscores.

Not sure whether properties is a good name. Another short idea: Move access one level up.
If we decide to basically hide symfony from people the question is whether telling people that learning Drupal 8 will allow them to apply the knowledge to other systems is still true.

pwolanin’s picture

@dawehner - yeah, this was a really quick first pass. However, I was trying to suggest that we need to avoid potential conflict with symfony keys that might happen if we put them all at the top level.

I don't think most module developers also aspire to be symfony routing experts, and the thought would be to leave the current parsing in place, as an "expert" mode.

sun’s picture

node.create.select:
  path: '/node/add'
  controller: '\Drupal\node\Controller\NodeController::selectTypeToCreate'
  title: 'Choose content type'
  access:
    user_permission: 'administer content types'
    '\Drupal\node\Access\NodeAddAccessCheck':
      entity_type: 'node'

node.create:
  path: '/node/add/{node_type}'
  controller: '\Drupal\node\Controller\NodeController::create'
  access:
    entity:
      type: 'node'
      bundle: '{node_type}'
      operation: 'create'

node.version.view:
  path: '/node/{node}/revisions/{node_revision}/view'
  controller: '\Drupal\node\Controller\NodeController::viewVersion'
  title: 'Version'
  access:
    entity:
      type: 'node'
      id: '{node}'
      version: '{node_revision}'
      operation: 'view'

Remarks:

  1. Massively simplify the >80% use-case: "path" + "controller" + "access".
  2. Built-in support for entity access/operations (80% use-case).
  3. Built-in support for user permissions, but clarify that the concept of "permission" actually belongs to User module.
  4. Wherever custom handlers are needed, specify the FQCN directly (instead of an indirection).
  5. Wherever parameters/arguments are needed/passed, consistently specify them as a hashmap.
  6. "title" conceptually feels like an alien in routing definitions. The fact that it cannot be specified for some routes (with dynamic titles) makes it even more inconsistent. It also suddenly requires us to parse the routing files for translatable strings? Lastly, even the titles in this small sample set were misleading/wrong.
dawehner’s picture

Just linking to the original title issue to explain why titles on routes are a thing which are needed.

https://drupal.org/node/2032535

webchick’s picture

#4 actually looks like a pretty promising start to me.

fubhy’s picture

#4 actually looks like a pretty promising start to me.

Except that the "access" part is short-sighted and won't work like that. The way we currently use it is short sighted too. That reminds me... I gotta open an issue for that. It's currently impossible to use entity access checks more than once on a single route... You can't currently check access for two entities in the route argument simultaneously. That's a limitation we will eventually run into. Access checks should be declared as lists or generally support lists internally. I guess I'd prefer the first option (making access checks a list in general).

Crell’s picture

Issues with #4:

1) Any format we use that's not consistent with Symfony's means more things we need to document ourselves. We won't be able to rely on Symfony documentation (and it's another speed bump for devs moving between Drupal and Symfony.)

2) By design, there's a lot more than just "controller" that can be specified. Currently we have _controller, _content, _form, _entity_view, _entity_form, and probably one or two others I'm forgetting. Supporting multiple varieties of "thing that handles the route" is a feature we cannot lose.

3) Title also has title callback.

4) As fubhy notes, access control is a bit more involved than that.

5) We are using the "requirements" parameters already for their regex filtering. We have to continue supporting that, even if we move our access check stuff out of that block.

6) There's also method / requirements: _method, format / requirements: _format, scheme, and domain restrictions we currently support. Those cannot be lost either just because they're not the common case. (Symfony has most of those start as _-prefixed requirements, and some eventually migrate to top-level items with BC support. Just as both path and pattern are currently supported.)

7) We still need to support "defaults", as that's a rather critical feature. You can't have optional parameters otherwise.

8) Just a note, even if we do change the YAML format we cannot really change the Route object structure. That means anyone making a dynamic route will be exposed to the "native" structure of the object, which maps 1:1 to the current YAML file. That is, defining routes dynamically will be *completely different* in terms of what values you need to set than defining them in YAML. That would be a major WTF/DX Fail.

damiankloip’s picture

Yes, #8.8 is a really good point. So route objects will still need options, defaults, and requirements. So they would still have to fit into one of those anyway. Which it currently does.

Otherwise, E.g. the (strange) access array above would still live in requirements? under an access key or something? This will make it much more difficult to find anything, plus we'll just get this mangled up data.

damiankloip’s picture

Issue summary: View changes

update summary

tim.plunkett’s picture

Title: Make YAML registration of routes have a delightful DX » Consider subclassing Route to simplify route definitions
Issue summary: View changes

The structure of *.routing.yml maps 1:1 with the constructor of the \Symfony\Component\Routing\Route class.
Here is the code that uses the parsed YAML:

foreach ($routes as $name => $route_info) {
  $route_info += array(
    'defaults' => array(),
    'requirements' => array(),
    'options' => array(),
  );

  $route = new Route($route_info['path'], $route_info['defaults'], $route_info['requirements'], $route_info['options']);
  $collection->add($name, $route);
}

In #2145041: Allow dynamic routes to be defined via a callback we just worked to increase the similarity between more advanced routing needs and static YAML files.
For example, the current SearchPluginRoutes does:

foreach ($this->searchManager->getActiveDefinitions() as $plugin_id => $search_info) {
  $routes["search.view_$plugin_id"] = new Route(
    'search/' . $search_info['path'] . '/{keys}',
    array(
      '_content' => 'Drupal\search\Controller\SearchController::view',
      '_title' => $search_info['title'],
      'plugin_id' => $plugin_id,
      'keys' => '',
    ),
    array(
      'keys' => '.+',
      '_search_plugin_view_access' => $plugin_id,
      '_permission' => 'search content',
    )
  );
}
return $routes;

If we change one, we need to change the other. Dealing with this data in a different form will be very confusing.

dawehner’s picture

If we change that here we also run into the problem that everything internal would be different as well. There are many places like Access checkers,
breadcrumb builders, theme negotiators where we deal with potential route objects. Yes these examples are drupal internal, but at some point we basically move away 100% from the architecture of symfony + CMF, which makes us less interobable and people have to understand kind of both syntaxes.

sun’s picture

Title: Consider subclassing Route to simplify route definitions » Make routing.yml files have a delightful DX

Hm. That issue title change was a bit premature. There's not really a reason for having to override Symfony's Route object with a custom one. The schema, structure, and format of route declarations in routing.yml files is decoupled from how the data is parsed and how the parsed data is being processed. There's no technical reason for replacing the Route object with a custom one.

Likewise, I do not think that anyone ever proposed that it should not be possible to use Symfony's native declaration syntax for routes. Yes, that is well-documented and yes, Symfony developers will be more familiar with it. There's absolutely no reason for why you shouldn't be able to use it. I do not think that anyone disrespects the point that is being made. And there is no reason to think in black and white.

But at the same time, Drupal is not a framework, but a product that uses a framework. A product always comes with vendor-specific things to learn, which have been optimized for the product's use-case. The underlying framework still supports the original framework concepts as-is. But there's no reason to cripple the common product experience for the ordinary and primary product consumers, just because the underlying framework is more complex and abstract (as it cannot make any assumptions).

Let's face reality: The vast majority of modules integrate with Drupal\Core\Entity, Drupal\Core\Form, and Drupal\Core\Access.

Because you see "Core" in all of that, and not "Component", all of these concepts are specific to Drupal. As a consequence, the implementations are specific to Drupal, too. Within these implementations, we're working very hard to improve the DX so as to make their usage as simple as possible.

But yet, when it comes to registering these implementations to be accessible on certain routes, the DX ball is suddenly dropped, and you're forced to learn how the underlying, use-case-agnostic framework wants the data to be declared. Not only the declarative syntax is immensely abstract, but on top, the entire nomenclature being used does not map to any of the concepts that you're familiar with (in your actual code/implementation).

As a product, we can optimize the declarative syntax for our concrete use-case in order to improve DX for our primary consumers, while retaining support for the original syntax in parallel. Let's stop thinking in black and white, please.

tim.plunkett’s picture

Because you see "Core" in all of that, and not "Component", all of these concepts are specific to Drupal. As a consequence, the implementations are specific to Drupal, too. Within these implementations, we're working very hard to improve the DX so as to make their usage as simple as possible.

That sounds more like an argument for Drupal\Core\Routing\Route than anything I wrote.

Not getting into an edit war just yet, but I don't think we can change the routing YAML without fixing the route callbacks and alters.

sun’s picture

@tim.plunkett: Could you elaborate more (as much as possible) on these two sentences? It sounds like you're having some more concrete concerns in mind, but the rest of us (1) doesn't know what those are, (2) why they are important, and (3) whether they actually are as important as you see and interpret them. That would be terrific. :)

tim.plunkett’s picture

Here is some D7 code:

function image_menu() {
  $directory_path = file_stream_wrapper_get_instance_by_scheme('public')->getDirectoryPath();
  $items[$directory_path . '/styles/%image_style'] = array(
    'title' => 'Generate image style',
    'page callback' => 'image_style_deliver',
    'page arguments' => array(count(explode('/', $directory_path)) + 1),
    'access callback' => TRUE,
    'type' => MENU_CALLBACK,
  );
  $items['system/files/styles/%image_style'] = array(
    'title' => 'Generate image style',
    'page callback' => 'image_style_deliver',
    'page arguments' => array(3),
    'access callback' => TRUE,
    'type' => MENU_CALLBACK,
  );
  // Other items here.
  return $items;
}

And here is some D8 code:

image.style_private:
  path: '/system/files/styles/{image_style}/{scheme}'
  defaults:
    _controller:  '\Drupal\image\Controller\ImageStyleDownloadController::deliver'
  requirements:
    _access: 'TRUE'

route_callbacks:
  - '\Drupal\image\Routing\ImageStyleRoutes::routes'
namespace Drupal\image\Routing;
use Symfony\Component\Routing\Route;
class ImageStyleRoutes {
  public function routes() {
    $routes = array();
    $directory_path = file_stream_wrapper_get_instance_by_scheme('public')->getDirectoryPath();
    $routes['image.style_public'] = new Route(
      '/' . $directory_path . '/styles/{image_style}/{scheme}',
      array(
        '_controller' => 'Drupal\image\Controller\ImageStyleDownloadController::deliver',
      ),
      array(
        '_access' => 'TRUE',
      )
    );
    return $routes;
  }
}

So the *.routing.yml has path, defaults, requirements, and options. And it points to this callback, which creates a new Route object, which has the parameters of path, defaults, requirements, and options.

As you can see from the D7 example, this code did not need to be separate. The same goes for if/foreach D7 code.

While knowing that we couldn't have the same cohesiveness as D7, we still tried to keep these two as similar as possible.

If you propose to change the keys of the *.routing.yml, any semblance of similarity will be gone. In order to write some routes, they'll need to learn our proprietary Drupal syntax, and all documentation they find about Symfony routes will be wrong. Until they need to write that one conditional or loop, in which case they would nee to learn the Symfony syntax after all.

That is unacceptable.

Either we keep with the current syntax (won't fix), or we have to change the class we use (retitle the issue).

dawehner’s picture

If you propose to change the keys of the *.routing.yml, any semblance of similarity will be gone. In order to write some routes, they'll need to learn our proprietary Drupal syntax, and all documentation they find about Symfony routes will be wrong. Until they need to write that one conditional or loop, in which case they would nee to learn the Symfony syntax after all.

Even if we also change the syntax in our dynamic route subscribers at some point we basically have to decide between getting of the island and accept what other people had in mind vs. building a solution which works at least for the usecase of core as good as possible.

There are a lot of good symfony docs/people which drupal can profit on, but yeah this is a way more meta question which has to be answered as well.

As a product, we can optimize the declarative syntax for our concrete use-case in order to improve DX for our primary consumers, while retaining support for the original syntax in parallel. Let's stop thinking in black and white, please.

Let's assume we have optimized for the solution of core, people still have to learn the way how it internally looks like for probably 50% or more usecases. Writing access checkers/breadcrumb builders or even accessing the route object in your controller is quite a common usecase if you look at drupal itself. Now you end up with two syntaxes which are hard to map for the user.

Instead of babysitting people like that we could also think of improving tools for the users. Create good scaffolding tools, talk with IDE people
and finally provide easy c&p-able examples.

webchick’s picture

Well, I think the general idea would be that all of those would be written in a similar way (aka, with definition language geared towards Drupal's specific use vs. Symfony's abstraction/genericism) but it's definitely a fair point that such a decision has far-reaching consequences.

chx’s picture

Wim Leers’s picture

tim.plunkett’s picture

Status: Active » Closed (won't fix)

The ship has sailed for D8, and I think we made the right choice to keep Symfony's structure. This could be reopened for D9 if desired.