Problem/Motivation

The new matcher/router system went in (yay!) but with only a stopgap API for defining routes, hook_route_info(). That is the one ugly part of that system, as it's the only part not testable and not object-oriented. Some have suggested that defining Route objects inline is less DX-friendly than route arrays, although that point has been debated. We Can Do Better(tm).

Proposed resolution

There are a couple of possible solutions on the table, with assorted pros and cons:

1) CMI files. Symfony uses YAML files to define routes, which is quite successful and nicely self documenting. This also allows routes to be treated as configuration, not code, which is more accurate and more flexible. The format of the file would be immediately familiar to any Symfony, ezPublish, or PHPBB developer. There would be no need for an alter hook, just config CRUD. It also means that, once we have a generator, we could allow the paths of routes to be changed very easily in config without obscenely complex gymnastics (aka, why Panels/Page Manager for Drupal 6 took 8 months to come out). See comment #6 for a patch. The downside of this option is dynamic routes (eg, those that right now would be defined with a foreach() loop in hook_menu()). Those would be very difficult to implement. The argument could be made that those are a code smell, though. See #57 for a discussion of dynamic routes.

2) Leave it as an info hook. I (Crell) personally don't like this approach as it's introducing a legacy Drupalism into an otherwise all new-fangled system. That's going to be very jarring. It would also be an info hook returning an object of objects, not an array of doom, which would be inconsistent with our other info hooks. However, we do know it handles dynamic routes reasonably well.

3) CMI files with "Decorators". Neclimdul suggested this to me earlier today. The idea would be to allow routes to declare some callback (which would have to be a class or service, not a function) that would take the CMI file and spawn multiple routes out of it, based on some rule. This is essentially the same as decorators in the Plugin system. Pros here are that it's similar to some other system, and still lets us use YAML files 95% of the time. Downside, it's an untested Drupalism so we don't know how viable or ugly it would be in practice.

4) A special method on bundles. Effectively the same idea as an info hook, but a specially named method on a module's bundle class rather than a specially named function. Pros: Still code definition, should be straightforward to do dynamic routes (although possibly that would break injection), lazy-loads cleanly. Cons: Still code definition. Drupalism. Not sure how we'd do the equivalent of an alter hook.

5) An event for collecting routes onto a RouteCollection. Basically the same idea as a hook, but done new-fancy style (and therefore easier to inject/test). Downsides: Still in code, unclear how we'd do an alter, and arguably a bastardization of events (Drupalism). Also, I don't know how this would support the very nice "rebuild one module at a time to avoid blowing up memory" capability that the new matcher dumper has.

It's also been noted that the above are not all mutually exclusive; that is, there are probably ways to offer both code-based and YAML-based route definitions, if we're willing to accept the potential for confusion (and two different ways to go about editing them).

Remaining tasks

Decide what to do, then do it.
#1840914: Convert routes to CMI

User interface changes

None at this point.

API changes

That's what we're trying to decide! :-)

Original report by [username]

From #1606794-135: Implement new routing system:

I want to change hook_route_info() before we ship. For now, though, I'm not sure what the better alternative is so I've provided minimal documentation and marked it as just a stopgap for now.

From #1606794-126: Implement new routing system:

It feels, well, off to have this one legacy definition hook in a system that is otherwise 100% Symfony-based OO

From nielsvm's comment on the change notice:

I'm certainly not saying that adding Route objects to a RouteCollection object is a hard thing or that our developing audience can't adapt to that, what I do mean is that code readability drastically drops in a key area of every Drupal module that would declare paths. What about wrapping around this lower level API by providing a hook that still looks similar to hook_menu and does allow to pass everything down the chain so that Symfony's routing API is fully leveraged and that module organisation remains clean and readable?

This is an area of Drupal that is simultaneously low-level (in the sense of integrating into Symfony's basic HTTP handling) and that many contrib module developers will need to deal with. We need to come up with something that makes sense architecturally and that Drupal's broad community of developers finds appealing to work with.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

My knee-jerk thought was to move hook_route_info to a bundle method. That may or may not be good, but it's a thought. :-)

Symfony full stack uses a YAML file provided by a bundle so you never(?) define routes in code yourself. You can also annotate a controller. I don't know the full details of either, but I am against annotations on the controller on the grounds that it tightly couples a controller to a path, which we don't want to do.

geerlingguy’s picture

And my comment (regarding, specifically, the change notice):

Could you just show a basic example of how, exactly, the old hook_menu() example would be translated? The new example seemed a little opaque to me until I read through it a couple times...

Raising the bar to basic module development (like defining custom pages/paths) means there may be fewer beginner-level PHP developers developing modules. I know that three years ago, I would've looked at the first example (hook_menu()) and been able to wrap my mind around it without much in the way of documentation. For the second example, even though I know a lot more about OO programming than I did three years ago, I still have to carefully read through the code comments to figure out exactly what's happening.

tim.plunkett’s picture

This

What about wrapping around this lower level API by providing a hook that still looks similar to hook_menu and does allow to pass everything down the chain so that Symfony's routing API is fully leveraged and that module organisation remains clean and readable?

would really be great for D8.
Along with hook_form_alter(), hook_menu() is one of the most basic and powerful hooks, and I think hook_route_info() is too much too soon for D8.

Providing bundles is clearly the way of the future, and should be well documented and encouraged. But there needs to be something vaguely familiar.

effulgentsia’s picture

Status: Active » Needs review
FileSize
7.68 KB

I am against annotations on the controller on the grounds that it tightly couples a controller to a path

I disagree. Annotations would only tightly couple if we provided no alter hook / event listener to override them. If we do provide alterability, then annotations can be seen as providing a default, which I think would be quite sane.

Symfony full stack uses a YAML file

Ooh. I like that more than annotations for this purpose. In fact, I really like the idea of routes being full-fledged Drupal config items. Here's a patch based on that. I tried to model the YML filename and content structure on http://symfony.com/doc/2.0/book/routing.html, except I didn't implement the shorthand naming pattern for controllers. That could potentially be a follow up.

@geerlingguy: any thoughts on whether the key/values in the YML file are easy to understand? We technically can make changes to it, though there's something to be said for sticking with a structure already well documented by Symfony.

Also, with this patch we lose the nice docs in system.api.php. Do we have a general issue somewhere for how to document keys within config objects? We can add comments within a YML file, but if all *.routing.yml files need to follow a set structure, where do we document that structure (even if all we do is link to the Symfony docs page, though we still need to explain _content, since that's a Drupalism)?

Status: Needs review » Needs work

The last submitted patch, routing-as-config.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
8.25 KB
geerlingguy’s picture

@efflugentsia - that looks way simpler (conceptually), especially since, I presume, I would be able to define the routes in a YAML config file, then implement the appropriate classes/methods in code (right?). Effectively, the definition of paths like we would do in hook_menu() could just be done in a YAML file.

Routes as config++

Crell’s picture

Status: Needs review » Active
Issue tags: +WSCCI, +CMI, +YAML

LOL.

I said way back at DrupalCon Denver that I preferred to use CMI for routes, precisely because I saw them as config, not as code. At the time CMI wasn't ready yet for such things, so we went with hook_route_info() precisely because it was the most Drupalish alternative, and so would be *more* familiar to current Drupal devs.

So I'm actually totally down with switching to CMI files, in part because it means you can blast away the entire set of routes in the entire system via config without hacking code. (No need for an alter hook, either.) I love that idea, so I'm not the one you'd have to convince here. I was convinced 8 months ago. :-)

The one caveat I'd put there is that we should absolutely stick as closely as possible to the Symfony format for routes. We're not doing the magic naming that Symfony does, and we allow for _content instead of _controller, so the syntax isn't exactly identical, but we should be as close to exactly identical as we can if we go this route (no pun intended).

We need to figure out how to document events still, too, in addition to YAML. That does not get any better or worse this way, it's the same amount of work to document.

yched’s picture

One problem I see is that modules making changes in their hook_menu beteeen point releases is a super common case.

AFAIK, we have no real mechanism to update the set of default module config between minor releases of a module. If version 1.2 adds new routes or modifies existing ones, it won't be updated in the active store unless the module provides an explicit update_N hook.

cweagans’s picture

Personally, I'm okay with requiring modules to supply an update hook to update their routes.

Sylvain Lecoy’s picture

Agree with cweagans, its fairly common to use field_cron(), system_list_reset(), registry_update() and friends in update hooks, so why not an update route functionality.

catch’s picture

Config for routes sounds sensible. However we'll presumably still have to compile that (into the router table) to do all the matching stuff. I guess that's probably OK though.

The problem with update hooks is you never know whether someone changed the router item themselves and you're going to overwrite their changes, or whether it's just updating something that was never changed. There's open VDC issues to resolve some of these things in CMI and it's not really worse than a module updating a router item that's also been altered.

sdboyer’s picture

i'm mostly +1 to routes as config, but if we need to provide an update hook, how does that work with modules/systems that need to dynamically generate routes?

@catch - yep, they'll definitely have to be compiled into a more central location for the purposes of doing real routing/matching. i think that's why it's fine to express them in whatever format we want, and YML's got lots of advantages.

effulgentsia’s picture

Status: Active » Needs review

Was #8's status change intentional? In case not, setting back to needs review.

I would be able to define the routes in a YAML config file, then implement the appropriate classes/methods in code (right?).

Yes. If you apply the patch to HEAD and then look at the router_test module, you'll see exactly that.

The one caveat I'd put there is that we should absolutely stick as closely as possible to the Symfony format for routes.

#6 does that with the following exceptions:
- '_content' is a Drupalism that has no parallel in Symfony.
- '_controller' is a direct PHP callable, not Symfony's shorthand.
- 'pattern' doesn't start with a leading "/". Maybe it should? But it doesn't in HEAD, so maybe that's best left to a follow up discussion.
- value strings are quoted. In http://symfony.com/doc/2.0/book/routing.html, they're bare. I'm not fully clear on when YAML allows bare string values and when it requires quotes.
- The 'defaults' array is written out in long form rather than inlined with {}.

we'll presumably still have to compile that (into the router table) to do all the matching stuff

Yep. HEAD already does that, and #6 doesn't change that. Will this be our first occurrence of config values cached somewhere other than {cache_config}, and are there problems with that we need to deal with here or in a follow up?

if we need to provide an update hook, how does that work with modules/systems that need to dynamically generate routes?

That's a problem we need to solve for dynamically generated / altered Fields, Views, and probably other use cases too. Though elsewhere, we've been using or planning to use ConfigEntity for possibly dynamically generated things. I wonder if we need to use ConfigEntity for routes as well, and if so, whether to do so in this issue or in a follow up once we have some real world routes converted.

sdboyer’s picture

Status: Needs review » Active

That's a problem we need to solve for dynamically generated / altered Fields, Views, and probably other use cases too. Though elsewhere, we've been using or planning to use ConfigEntity for possibly dynamically generated things. I wonder if we need to use ConfigEntity for routes as well, and if so, whether to do so in this issue or in a follow up once we have some real world routes converted.

OK. somewhat more immediately, we need it for blocks, too.

Crell’s picture

Status: Active » Needs review

Yeah, accidental, and probably for sdboyer, too. :-)

pattern: I've been using /leading/slash for anything touching the new routing system, since that's what Symfony does and I don't want to have too many places where the slash confuses things. Long run I think we have to mass-migrate over to leading /. So let's do that here.

Quoted values and short-hand arrays: I think those are just YAML options/idiosyncrasies, not something that has logical impact. As long as it doesn't change semantic meaning I don't have a super strong preference either way.

It sounds like the module-update problem is already existing, and we know we need a solution. I therefore don't think that's a blocker here, because we are going to have to solve it for blocks, Views, image styles, and anything else that we're moving to CMI. Whatever solution we come up with there should work here just as well, so I'm not going to spend too much time on it in this issue directly; we can let it inherit whatever other solution we develop.

Aside from the leading /, hell, I'd say #6 is an answer I'm willing to RTBC. I won't though to get more feedback first.

geerlingguy’s picture

Might we want to mention this via g.d.o or a front page post, or at least try to get a few more eyeballs here, since this probably affects 80% of contrib modules, and 99% of Drupal sites that are more extensive than a plug-n-play site?

Large swaths of books like 'Pro Drupal X Development' and 'Definitive Guide to Drupal X' are being negated by some of these sweeping changes, and I think it's important to make sure people feel there is still continuity between Drupal 7 and Drupal 8 for the basics: nodes(entities), blocks(and now layouts), menu paths(now config router entries), etc...

Change is good, but I think it's important to make sure people realize that it's a change to make things better, and not a change for 'code/objective purity' that leaves them in the dust in terms of their built-up knowledge of Drupal :)

[Edit: I'm not suggesting that there is not continuity, or that any of these changes make things worse. I'm just suggesting we try to emphasize the continuity and the positive aspects of the changes in change notices and summaries... and try to write for people who are not involved in the issues themselves.].

Crell’s picture

I posted a /core group announcement with a link here about an hour ago: http://groups.drupal.org/node/259113

xjm’s picture

Large swaths of books like 'Pro Drupal X Development' and 'Definitive Guide to Drupal X' are being negated by some of these sweeping changes, and I think it's important to make sure people feel there is still continuity between Drupal 7 and Drupal 8 for the basics: nodes(entities), blocks(and now layouts), menu paths(now config router entries), etc...

This is irrelevant, off-topic, and partially false. node.module still provides pretty much exactly the same API it did in Drupal 7, and nodes have been entities since 7.0 was released. In fact, many, many things still provide the same APIs they did in D7, even if the internals have been rearchitected.

Saying that a book about Drupal 7 is not accurate for Drupal 8 is like saying yesterday's weather forecast isn't accurate for today. Of course it isn't.

If you're concerned about the DX for this particular issue, discuss that. If you're concerned about changes that break BC not being well-documented, help improve the text of those change notifications, but that is outside the scope of this issue.

xjm’s picture

And, tagging for a better summary, because @geerlingguy is right in that this issue could use a better one. :)

chx’s picture

My first reaction to the route change notice was a big OMGWTFBBQ -- that's poor DX if I ever saw one for sure. Now, this one is a helluva lot better but

  pattern: 'router_test/test1'
  defaults:
    _controller: '\Drupal\router_test\TestControllers::test1'

router_test_2:
  pattern: 'router_test/test2'
  defaults:
    _content: '\Drupal\router_test\TestControllers::test2'

Why do I need _controller, why is that not controller? Why defaults? The whole syntax here looks as un-intuitive as it gets.

chx’s picture

sorry x-post.

Crell’s picture

Defaults get merged with values from the pattern placeholders to form the request attributes. Underscore-prefixed values are to avoid namespace collisions. _controller in particular is what the ControllerResolver class looks for. It's no better or worse to document than "how do I know I need an array element named 'page callback'?"

The other advantage here, per that, is that using Symfony-based YAML files is a non-Drupalism. It's a Symfonyism, sure, but that makes it a Symfony-and-Drupal-and-anyone-else-using-Symfony-ism. :-) (So PHPBB, ezPublish, Drupal, and Symfony would all be using essentially the same Route syntax; which, when I say it that way, makes me all warm and fuzzy inside.)

ygerasimov’s picture

Would it be possible to have all configurations that routing is based on? For example can we specify HTTP method for the route?

effulgentsia’s picture

Yes. Anything that Drupal can react to in 'defaults' or 'requirements' can be specified in the YAML. An HTTP method example is covered in http://symfony.com/doc/2.0/book/routing.html#adding-http-method-requirem.... We'll need to add more complete docs for whatever other features Drupal's routing system ends up supporting.

Crell’s picture

ygerasimov: Yes, that is already supported via the _method "requirement" property. See "Defining routes" on:

http://symfony.com/doc/current/components/routing.html

(I don't recall off hand if we're checking for scheme. If not, we should.)

Owen Barton’s picture

I had a similar reaction to chx to the underscore prefixes - traditionally underscore prefixes in function names mean "private, do not use (unless you are writing the API)". Obviously the meaning and purpose here is quite different (more like the "#" prefixes in FAPI), but it does conflict somewhat with the traditional semantics.

Other than that, bundling in yaml sounds great :)

pounard’s picture

+1 to Synfonism in routing!

Crell’s picture

As I discussed with chx yesterday in IRC, the _ for default/requirement keys should, I think, be best interpreted as "magically important", much like __ prefixes in PHP. That's not quite the same as pseudo-private, but it's kinda similar. :-) It's also an inherited Symfonyism that already exists, and we'd have to override more code to NOT have, so I'm not worried about it.

sun’s picture

Issue tags: -CMI +Configuration system

Can someone explain to me why everyone in here thinks that module-defined data is configuration?

It's not like you can remove the configuration for route /node/{node} and expect your site to still work, or is it?

I can perfectly see how routes are configuration in Symfony, because you generally don't have any predefined assumptions on anything in a PHP framework and you're expected to define and configure all the routes on your own (but also do the heavy plumbing to make your site/UI/links/forms actually use the custom routes you defined).

Thus, I could get behind the idea of having "custom routes" in configuration... but I'm afraid, except of a couple of rambling comments by @Crell on one of the URL alias issues, I haven't heard nor seen anyone actively working towards an architectural design concept for how that could actually work out in the real world yet. And next to the fundamental question of whether custom routes are a good idea in the first place, given the remaining time-frame, I doubt that the idea of arbitrary custom routes will be possible for D8 - we have far more pressing issues in the pipeline.

The update/maintenance problem definitely shouldn't be put simply aside by saying that other functionality would like to have a similar (aka. "nice-to-have") solution. For one, I think that the desired feature elsewhere is vastly different in terms of architecture and how rigid defaults/updates/reverts/orphans have to be applied (also: no tree involved). And second, it is not guaranteed that we'll have time to make that stuff happen for D8.

Lastly, routes in Drupal are always bound to code. Why should I have to stage configuration in order to "enable" the routes that actual module functionality depends on? And do we really want people to be able to edit the access definitions and everything else that is defined for routes? Do you think that's a good idea in terms of reliability and security?

I'm a bit confused by this proposal, and would love to see some clarifications.

sun’s picture

Title: Figure out how to replace and/or improve DX of hook_route_info() » Replace or improve DX of hook_route_info()

Improving issue title for mobile sanity.

frob’s picture

I am a little confused as to where this issue is going. I came looking to see if or how the DX was improving for hook_route_info (I got here from this comment).

I was really hoping that hook_menu would simply remain as a wrapper for simple routes that do not require all the power of the new routing system.

Crell’s picture

fronb: No, the systems are too different, and hook_menu() is already a many-headed hydra of a hook. We want those capabilities pulled apart. This issue is about making the new system as easy as possible while retaining all of the power we want from it.

To sun: Routes are configuration because the way you bind functionality to a URL is, or rather should be, a site-specific decision. Sure, most sites will use the defaults, but they should not be forced to. Routes as configuration implies a deeper separation between components, which is a good thing.

Dynamic routes are the only place where I think we run into an issue. I'm not sure yet how to resolve that.

Anonymous’s picture

late to the party here, but i agree with sun in #30 - routes as CMI doesn't look right to me.

really interested to hear responses to the issues sun raises.

Sylvain Lecoy’s picture

If you guys are speaking about dynamic routes as it is the case for node based entites (e.g. added by Field API and entity system), then I think we can still use the RouteCollection object.

In Parsley, a Flex framework, they have 3 ways of defining configuration, by XML, by MXML, or by a DSL (Domain Specific Language). It is very common among developpers to write different parts of the configuration on static files, and when you need more fine-grained control use the DSL.

The problem here is the same, if you need routes for user/login, user/logout then it is simple routes that you can list on .yml files. When you need dunamic routes, such as content_type/page/edit, content_type/article/edit, etc. then you use the DSL a.k.a the API: http://symfony.com/doc/current/components/routing.html.

I see really no problem about that, and specially since the component both support DSL routes and loaded from a file routes.

effulgentsia’s picture

Can someone explain to me why everyone in here thinks that module-defined data is configuration?

For all configuration, modules define a default, so this isn't really the question: the question is whether routes should be (user) configurable. An example use-case is whether a particular form belongs in admin/config or admin/structure. For some modules, this is frequently debated, and some sites want to do the opposite of whatever the module maintainer ultimately decides. For example, see #949996: Move admin/config/media/file-types to admin/structure/file-types.

It's not like you can remove the configuration for route /node/{node} and expect your site to still work, or is it?

If you do this, site visitors will get 404 when visiting node URLs. Similarly, if you remove "access content" permission from all roles, site visitors will get 403 when visiting node URLs. What makes the first more broken than the second?

And do we really want people to be able to edit the access definitions and everything else that is defined for routes? Do you think that's a good idea in terms of reliability and security?

One aspect of security is whether we're concerned about people getting access to pushing config files independent of any UI. If someone gets this access, they can push permissions to the anonymous role, so I don't think route altering presents any new risk. The other aspect of reliability and security is what kind of UI to expose for editing routes, and how to communicate what the impact of making changes are. Even if we make routes into config, I don't think we should create a UI for it in core (other than whatever limited page manager UI ends up getting made as part of the Blocks and Layouts initiative), and instead see what contrib comes up with.

The points raised about complications with dynamic routes, including #1783964: Allow entity types to provide menu items, workflow issues about module updates to sites with customized config, and whether there's other problems that makes this more work than worthwhile, are all good ones to discuss. If we decide to not turn routes into config, would we still want to use YML files, just not in the config directory? If so, should we turn this issue into that, and then have a follow up for the CMI-related discussions?

Sylvain Lecoy’s picture

How to _alter yml defined routes ? With the same DSL.

It makes sense because the population of users likely to use simple routes or overriding them by admin UI will seldomly need complicated routes. The second part of users which need complex and/or dynamic routing are more likely to be developper, so they need the full flexibility of the API.

Is it technically possible to alter routes in Symfony component?

yched’s picture

What worries me is module dev experience. Update defaults + live config & reimport on each adjustment of what currently lies in hook_menu ?

frob’s picture

Sylvain Lecoy, Big ++ for making sure we can include an equivalent _alter hook.

So to sum up this is what I see here. (Please let me know if I am confused about this.) It looks like a YAML and/or a code definition for routes will be allowed.

If that is the case then I have a question.
For code, why have a hook at all if an object is required? Could it not all be contained within the object? splitting this seems like really bad DX. I am suggesting something like this.

class TestControllers extends ContainerAware {
  public function route_info() {
    $route = // put route info stuff here

    $this->route_info = $route;
  }

  public function test1() {
    return new Response('test1');
  }

  public function test2() {
    return "test2";
  }

  public function test3($value) {
    return $value;
  }

  public function test4($value) {
    return $value;
  }
}

Please forgive me if the syntax is shoddy, I just did this off the top of my head.

Crell’s picture

We haven't settled on YAML vs. code yet, although right now it seems we're leaning YAML if we can figure out dynamic routes.

Putting route definitions into a method of controller classes is a non-starter. It means to discover routes, we'd need to scan all classes in the system for a routeInfo() method (or check for an interface that has that method). That's going to be prohibitively expensive.

Symfony fullstack allows annotation-based route definition on the method, but per comment #1 I'm not in favor of that. Plus, we'd still have the same mass-scan problem unless we require controllers to be in a specific directory/path/namespace. I don't know if we want to require that.

frob’s picture

I would vote for both. YAML is nice (if route alters will be possible), but it would be nicer to be able to have both.

I am not sure as to why having the routes register in the class is such a non-starter. I am assuming that this system registers the route --caching it for later. If this is not the case I see why it would be considered too expensive, just not why it would be more expensive than doing the same thing with a hook.

Crell’s picture

With a hook, we know the pattern to use. function_exists($module . '_router_info'); For an event, things are pre-registered so we already have a built list.

But there's no pre-existing list of "all classes that have a given interface and exist somewhere on disk". You can get that information for classes that are loaded into memory I believe, but with autoloading the vast majority of classes will not be loaded on any given request, by design. So we'd have to do a file system scan a la the Drupal 7 registry or the testing system. I don't know what the memory requirements are for the testing system's scan, but I cannot imagine it's something we'd want in-production sites to be doing.

If routes are defined in CMI, then there would not be an alter hook as we understand it now, IMO. Instead, you'd just do a load on the appropriate CMI object, edit it, and save it again. It's data, not declarative code. For static routes that's fine. For dynamic routes, still TBD.

If someone has a suggestion on how to handle *that* question, that would be awesome. :-) My primary goal is just killing that hook.

tstoeckler’s picture

I hope I don't get my head cut off for this proposal, but I personally think we could simply put the route_info() (or similar) method in the bundle class provided by the module. We load that anyway, and it seems to make sense architecturally. The Bundle class currently exists to register some services in the DIC, and (menu) routes seem to be similarly low-level stuff compared to services.

pounard’s picture

@#43 Proposal makes sense, having optional methods on Bundles sounds fine, something such as SpecificBundle::getRoutes() or whatever else. EDIT: ZF2 seems to be doing that for a lot of configuration related stuff.

Sylvain Lecoy’s picture

I may have missed something, what is the Bundle class ?

I was about to propose a naming convention, e.g. NodeRouteInfo for node, HtmlMailRouteInfo for html_mail, and so on... But if there is a structure already with the bundle class, we just introspect the class by a reflection mechanism when discovery process starts.

Just.. what is this bundle class ?

tstoeckler’s picture

@Sylvain Lecoy: See http://drupal.org/node/1539454

The bundle class was introduced to allow modules to register services in the DIC.

sun’s picture

re #43:
I can't speak for @Crell, but I can only guess that one of the goals is to keep the route definition code separate from the runtime code; i.e., to not have to load dead code on every request when it's not needed.

pounard’s picture

From bundles you can extract data and set it as parameters of the DIC, which then once compiled is pure static PHP information. I don't know if for routes, in our case, this would be a good idea (it mainly depends on the route number) but if this hook keeps a small amount of dynamic routes, this can work.

Sylvain Lecoy’s picture

Re #47 because the bundle would be loaded every times, this woul not be ideal in our case, loading "runtime dead-code" is not a good practise. Then the easy solution would be to have a class dedicated to routes placed in the lib folder or somewhere defined:

  • node: NodeRouterInfo
  • user: UserRouterInfo
  • custom_module: CustomModuleRouterInfo

This would be a possible convention of course.

pounard’s picture

If it's being defined in a .module file, then it's being loaded every time too, so choosing one or the other doesn't really cause any red flag. The idea of the new route info is that if it can be factorized to bare essentials, it would even be better to have those new routes in bundles than in .module files! The idea I think if we care about caching and efficiently to support the same volumetry as we do today but faster is to provide compilation for the DIC and routes.

Config would hurt more because it would be loaded from cache|database|files (choose the right one depending on how is warmed up the system) and it would be probably be split among multiple files (one per module I guess?) so it would be good only if the cache is ultra fast AND if the router load only the right one, but if you're dealing with sub requests, you may end up loading multiple cache entries. In the end, if the code volumetry is the same as actual hook menu, I vote for a code based solution (no more than hook menu in the end) which doesn't necessitate any cache, and where the potentially precomputed stuff ends up right here optimized in the compiled DIC.

From my point of view speaking about DX I think that three of hook_route_info(), Bundle::getRouteInfo() and config file are equivalent in usability, overridability and ease of learn. EDIT: Actually the Bundle::getRouteInfo() might the less easy to override for old school drupalers due to the new DIC mecanism, but that's not a blocker IMO.

Crell’s picture

Yes, minimizing dead code is a goal.

Bundle classes load on every request currently because we are rebuilding the DIC on every request. Once we are able to compile it, that will stop happening.

However, there are other methods on bundles, like boot(), that would get called on every request. There's an issue somewhere where we were discussing whether nor to use that because it would cause bundles to get loaded every request. I don't think we came to a resolution.

Crell’s picture

pounard: Config files after being parsed can be released from memory. Code cannot. That's another of the key advantages of config-defined routes over code-defined. (This is all compile time; For runtime, it's all coming from a DB table anyway.)

pounard’s picture

Yep true. I was doing the comparison with actual hook_menu() implementation. That said, I wouldn't mind a code based menu router since we're already doing it and I'm not sure this is a bottleneck today. Config would be definitely fine too.

webchick’s picture

Status: Needs review » Needs work

Shannon informed me that Crell would like some core committer feedback on this issue. To do that, we need an issue summary that reflects the current proposals on the table and their pros/cons. I would tag this issue but it's already tagged. :)

mitchell’s picture

If cached plugins are available through the Dependency Injector, then I am in favor of dynamic routes through that option (though I think both static and dynamic would be useful). I'm not so familiar, but from what I could tell from Cache API and Plugin Cache, plugins would be bundles in the DI.

If that's then case, then I would also think that plugin interdependencies would help for loading modular data on an as-needed basis, within requests. #1509164: Use Symfony EventDispatcher for hook system and #1764474: Make Cache interface and backends use the DIC seem to cover this, but I cannot totally glean the message from those.

The main reason why I think it would be also be a DX win is that it would make them easy to define and easy to invoke or extend, for example, like these potential plugins' route definition:

One last thing, I would like to request is that routes be able to support any verb for their request methods, instead of only the common ones.

Crell’s picture

Routes already now support any HTTP-legal verb. That's already implemented. I'm not sure I follow the stuff about plugins otherwise. Routes are not plugin objects, and are not going to be. I don't understand the rest of the point you're making.

donquixote’s picture

The dynamic routes problem is still out in the air, and imo its relevance is still underrated in the discussion.

Imo, APIs in Drupal should be designed so that they allow higher-level APIs to be built on top. This requires the lower-level API to allow definitions based on dynamic criteria.
E.g. in Drupal 7, we have Views/Panels and Entity API built on top of hook_menu(), hook_permission() and others. You create a view or a new entity type, and those modules do the hook_menu() and hook_permission() stuff "for free".
In Drupal 8, a successor of hook_menu() could be built on top of the routing API.

This multiple-level API thing is not an edge case, but a recurring pattern in Drupal (*), that we should encourage and make as easy as possible.
It is about routes in this case, but I'd extend this to any API.

yml files (or .info files etc) totally do not support dynamic criteria - so we need something else for dynamic definitions.
With D7-like info hooks such as hook_menu(), hook_permission(), hook_theme() etc, the nice thing was that once you understand how to do static definitions, it is quite obvious how to do dynamic ones.
If we change that to a dual system of yml files vs something else, we lose this advantage. If you want dynamic definitions, you need to learn it all anew. Not nice.

Imo, we should find a solution for dynamic definitions first, and then decide whether to use yml files for static definitions.
For my taste, this can be an info hook.

----------

(*) I think in Symfony this use case is less relevant, because application developers are used to writing a lot of configuration and custom code, instead of just enabling extensions. And there are not as many extensions that interact with each other, or that provide higher-level APIs.

Crell’s picture

Assigned: Unassigned » Dries

I've updated the summary with the options I feel are legitimately on the table. Assigning to Dries to get his input, because we can't let this sit for long.

webchick’s picture

Hm. Isn't it possible to deal with this issue anytime up until code freeze? The issue summary could use an explanation of why it's necessary to make a decision on this right now, as opposed to waiting until later when we have some experience porting things to the new router system under our belt.

webchick’s picture

Status: Needs work » Needs review

But otherwise, seems like a good summary. Thanks! Marking back to needs review.

donquixote’s picture

One option is missing:
If yml files is an option ("1" in issue summary), then why is "array of doom" info hook not an option? PHP nested arrays are equally expressive as yml trees, except that
- php arrays can depend on dynamic criteria.
- yml file would be part of the configuration system, allowing it to be overridden by local configuration.

Also, I would be careful calling things "legacy" before we have an alternative that is clearly superior. We postponed "replace hooks by events" for good reasons.

Obviously, the same array thing could be done as a bundle method, instead of a hook.

---------

Btw, what I don't like about the current hook_route_info()
- It is verbose
- It lets the implementing module deal with implementation details: "new Route" and "new RouteCollection" should be abstracted away. In theory, we should be able to swap the router system, without changing the API.

Dries’s picture

I think my preference would be annotations. I don't understand why that is frowned upon? Have a look at http://www.javahotchocolate.com/tutorials/restful.html and http://jersey.java.net/nonav/documentation/latest/user-guide.html#d4e8 -- could be a source of inspiration.

Crell’s picture

Re #61: We can already swap out the matcher if we want to, which is a good thing. But swapping out all traces of Symfony-based routing is not in scope. We've committed to leveraging Symfony's model. Routes and RouteCollection are just ways of feeding data to it. Making *that* pluggable isn't really effective. Arrays of Doom are off the table because they are less self-documenting than Routes, offer no benefit over declaring Routes in code, and would have to just get converted to Routes anyway, meaning they're slower to compile.

Dries: Annotations on a controller do not work for several reasons:

1) Many of the things we define will not be controllers. Most normal "html page" routes will define a _content callback, not a _controller. So we can't say "find me all controllers", because that won't be all we define.

2) Scanning for any callable that has annotations on it is costly. Plugins get away with it by having extremely tightly controlled namespace rules. We would have to do something similar, and scan \Drupal\$module\Controllers\Foo, and then scan each method individually. That still runs into the "it's not always a controller" problem above.

3) Annotating the controller (or content callback, or any other such thing) tightly couples the controller to a specific path and route. We've spent a long long time decoupling page callbacks from specific paths, and in *most* places have succeeded. Even if we were to offer annotations and then some other method so that you could reuse the controller/content/thing, we'd then need to fully define that other method, and we're right back in the same "2 methods are confusing" problem that a couple of alternatives have.

There's probably a few other reasons I'm forgetting. :-) Basically, Plugin annotations are metadata about what the plugin IS. Route information is configuration about where a controller or other callable should be USED. It's a different sort of information that doesn't make sense to tightly couple.

I know some frameworks offer that approach, but that's because their controllers are, in practice, hand-written per project for one use. In this case, Drupal as a platform/product means that we cannot take that shortcut that a pure framework could.

donquixote’s picture

Arrays of Doom are off the table because they are less self-documenting than Routes, offer no benefit over declaring Routes in code, and would have to just get converted to Routes anyway, meaning they're slower to compile.

The same is true for yml config, isn't it?

pounard’s picture

I have to agree with Crell in #63 about annotations, I don't think this is the best way to go.

I also agree with donquixote in #64, and the choice should not be driven upon performance considerations because it'd end up compiled in the end.
Configuration driven routes will be way slower to compile than PHP based ones (loading config, parsing config, move it into an array, and then do the same stuff that we would do with array). PHP arrays are probably the most performant way to store stuff there is for this use case especially when it gets compiled by an OPCode cache.
Arrays are not always synonym of doom, as long as they are not used as a public API. The real doom of Drupal array's are render arrays, form arrays, element info arrays, ... Regarding DIC configuration arrays, it does not matter because it's not a public API for the module developer, just pure service definition that will never ever be read outside of DIC compilation phase.

donquixote’s picture

#63, I agree with Crell about annotations.

#65 thanks for your support, but..

Arrays are not always synonym of doom, as long as they are not used as a public API

Maybe that's a misunderstanding :)
My proposal is indeed public API arrays, as D7 has in hook_menu(), hook_theme(), etc. I don't see what's the big issue with that, if we already accept per-module yml files as a viable option.

-----

The current hook_route_info():
I think it feels awkward and unpleasant and too verbose, but technically it does what we need.
Esp, it does support route definitions on dynamic criteria, so other dynamic systems (APIs, UIs, other) can be built on top of it.

--------

Btw, I myself sympathize with yet another syntax, but I don't have much hope that it will be generally supported.

function hook_route_info($api) {
  $api->route('router_test_1')
    ->path('router_test/test1')
    ->controller('\Drupal\router_test\TestControllers::test1')
  ;
}

(The $api object, and the return value of $api->route() could be route collections and routes, but I rather imagine them as something built only for the sake of this hook.)

Crell’s picture

Yes, any API other than a developer writing a Route object is going to be slower on build than that, but some are slower than others. :-) That's not the sole hit against annotations.

#66: You can already call setOption(), setRequirement(), etc. on a Route object if you prefer to using the constructor. It's fairly flexible. I definitely don't think defining a different PHP API is worthwhile, as it just adds another layer of indirection. If we want to improve the signature of Route in non-BC-breaking ways, that can be done upstream in Symfony.

Which is, incidentally, one of the reasons I don't consider more mega-arrays to be a viable option. It's a less-documentable Drupal-only PHP layer on top of a perfectly working and already-documented non-Drupal-specific PHP API. I'm fine with exposing Route objects directly to module devs, or offering a YAML alternative. Both of those are non-Drupalisms. I am not OK with adding another Drupalism on top of it.

effulgentsia’s picture

FileSize
5.03 KB

I haven't chimed in here in a while, so here it goes. If you don't want to read a lot, you can jump straight to the last item and the patch.

- First of all, some good arguments have been given in this thread for why routes should not be config: they're not currently and making them that would have repercussions that we probably don't have time to deal with at this point in the D8 cycle. That's not to say that some routes (e.g., custom pages from SCOTCH, per-bundle Field UI pages) can't be derived from config, but that's an example of module-specified dynamic routes, not a universal "all routes are config" architecture.

- Secondly, I think it would be good DX to have a standardized entry point for modules to declare their routes. For example, non-CMI YAML files, controller annotations, hook_route_info(), [Module]Bundle::getRoutes(), an event subscriber, or whatever other ideas are out there. But of these options, I think we need to pick one and not make RouteBuilder::rebuild() invoke all of them, trying to figure out what any given module wants. That way, whatever is picked, someone knows that for any module, there's a single known place where to look for routes. Since we must support dynamic routes, that rules out YAML files and controller annotations as the common entry point. Of the other choices, I haven't found any other option more compelling than a hook. Maybe I'll change my mind on that after we get rid of all other info hooks, but there are some info hooks (hook_theme(), hook_element_info()) that I have my doubts on us converting to plugins or anything else OO like in D8. In any case, I see no problem with sticking with the info hook for now, and converting to something more OO any time up until code freeze. As long as we can agree on what the contents of the info hook implementation should be, then moving that contents later into a class method is pretty trivial.

- I also think that the info hook should continue to return a RouteCollection, as it does now. We're using Symfony's routing architecture, so that should be our common language at an API level.

- However, for the 95% of hook_route_info() implementations that don't need to do anything super fancy, I see no reason not to allow a terser, more declarative syntax. PHP arrays are great for that, so here's a patch that demonstrates one way we might do that. The key here is that we still return a RouteCollection: we just provide a helper object for generating it. For the key/value structure, I think we should stick as closely as we can to what Symfony defines for their YAML structure (since YAML and PHP arrays are pretty much interchangeable).

- Given that YAML is convertible to a PHP array in 1-3 lines of code with Symfony\Component\Yaml\Parser, this means that it would be trivial to extend this patch's RouteCollectionFactory to have a createFromYAML() method or whatever. Whether we want to do so in core or leave it to contrib is something I think we can defer to a follow up.

- I don't agree with some of #63's objections to annotations. As long as we provide an alter(), which we already do in HEAD, whatever is in annotations is no more tightly coupled than what's in an info hook, a yaml file, or wherever. In practice, most contrib modules involve the same person writing/maintaining the 'page callback' functions and the hook_menu() implementation. While it needs to be possible for *other* modules to rewire or add additional wiring, I think allowing the module itself to keep the default wiring together with the implementation is handy. Perhaps we can look into adding a RouteCollectionFactory::createFromControllerAnnotation() method, where you can pass it an array of controllers? If we do this, I think we should use Symfony defined annotations for this. Symfony also implements a concept of resource activation, where YAML files can instruct Symfony to load additional data from elsewhere. We could do something similar in our createFromArray() method where a key of 'resource' could be set to the value of a class name, and that means to load up annotations from that class.

- Summary of my recommendation: keep hook_route_info() as the primary entry point, keep it returning a RouteCollection, pass a $route_collection_factory that can be used to generate the RouteCollection in different ways, start using it with simple PHP arrays that follow Symfony's YAML structure, have follow ups for whether to extend $route_collection_factory to also work with controller annotations and YAML files.

Status: Needs review » Needs work

The last submitted patch, route_info-dx-68.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
5.9 KB
effulgentsia’s picture

FileSize
5.81 KB

sorry for the noise.

Crell’s picture

"Here's two different PHP syntaxes you have to learn in order to declare routes in Drupal. One is a the natural format used by a half-dozen systems. The other is a bunch of arrays that's kinda the same but not, and you still have to use objects in order to use it, but some module developers will use one or the other so you really do need to learn both."

Sorry, I really cannot get behind that statement.

tim.plunkett’s picture

"Here's a thing that you've never seen before, doesn't quite make sense, and is overly complicated for what you're doing."

That doesn't quite work for me either.

---

#68 perfectly sums up how I feel about this.

pounard’s picture

Here's two different PHP syntaxes you have to learn in order to declare routes in Drupal. One is a the natural format used by a half-dozen systems. The other is a bunch of arrays that's kinda the same but not, and you still have to use objects in order to use it, but some module developers will use one or the other so you really do need to learn both.

Almost everything in Symfony can be configured in all Yaml, XML, PHP or annotations. You may be shocked by giving two different ways of doing things, but Symfony always gives 4 different ones. EDIT: And by bundle DIC registration code, which makes 5.

EDIT: Edit that said, don't take it wrong, I understand your point of view, all that I'm saying is that if having two instead of one registration mecanism can bring more flexibility to the whole, I'm not against. First thing is to agree about at least one, and maybe discuss later about alternatives.

I'm all in for the first one as plain PHP, but not mandatory as an big array: bundle registration would be fine too. I'm not too happy with Drupal arrays, but let's face it, we are used to it (I'm against render arrays, not menu registration arrays). I'm not too happy about config because it sounds less evident to override right now, and because it doesn't bring the dynamic capabilities, but why not if somewhere find a parade, and I would be OK with this too in the end, in fact, config sounds really good in the paper if we find this dynamic stuff parade. I'm oppposed to annotation because of hardcore naming conventions it brings.

Crell’s picture

Offering PHP and YAML is one thing; they're two distinctly different mechanisms with different purposes, trade-offs, and use cases. PHP-A and PHP-B offer no meaningful difference other than one has more brackets than the other, yet both are still OO-based. They're effectively duplicates, not alternatives.

What about this: YAML for static routes, a la Symfony. And then an event listener for dynamic routes that just uses Route/RouteCollection objects. Those can be their own route set. (route set is a concept introduced with the routing patch that groups routes so that we can wipe/rebuild in pieces and never have the entire site's route collection in memory at once. Right now all route sets are modules, but there's no inherent reason why that has to be the case. That is by design.)

And since listeners can be injected, they can have access to any other services they want to cleanly and testably base routes off of entity types, image presets, etc.

donquixote’s picture

@effulgentsia / RouteCollectionFactory:

In this hook we are only ever going to return one route collection. Not more, and usually not less.
So what about we directly pass in the route collection? Can be a separate one for each implementation of hook_route_info(). And extend it with methods to create routes from yml, etc.

Or is it important to allow people to use an alternative implementation of RouteCollection?

I think it is better to have helper methods to create routes, than to have helper methods to create the collection. What if you want to add 3 routes with array notation, and then 2 routes with some other notation, in the same hook? Then the $route_collection_factory->createFromArray() can't do it.

Crell’s picture

Currently we compile each module's collection separately, to avoid memory issues. Passing one big collection around to all modules would likely run into memory issues. That said, we are not using any of the collection nesting ability that Symfony offers. I'm not entirely sure how we would, but it's there. :-)

donquixote’s picture

I'm not talking one big collection.

foreach (module_implements('route_info') as $module) {
  $function = $module . '_route_info';
  // Our modified RouteCollection knows the current module - this can be useful for some of the added convenience/factory methods.
  $collection = new RouteCollectionWithConvenienceMethods($module);
  $function($collection);
  .. // Do whatever with the collection, as if it was the return value.
}
effulgentsia’s picture

I actually kind of like #75, which might make #76 moot. Curious what others think of that.

#75 violates what I was suggesting in #68 about having a single entry point only, but we can choose to either be ok with a "yaml for static + event listener for dynamic" split at the RouteBuilder::rebuild() level, or we can make yaml the single point of entry, but allow yaml files to specify a resource key to import dynamic routes from a DIC service or similar.

effulgentsia’s picture

FWIW, I grepped HEAD for hook_menu() implementations and found that:

  • action, aggregator, ban, book, comment, contact, file, filter, forum, language, locale, menu, node, openid, overlay, path, poll, shortcut, statistics, taxonomy, toolbar, tracker, translation, update, user, xmlrpc all have static routes only that can easily be expressed in YAML.
  • dblog and help have routes that depend on modules being enabled, so would need PHP to express their routes unless we build in module checking/iterating into our YAML support.
  • Most routes in system are static, but a few check module existence or iterate themes, so it being able to use YAML for most, but PHP for the rest would be nice.
  • block, field_ui, image, search have dynamic routes beyond simple module/theme existence checking and iteration.

I would guess that to a first approximation, the above spread is reflective of the spread in contrib.

Crell’s picture

Another advantage of #75 is that it would be really easy to do. Start with the patch from #6, then add a listener. No new syntax, just some additional wiring.

I don't think core needs to provide a RouteCollectionWithExtraFun class unless we're going to push people to use it. Because it's a class, individual modules are free to provide subclasses of Route or RouteCollection if they want to offer extra utilities. Core doesn't need to care unless someone breaks the API. (We gotta leave something for ctools to do in Drupal 8... ;-) )

donquixote’s picture

#75, I like the DIC / testability argument.
I wonder, does it need to be an "event listener", or is just a method on the bundle object good enough? I imagine with an event listener we gain some flexibility but also some added DX overhead.

#79 / #75 / #76:
All that is said in #76 about hook_route_info() is still valid for event listeners or methods on the bundle object.
Ok, static config can now be treated separately, but still it can be useful to have some convenience/factory methods for route creation, or not?

Crell’s picture

The advantage of an event over a bundle method is that event subscribers can be managed by the Container and have dependencies injected into them. I don't believe that's possible for Bundle classes. They would need to do something like drupal_container()->get(), which makes them untestable, and more tightly coupled than they should be.

I'd rather hold off on additional wrapper utilities for Routes/Collections at this point. We don't have enough real data that Route objects are "hard" (I don't find them hard, but I admit I'm not a typical case). And, worst-case scenario, it's dead simple to add via contrib, or possibly during Code Slush. (I don't know if that would count as a new feature or an improvement of an existing feature. That line is a bit, er, slushy.)

donquixote’s picture

I thought that bundle classes would also be DIC-managed. If not, then sure, event listener is the better option.
(or maybe we should change the bundle classes and make them DIC-managed)

wrapper utilities / Route objects being "hard":
What nielsvm said in the comment on the change notice:
The hook in its current state is not unmanageably hard, it is just less pleasant to work with (read and write) than D7 hook_menu(). The syntax is more verbose and feels more redundant, and it has a poor ratio of information vs boilerplate.

Imo this is relevant - we are doing API design here, and this one will be used all over the place. We should attempt for a pleasant DX, not one that is "good enough" or "does not suck".
Others may have other priorities, of course.

sdboyer’s picture

we're talking about an EventSubscriber...which means registering something in the DIC that (potentially) does dynamic route calculation information at runtime?

i mean, that's kinda Drupal 5-y. and it's another thing that multiplies the complexity of each url-generating call. it also means the PathMatcher will need to update to accommodate the dynamic routes, since at the moment it only has to read out of the static store.

unless we're talking about having some kind of event subscriber which is just another event we make up, at which time route information is returned, then compiled back into the router table. in which case, i don't see much advantage over the hook.

feel like i've missed something, here.

Crell’s picture

sdboyer: Yes, you're missing something. :-) We're only talking about route discovery here. Routes will still get dumped down to the router SQL table (or an alternative implementation if one is so inclined), where the already-written PathMatcher and the in-progress Generator (#1705488: Implement a generator for Drupal paths) will work with it. This issue has no runtime impact at all. It's purely discovery/compile-time impact.

donquixote’s picture

@sdboyer:
A subscriber that registers dynamic routes will need services that represent persistent information: Views configuration, page manager configuration, entity bundles, etc. All those things that are currently used in hook_menu() implementations.
It should not depend on services that represent per-request information.

In a traditional hook, you need to pull all this stuff from global space, with things like entity_get_info(), node_get_types() etc.

The DIC can provide both (*):
Services that are tainted with request state, and services that only represent persistent site-wide state.
In an ideal world we should separate the two. I did not study the DIC in Drupal well enough, but afaik this does not happen atm. It is all one big container.
Still, you can decide to not provide services to your component that rely on request data.

(*) @Crell/others, please correct me where I'm wrong.

sdboyer’s picture

ah ok, so it's really just about container injection. ok fine, so long as we're still compiling it down. i suppose fewer calls to drupal_container() is always a good thing.

msonnabaum’s picture

Just wanted to comment that I don't agree with the arguments against annotations here. The limitations are almost identical to those of YAML, and annotations will be better DX.

EclipseGc’s picture

Jumping on the annotations train here re: #63:

1) Many of the things we define will not be controllers. Most normal "html page" routes will define a _content callback, not a _controller. So we can't say "find me all controllers", because that won't be all we define.

This is completely solvable via plugin derivatives. A derivative class to find all _content callbacks and update the parsed annotation appropriately is completely feasible.

2) Scanning for any callable that has annotations on it is costly. Plugins get away with it by having extremely tightly controlled namespace rules. We would have to do something similar, and scan \Drupal\$module\Controllers\Foo, and then scan each method individually. That still runs into the "it's not always a controller" problem above.

And you'll have to do similar for your proposed yaml files, congrats you rebuilt annotated class discovery with yaml.

3) Annotating the controller (or content callback, or any other such thing) tightly couples the controller to a specific path and route. We've spent a long long time decoupling page callbacks from specific paths, and in *most* places have succeeded. Even if we were to offer annotations and then some other method so that you could reuse the controller/content/thing, we'd then need to fully define that other method, and we're right back in the same "2 methods are confusing" problem that a couple of alternatives have.

Again, plugin derivatives get around this problem (as your 3rd objection and 1st objection are actually the same objection) and can have a very powerful consolidating effect by allowing you to provide a single controller for 99% of pages (dare I say the panels controller) and custom controllers for the pages that have that requirement. You're going to cache all of this stuff via some method in any event, so the costliness you've outlined here is negligible, and your solution to the problem is likely to be just as costly.

It's important to recognize that what plugins is really buying you here is consistency. Consistency within Drupal in general (as they're being implemented in many many sensical places right now). No it's not a golden hammer (As others have accused) but when you have a listing of metadata about something that you'd like to be a class, plugins are your goto thing, and that's what routes are. They have data about where they reside, and then a class to instantiate when you reach said destination. The discovery mechanism are pluggable for the sake of sanity and performance (i.e. we could discovery these HOWEVER we want), and we have a number of tools to facility common needs within this process (caching, altering, etc).

It's really disheartening to hear how plugins won't work for this situation and then see a custom yaml process proposed. At least learn the plugin system before telling me it won't work. I'm happy to help and outline how this could be implemented. Almost anyone who has asked for help with plugins has gotten it from me. Most of them have gotten code examples too. It'd be nice to make as many places within Drupal as consistent as possible. Plugins provides a robust solution for any info style hook system that needs replacing. I have yet to see a compelling argument for why it can't work here.

Eclipse

pounard’s picture

I was against annotations at first glance, but if you indeed have solutions for making it all of easy, performant and alterable, all my fears would vanish. What I'm afraid is that annotation defines routes, and something else alter them, is there something that would be homogeneous enough (so that a site developer can find alterations as easily as he can find the original menu entry definition) to provide that with plugins and annotations?

EclipseGc’s picture

The way plugin discovery works can be outlined at a few different levels. I'm going to be brief-ish.

AnnotatedClassDiscovery:
This is a basic discovery class, it look for annotations within a specific directory structure and takes two parameters in order to help facilitate the discovery of plugins within a custom (per plugin type) directory structure. $owner (the module who created this plugin type), and $type (the type of plugin). In the case of routes, I would guess the parameters here would be "Core" and "Route" respectively. This would mean that any PSR-0 directory structure could provide route classes in a [PSR-0 dir]\Plugin\Core\Route directory. Annotated classes within these structures then become available to us to use.

DerivativeDecorator:
The discovery system utilizes a decorator approach in order to nest various discovery behaviors. In the case of "derivatives" this is best described as being the same scenario as when you'd want a foreach loop in an info hook. In the case of Crell's objections, derivatives allow the same class to be used multiple times by completely different routes. This can be facilitated by any number of "sub-discovery" logic sets through a custom derivative class that is documented in the annotated plugin. This could be fed by a hook (and indeed is a marvelous way of doing backward compatibility layers in D8 for previous drupal versions) or really anything you're willing to code.

AlterDecorator:
The alter decorator is a simple wrapper around drupal_alter that passes previously found plugin definitions (within some other discovery method that was passed in here) and hands them off to a good old fashioned alter hook. The alter hook is specified as one of the parameters of the constructor of this class.

We have other decorators (CacheDecorator... which could honestly use some love) and a few others are starting to become apparent. In the case of what I've outlined thus far, we'd have a Plugin Manager class for routes with a discovery line that would look thus:

  $this->discovery = new AlterDecorator(new DerivativeDecorator(new AnnotatedClassDiscovery('Core', 'Route')), 'route');

This one line of code sets up a new search for [PSR-0 dir]\Plugins\Core\Route directories to find any route plugins, runs them through a derivative search to see if any plugin has a derivative class attached and if so treats each derivative found as though it were a separate plugin definition, and then offeres up all of these definitions to the module system via hook_route_alter(). Obviously we'll likely want caching here so either the cache decorator or a custom caching approach (which timplunkett has been playing with for the entity to plugins conversion). The plugin manager class is dense with capability and most people don't understand it yet. I don't have the time to dig into the possibilities there, but to say that I'm sure the system could support this would be an understatement. Hopefully this is helpful in illustrating some of the tactics available within the Plugin system and how it might apply here.

Eclipse

Crell’s picture

First, the claim that using YAML means "rebuilt annotated class discovery with yaml" is false. The patch in #6 is dead simple. The meaningful change (excluding updating the existing defined routes and such) is somewhere around 10 lines of code or so. That's hardly rebuilding anything. It's speaking directly to CMI, which is fine.

Second, Routes as plugins is not on the table. A year ago I might have argued for that. However, the routing system is deliberately skewing as close to Symfony as possible. The reason for that is two fold: One, it reduces the learning curve for new developers coming into Drupal if we do not hide the Symfony bits. To be sure, not all developers coming to Drupal will know Symfony inside and out. But given the choice between a Symfonyism or a Drupalism for a system that we are inheriting from Symfony, the odds of them having some prior knowledge to draw upon are unquestionably higher for a Symfonyism. Two, the Route definition system is already there, and we're already leveraging it, and it already works. Any Drupalisms we build on top of that should be kept as minimal as possible, so as to not end up reinventing the wheel.

I'm all for consistency, but just because we have an awesome new screwdriver doesn't mean everything long and pointy is a screw.

Because we have to move forward, I'm going to time box this discussion until Friday. Is there a compelling argument why "YAML (static) + listener (dynamic)" as an approach cannot work?

Sylvain Lecoy’s picture

Eclipse, annotations does not necessarily mean plugins, there is a lot of uses for annotations: ORMs, configuration... and I don't like the new new new syntax (we should definitely fix it by a fluent interface API).

Your proposition is very close to what I have explained in #49, but the approach of Crell with event seems a viable solution as well. I am just looking information if this would scale well.

sun’s picture

My stance:

  1. Annotations are very tempting (i.e., Annotations, regardless of Plugins), but realistically, we're not there yet. I'd suggest to ask @Driecatchick for an exception, in case it turns out Annotations would still be feasible to do for D8 post feature freze.

    Though I directly want to amend that Annotations are two-fold in terms of DX — while they make the corresponding metadata directly apparent/visible on the controller implementation, they also present a substantial loss of overview and discovery at the same time, because route definitions are inherently scattered across the code base -- there's no longer a single go-to place (within an extension) that provides a quick and easy overview of defined routes like we do have now. That's a very important aspect and downside of Annotations. I worked with Java Jersey annotations recently, which are the direct equivalent, and while their usage was very impressive and appealing, the lack of a single overview was very disturbing; not only during coding ("WhereTF is this route defined?"), but also, perhaps even more significant, with regard to security audits ("Which routes, exactly, do we have, and how, exactly, are they defined with respect to X?"). My personal bottom line: Annotations are nice when implemented properly, but they also have their own problems.

    Speaking of "implemented properly", the part that makes Jersey's annotations actually powerful is that they are not a single Annotation à la @Plugin. Instead, there are multiple directives à la @Path, @Method, @AccessPermission, which can be mixed and matched together and be supplied with custom arguments, whereas each Annotation is backed by a underlying service (i.e., you do not specify class names, the Annotation directive itself means that a certain class is invoked). AFAICS, that makes a big difference to what is being proposed here, which is only a manner of defining metadata somewhere. I'm therefore skeptical whether our usage would be a proper implementation of Annotations in this case.

    Lastly, although it is a weak case to be made, I tend to agree with @Crell that diverging too much from Symfony's way for defining routes is probably not in our best interest. This point is rather negligible though, since every Symfony-based application deals with the task in a different way and Symfony was purposively designed to handle it, and also, since our architectural needs are vastly different from typical Symfony applications in the first place. Thus, I think it is good to keep this point in mind, but it shouldn't be a major decision reasoning.

  2. The proposal of putting all static routes into YAML files and adding a pre-compilation event to allow bundles to adjust the routes based on dynamic data makes sense to me. If I understood the new router architecture correctly, the need for dynamic stuff should significantly decrease anyway.

    I'm removing some major offenders that were mentioned in #80 anyway already. Fact is, most of the current dynamically defined routes should not exist and just be static routes in the first place.

    Thus, let's go with static routes in YAML files + pre-compilation event as a replacement for hook_menu() and hook_menu_alter().

  3. However, I still strongly object to turning routes into configuration, for all the reasons that have been outlined earlier in this thread. We won't be able to ensure the required amount of reliability, dependency handling, and maintenance processes for configuration in D8.

    Furthermore, my stance on "what can be configuration" is crystal clear in the meantime: In order to convert something into configuration, all hard-coded references to it must cease to exist. Routes are currently hard-coded in entity render controllers, form controllers, list controllers, arbitrary forms, arbitrary links, arbitrary markup, JavaScript, and plethora of other code. I'm aware that you will repeat and tell me the idealistic "That's wrong and ought to be fixed.", but realistically, I'm not able to see how you/we would be able to pull this off for D8.

    Thus, routes in config is a non-starter for me. At least for D8.

  4. Potentially OT/separate issue: The only remaining part on DX from my perspective is that the new route definitions suck, big time, compared to our current/previous definitions. Each route has an obscure ID/machine_name, which, for the life of me, I'd have no idea what to specify there and I don't really understand why it can't be the path like previously/currently. The test implementation uses an integer count as suffix... I can only guess/hope that this is not meant to be how the reality will look like. I also don't quite understand where the rest of former hook_menu() will be specified. Overall, this leaves me with the impression that there's an awful lot of work still to do — not even taking the massive amount of conversions into account... Consider me scared. :-/

effulgentsia’s picture

FileSize
6.1 KB

Reroll of #6 to use non-CMI YAML. This does not yet invoke an event though.

effulgentsia’s picture

FileSize
6.1 KB

Fixed typo.

Dries’s picture

Assigned: Dries » Unassigned

Just caught up on this issue. In short, I'm fine with a YAML-based approach -- it looks like we have a lot of consensus on that -- and I'm fine with Crell making the final decision on Friday. I think it is safe to un-assign from me now. Feel free to assign it back to me if necessary.

effulgentsia’s picture

FileSize
8.86 KB

This adds the event dispatching.

- There's a single event. So whether you want to add a dynamic route or alter another module's route, it's the same event. Is that ok? It means that if a module wants to alter another module's dynamic route, it needs to register the listener with a smaller priority number (Symfony dispatches events in order from large priority numbers to small priority numbers).

- This preserves HEAD's existing approach of dumping one module's routes at a time. So the event runs per module. As opposed to hook_menu_alter() and most of our other alter hooks that run on the entire data in one shot (and correspondingly use more memory).

Annotations are very tempting (i.e., Annotations, regardless of Plugins), but realistically, we're not there yet.

I agree with deferring annotation-driven routes to a follow up. I don't think it would be that hard. Symfony already provides us with a Symfony\Component\Routing\Loader\AnnotationClassLoader class if we're willing to use their @Route annotation format. However, it only works with controllers that are class methods, not procedural functions, so one thing that will slow down that follow up is deciding how far we want core to go in converting 'page callback' functions into class methods.

Crell’s picture

My concern with a single event is that if you're registering a dynamic route, it will then get registered as part of every route set. That's no good. I think we probably need a separate dynamic_routes event, which themselves form a single route set. Then there's a separate route_alter event that runs for every route set separately, including that.

effulgentsia’s picture

FileSize
8.87 KB

I don't think I fully grok #100. Perhaps an updated patch or a comment with pseudo code would help. Meanwhile, this patch includes a typo fix and also adds a leading "/" to route paths, per Symfony's standard.

effulgentsia’s picture

I think we probably need a separate dynamic_routes event

If we want dynamic route additions to be separate from an alter event, do the additions make sense as an event at all? Or should they be a specially tagged DIC service?

effulgentsia’s picture

FYI: no need to hold this issue up for it, but I reopened #1632930-12: Add Symfony/Component/Config despite Symfony potentially releasing BC-breaking updates after 2.3., since that can let us directly use more of Symfony's implementations for all this (including Annotations).

webchick’s picture

I sat down with Alex about this tonight (thanks Alex!) because I was attempting to grok this via preemptively writing a change notice, and was left scratching my head over many of the same things that sun pointed out in #95.

Here's what I started: http://drupal.org/node/1822626 Let's continue to edit it so we can all talk about real, actual code in the context of this change.

We found a pretty great use case for the change notice, btw: book_menu(). It includes statically defined paths (admin/content/book), named wildcard paths (admin/content/book/%node), un-named wildcard paths (book/export/%/%), and a callback that returns non-themed output (book_export(), also defined at the book/export/%/% path). One thing it doesn't have though is dynamically-defined paths like node_menu() does for e.g. "node/add/' . $type_url_str" so I added that as well.

The biggest WTF for me was trying to figure out what to put as the machine name of the router. Alex and I kicked some ideas back and forth and settled on {module}__{description} (that's two underscores so there aren't namespace collisions) as a recommendation so we could get some baseline docs established. Definitely not married to that, but we'll run into the possibility of namespace collisions if we just use a regular _ there. I liked sun's idea of just continuing to use path, but I don't know how robust these machine names are in terms of accepting things like / and {} and didn't have time to play with it.

In general, I find it a bit frustrating that we're being asked to "decide" on this when there are still zero real routes in core converted to this system so we have zero experience with working with and using it, and while there are still widely unknown/open questions about how we're going to handle comparables to the existing code in hook_menu(). For example, we have no equivalent for the access callbacks/arguments, the 'file' index, menu types, title/description, weight, context, and more. So how can we possibly state right now that this is better for DX?

Anyway, hope the change notice helps us to clarify those things. It's necessary to close this out anyway, so we might as well agree on its contents ahead of time.

Berdir’s picture

@webchick: Some answers to these questions, I haven't been involved here that much, so correct me if I'm wrong.

- access stuff is handled in #1793520: Add access control mechanism for new router system, so that should probably be linked in the change notice as well.

- There is an existing change notice about routing, see http://drupal.org/node/1800686. That might help to answer some questions. These two change notices should be merged IMHO because they complement/replace each other.

- About dynamic routes and also stuff like title/description/weight.... hook_menu() currently defines both router information and menu_links. This system does *not*. It does not care about menus, menu items or how things will be displayed. It only cares about mapping a request to the function/class that is handling it. Menu links will be a separate system on top of this. Also, if you look at your example of dynamic routes, you can see that only the menu link part of it is dynamic. The route node/add/{type} can be static. I'm not sure about the plan in regards to about creating menu items, it is possible that we keep hook_menu() for that, maybe renamed to something else.

webchick’s picture

The intent of the change notice I created is to be then copy/pasted into http://drupal.org/node/1800686 and then deleted once it's ready, but we need a place to work in the meantime and I didn't want to obliterate the old one since it's still accurate for current 8.x.

And yes, I know about all of this. :) About hook_router_info() only defining routes, about access not being figured out yet, about menu links not being figured out yet, etc. I'm saying being asked to rule on DX when there are a bunch of open "will be"s and "maybe"s is incredibly silly. The @todos in that change notice are exactly what people updating their modules from D7 to D8 are going to want/need to know. We need to be able to fill in all of those @todos before we can really evaluate the DX of the system.

disasm’s picture

If anyone is interested in seeing a real world example of implementing the patch effulgentsia wrote, I converted the taxonomy autocomplete (JSON callback) to use the new YAML route in #1801356: Entity reference autocomplete using routes.

tim.plunkett’s picture

Status: Needs review » Needs work

I'd like to push back on the direction taken here. Namely, forcing developers to hand-write YAML to accomplish anything in Drupal 8.

This will be the first, and likely only, place that a module developer has to hand-write YAML files, with no opportunity to do it through a UI. Neither CMI settings nor ConfigEntity require handwritten YAML, and this will mean a third method of working with Drupal as a module dev.

Dries said earlier "I think my preference would be annotations."

After further discussions, he said "I'm fine with a YAML-based approach -- it looks like we have a lot of consensus on that".

But, I fail to see where the consensus lies.

mitchell’s picture

#108: #1816354: Add a REST module, starting with DELETE adds routes in a "resource plugin system with derivatives (stolen and modified from layout.module)".

EclipseGc’s picture

+1-ing #108 here. I fail to see the consensus as well.

frob’s picture

I don't really see much difference between yaml or an "array of doom" because all they both do is assign values to variables --except yaml cannot execute code.

With that said the added flexibility of having access to php in an "array of doom" makes me lean that way over anything else. I understand that we are attempting to move to a more standardized system and thus make it easier for new developers to enter the world of Drupal (at least I think that is the reason). However, I don't understand why we would move from one system that is as-flexible-as-can-be to a strict system that allows for no flexibility -especially when that strict system is just another way to define variables (which is what we where doing with "arrays of doom").

I have no problem with allowing developers to use yaml to define routes so long as we also have access to define them directly and access enough to alter them.

Before we can solve this problem we need two things -experience with the proposed system and a goal for developer experience. Do we have a standard for expected developer experience? If we don't then we should come up with one. It seems like our old standard for developer experience (arrays of doom) is loosing favor. I don't really know why "Drupalisms" are being frowned upon, this is Drupal. So long as we have a standard for developers to expect then I don't see what is wrong with that.

tim.plunkett’s picture

So long as we have a standard for developers to expect then I don't see what is wrong with that.

How will people know when to use which standard? We already have hooks and annotated classes, how would someone know that now they need to learn a third thing, handwriting YAML?

Crell’s picture

YAML would also be required here: #1793074: Convert .info files to YAML

I understand the discomfort with using a hand-edited non-PHP file here. However, just using an event would not allow us to segment routes by the module that provided them, which is necessary for the module-at-a-time-rebuild we want for performance. Losing that would be a regression. Also, YAML files can be parsed and unloaded. In-code route object definitions once parsed can not be unloaded, which results in wasted memory.

frob: As discussed, we're planning to keep an event-based runtime route definition mechanism, and an alter event. However, those should only be for dynamic routes, vis, routes that cannot be entirely defined from a static definition. I don't have a problem with Route objects, but a few other people have suggested that they may not be the ideal DX so we're offering a more self-documenting alternative, one that should be familiar to other Symfony-based project developers.

Note: The route-alter functionality is also being incorporated here: #1793520: Add access control mechanism for new router system. Hopefully we can get both of these nailed down and committed or at least RTBC this weekend at BADCamp.

frob’s picture

How will people know when to use which standard? We already have hooks and annotated classes, how would someone know that now they need to learn a third thing, handwriting YAML?

Because all developers know that they should read the documentation.

As discussed, we're planning to keep an event-based runtime route definition mechanism, and an alter event. However, those should only be for dynamic routes, vis, routes that cannot be entirely defined from a static definition. I don't have a problem with Route objects, but a few other people have suggested that they may not be the ideal DX so we're offering a more self-documenting alternative, one that should be familiar to other Symfony-based project developers.

I will hold you to this ;)

I think a layered approach is good. @tim.plunket YAML is very easy to write -so long as we remain with the standards and don't try and go crazy.

YAML -> routeObject

If we stick to the Symophony YAML then a basic route should look something like this:

blog_show:
    pattern:   /blog/{slug}
    defaults:  { _controller: AcmeBlogBundle:Blog:show }

Which is as easy to write as:

function hook_menu() {
$items = array();

$items[] = array(
  'blah' => 'blah',
);
}

It might even end up being easier for most routes. I don't have a problem with routeObjects, but their current implementation is kind of awkward.

donquixote’s picture

frob / Crell:

frob: As discussed, we're planning to keep an event-based runtime route definition mechanism, and an alter event. However, those should only be for dynamic routes, vis, routes that cannot be entirely defined from a static definition.

I don't see the necessary connection between "runtime" and "dynamic".

static and thus cacheable:
- yml definition, without altering mechanics
- annotations, without altering mechanics

dynamic, but cacheable (*) between requests:
- yml + alter mechanics
- annotations + alter mechanics
- "array of doom" info hook (+ alter mechanics)
- "array of doom" event-style hook (+ alter mechanics)
- "injected api" as in #66

dynamic and not cacheable:
- runtime route definitions

(*) "cache": I am talking about a persistence layer similar to our current menu_router table, which means
- only load routes that we actually need during a request
- rebuild every once in a while.

I personally don't see why we should be doing anything at runtime, if we can cache instead.
It might be ok for Symfony, but only because they don't expect a ton of routes to be registered.

Crell’s picture

All routes without exception will be "dumped" to the router table; at runtime, their origin does not matter. "Dynamic" routes are those that would, for instance, be "one per node type" or "one per entity type" or such, where they could *not* also be implemented by properly incorporating that variable into the path like any other parameter. In practice, I expect it to be a fairly small set of routes, just as it is now.

(I guess referring to those as "runtime" is not accurate; "code-defined" would be more accurate.)

I'm about to get on a plane to badcamp, so I'll be stealing the patch here to work on. :-)

donquixote’s picture

Ok, indeed "runtime" is a bad term to use.
It was not really clear to me that those route collections will be "dumped to the router table".

For "code-defined", we have quite a number of options (#115, "dynamic, but cacheable". But also explicitly creating route objects, if we indeed cache that).
Even yml or annotations can be seen as dynamic, if we have an alter hook.

I see a more extended field of use for dynamic routes than just the "per node type" example. Everything in current D7 where we use hook_menu_alter(), or where we let only one single value in the info hook array depend on something dynamic.

----------------------------

I understand the discomfort with using a hand-edited non-PHP file here. However, just using an event would not allow us to segment routes by the module that provided them, which is necessary for the module-at-a-time-rebuild we want for performance.

The main benefit of event subscribers over procedural hooks is that they can be DIC-powered.
If at the same time we lose the ability to know about which module the subscriber belongs to, then maybe it's not such a useful tool after all?
I would rather see us stick to producedural hooks (which are not officially dead in D8), or come up with a DIC-powered hook equivalent that knows about Drupal modules, than accept this as an argument.

-----------

module-at-a-time-rebuild

Btw, do we have any discussion about the actual performance and memory impact of module-at-a-time rebuild?
It seems wrong to me to just assume that.
From my experience, the major memory impact is not by the router data itself, but the other stuf that modules load to build their router items. E.g., views will load all views, and build routes from that - etc. This stuff will stay in memory, even if the router array is already gone.

Also, what exactly is the idea of module-at-a-time?
We still want to process all modules during one request, do we? Just "forget" the information of one module, before we step to the next?

Sylvain Lecoy’s picture

For preventing dev to write "new new new" syntax with Route object we can add a fluent interface API a la db_query or jQuery.

That would make the writing of dynamic routes less boring for devs.

disasm’s picture

#118 completely lost me. what does an API for jQuery have to do with routing?

donquixote’s picture

#118, are you talking sth like #66 ?

Berdir’s picture

Note in regards to "Developers would have to learn yet another way to specify things" (manually writing YAML).

If #1793074: Convert .info files to YAML happens, they will have to do that anyway and it could even make sense to argue that using the same format for routes makes sense. So we wouldn't add something additional, we'd just replace one format with another. One that they could also use to read and manipulate config files. Speaking if config files, how exactly do you create simple default config files (not talking about config entities and so on) if not by hand?

I'm not sure what's better but I don't think this is a valid argument against yaml files.

cweagans’s picture

I know EclipseGC has some interesting work around hook_menu that could be an interesting place to start. Personally, I think we should be using annotations, but let's get together in person to discuss it. We need to get this ironed out. Who all is at BADcamp right now and can meet up during the Core Summit to chat about this?

Sylvain Lecoy’s picture

@disasm no I was taking jQuery as an example of a fluent interface, takes #66 as proof of concept:
@donquixote yes I find #66 very cool.

<?php
$menu->addRoute('user/login')
     ->access(TRUE)
     ->title('Log in')
     ->file('user.pages.inc');
?>
Crell’s picture

Status: Needs work » Needs review
FileSize
10.6 KB

New patch, incorporates the two-event concept I was describing above. Also, I am defining routes in a class so they are constants, which gives us a place to document them, per suggestion in #1751116-13: [policy, then patch] Determine standard for documenting Events and Subscribers/Listeners.

I talked to catch at BADCamp and he's cool with the YAML/event model. The YAML part may or may not turn into CMI later. I can go either way, but since there's some added complexity there we'll punt that to a follow up.

This is also now in the wscci sandbox, branch 1801570-routing-dx.

Status: Needs review » Needs work

The last submitted patch, 1801570-routing-dx.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
FileSize
10.62 KB

Well that was a stupid mistake, Larry...

Status: Needs review » Needs work

The last submitted patch, 1801570-routing-dx.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
FileSize
11.26 KB

Forgot update.php. Sigh.

disasm’s picture

I've been working with this patch in #1793520: Add access control mechanism for new router system for the several hours, and I can confirm that it does exactly what it's supposed to do. The code is much cleaner and more elegant than the previous hook_route_info(). The YAML syntax is very easy to work with.

I think it's ready for RTBC, but I'd like to have a few more people weigh in before changing the status.

aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Routing/RouteBuildEvent.phpundefined
@@ -0,0 +1,46 @@
+class RouteBuildEvent extends Event {

Can we document wat this class does?

+++ b/core/lib/Drupal/Core/Routing/RouteBuildEvent.phpundefined
@@ -0,0 +1,46 @@
+   * Constructor.

Constructor for what? Should be Constructs a .... object.

+++ b/core/lib/Drupal/Core/Routing/RouteBuildEvent.phpundefined
@@ -0,0 +1,46 @@
+   * Get the route collection.

GetS

+++ b/core/lib/Drupal/Core/Routing/RouteBuildEvent.phpundefined
@@ -0,0 +1,46 @@
+   * Get the module.

See above

+++ b/core/lib/Drupal/Core/Routing/RoutingEvents.phpundefined
@@ -0,0 +1,38 @@
+   * @see Drupal\Core\Routing\RouteBuildEvent

\Drupal\....

Ok so I'm a developer, I made a nice yml file with my routes. The module is deployed. Suddenly I need a new admin page so I add a new route. In the old days I just cleared all caches. How does it work with the yml files?

effulgentsia’s picture

In the old days I just cleared all caches. How does it work with the yml files?

Deploy the yml file and clear cache.

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
@@ -57,15 +72,38 @@ public function rebuild() {
+    $collection = new RouteCollection();
+    $this->dispatcher->dispatch(RoutingEvents::DYNAMIC, new RouteBuildEvent($collection, 'dynamic_routes'));

Adding to an empty thing seems like an odd use of an event. Does Symfony do this elsewhere? Would a service tag like we do for 'chained_matcher' be more appropriate?

klausi’s picture

Status: Needs work » Needs review
FileSize
3.57 KB
11.48 KB

Fixed documentation stuff mentioned by aspilicious and the module name for dynamic routes. Committed to 1801570-routing-dx in the WSCCI sandbox.

Not sure what effulgentsia means, so no changes regarding that.

klausi’s picture

Title: Replace or improve DX of hook_route_info() » DX: Replace hook_route_info() with YAML files and RouteBuildEvent
effulgentsia’s picture

FileSize
10.48 KB
14.95 KB

Not sure what effulgentsia means

What I mean is that if you look at Symfony's HttpKernel::handleRaw() method, here's the flow:
- You start with an already populated $request, and KernelEvents::REQUEST is dispatched to allow changes/reactions to the request.
- You then get a controller (not by invoking an event), and then KernelEvents::CONTROLLER is dispatched to allow changes to the controller.
- You then call the controller to get a response, and then KernelEvents::VIEW is dispatched to allow changes to the response.
- etc.

Whereas #132 dispatches RoutingEvents::DYNAMIC not to change a RouteCollection, but to generate one. This seems wrong to me, but maybe others here more experienced than I am with event driven architectures can respond that this isn't as unusual as I think it is.

Here's a proof of concept patch for replacing RoutingEvents::DYNAMIC with a tagged service. I'm uploading it as a .txt file rather than .patch file, so that we do not consider it this issue's latest patch until Crell approves it, at which time, we can reupload it as a .patch.

The idea here is that CoreBundle includes a YamlRouteLoader service and other bundles needing to implement dynamic routes can add additional 'route_loader' services. One could also imagine something like CTools adding a AnnotatedControllerRouteLoader service, etc.

I haven't actually tested this patch, and I don't know if this is a good idea or not: just posting it as something to consider.

klausi’s picture

Interesting, so the dynamic routes provided by a module would be on the same level as the YAML defined routes. That could result in a lot of loaders, but I guess we produce a lot of objects either way.

1) I think that we all agree that statically known routes described in YAML are great.
2) We really need test cases for dynamic routes so that we can see the DX implications for a module. One test to add new dynamic routes and one to alter routes.

catch’s picture

If hand-editing YAML is a problem we can make these config. A lot of routes already are (based on) config anyway - anything to do with bundles, any manual pages like views pages. That means if you update your route you need to write a hook_update_N() - but that's the same for a lot of other things like this already.

klausi’s picture

Ok, added a test case for dynamic routes which currently fails. Bases on the original patch from #132. Not sure what I'm doing wrong and how I can register the dynamic route subscriber. Any ideas?

effulgentsia’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RouterTest.php
@@ -86,4 +86,20 @@ public function testControllerPlaceholdersDefaultValues() {
+    drupal_container()->get('router.builder')->rebuild();

This shouldn't be needed. Enabling the router_test module should automatically trigger a rebuild.

+++ b/core/modules/system/tests/modules/router_test/router_test.module
@@ -1 +1,11 @@
+function router_test_init() {
+  // Add the subscriber for dynamic route testing.
+  drupal_container()->get('dispatcher')->addSubscriber(new RouteSubscriber());
+}

This is unnecessary and likely the problem. There should be a RouterTestBundle class that registers this as a service with an ->addTag('event_subscriber').

20 days to next Drupal core point release.

klausi’s picture

Status: Needs work » Needs review
FileSize
15.61 KB
3.07 KB

Thanks, you are absolutely right. Works now with a RouterTestBundle.

I need to rebuild the routes because I'm using a random variable that is passed via the state() system to the RouteSubscriber.

Drupal 7 DX: implement mymodule_menu().
Drupal 8 DX: implement MyModuleBundle, implement MyModuleRouteSubscriber.

We can assume that quite a lot of modules will implement Bundle classes anyway, so the situation is not that bad.

Next TODO: add a route alter test case to this patch.

Crell’s picture

+++ b/core/modules/system/tests/modules/router_test/lib/Drupal/router_test/RouteSubscriber.php
@@ -0,0 +1,44 @@
+class RouteSubscriber implements EventSubscriberInterface {

We should probably call this RouteTestSubscriber or something to help avoid confusion with the real runtime RouteSubscriber.

+++ b/core/modules/system/tests/modules/router_test/lib/Drupal/router_test/RouteSubscriber.php
@@ -0,0 +1,44 @@
+    $route = new Route('/router_test/' . state()->get('router_test'), array(
+      '_content' => '\Drupal\router_test\TestControllers::test5'
+    ));

False. Why do we need state() here? This is a test. Just hard code a string here.

Even if we needed state(), this is an injected object. The state object should be added via a the constructor.

Crell’s picture

Also, I spoke with EclipseGc and effulgentsia at BADCamp about this issue. Kris made a decent argument for plugins, but I'm still not convinced that's the right approach. There's also a number of other issues that are piling up behind this one. So, we're going to move forward here as is (YAML and an event), but I am open to reconsidering this question after we get SCOTCH in, because blocks-as-controllers will have interesting implications here. We'll come back to that question when blocks-as-controllers actually happens.

klausi’s picture

Fixed the issues raised by Crell, added a route alter test case. Yes, the test for dynamic routes was probably a bit to much with passing the random variable around. Simplified it to a static string.

donquixote’s picture

Just a quick comment on dynamic vs static routes:
Currently it looks like static will be the usual case, and dynamic is something only a few modules will do.

While this is probably true, we should be prepared for a case where one module does register a ton of dynamic routes. Example:
- Integration of 3rd party apps: E.g. CiviCRM could decide one day to register all its routes in the Drupal routing system. It will not go the yml route, because those routes are already defined in code, and then it will just loop through that array to register all of them in a dynamic way.
- Modules that register routes based on higher-level user-defined information. In D7 that is views, not sure if that will still be the case in D8. But there is no reason why another module would not do the same.
- APIs built on top of hook_route_info(). I personally find our old-school hook_menu() very attractive, as gives you the menu tree placement for free. If we don't want to keep that hardcoded, the logical thing would be to make it an API on top of hook_route_info(). We can fight about hook_menu(), but the thing is, other modules could still have legitimate cases where they want to build an API on top of hook_route_info().

So, it might be only few modules that do hook_route_info(), but some of them might really register a ton of routes. Legitimately.
This does not invalidate any of the current work, but it needs to be considered when we talk about scaling.

Crell’s picture

donquixote: I believe you're correct in that analysis. However, even with all dynamic routes coming in a single "batch" that's still going to be fewer in memory at one time than now, so it's still an improvement. Installing something like CiviCRM may still cause memory issues there, but fewer than now. In core itself, we have over 1000 routes, something like 990 of which are static. That's a lot of memory we can just wipe from consideration.

As currently designed, I don't think there's anything route-based you could do in hook_menu that would not be possible via the Dynamic route event, so there's still feature parity there.

donquixote’s picture

Ok, I think we mostly agree in #144 and #145.
The rest in this comment is just notes for follow-ups and background thoughts, not meant to disrupt the work in this issue.

In core itself, we have over 1000 routes, something like 990 of which are static. That's a lot of memory we can just wipe from consideration.

I just want to add, we have never officially measured the memory impact of 1000 routes. Maybe it is not such a big deal in the first place? My suspicion is, the array itself is not such a big deal. More important is probably the stuff that modules load to dynamically create those routes. Definition of views, content types, fields etc, all loaded into memory.

As currently designed, I don't think there's anything route-based you could do in hook_menu that would not be possible via the Dynamic route event, so there's still feature parity there.

"route-based", yes.
I am still waiting for some proposal how modules can register links to show up in menu trees, breadcrumbs and tabs, if not with hook_menu() (and with an acceptable DX).
This has to be a new issue, obviously.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

catch has previously done a lot of profiling and determined that Drupal's various mega-arrays are a major memory suck in general. I don't recall the numbers for menu in particular, but I think it was multiple megabytes worth of memory for a full rebuild. That's the thing we want to avoid with the piecemeal rebuild. It's true that it doesn't help with the other dynamic things that loaded for that, but we can only fix so much at a time. :-)

I don't think the menu-split plan has changed since Munich: Move menu links to entities, routes to their own thing, and then add a layer on top of them that bridges from one to the other if needed. I'm not sure about some of the other bits in there, but that's as you say separate from this issue.

#142 is ready to go. Let's commit this and unblock things. Thanks for driving it home, Klausi!

Crell’s picture

I've also checked with the Symfonians about the "alter event" pattern. It's not something seen in Symfony, but they didn't have a better idea either, so we'll run with it. :-)

donquixote’s picture

Move menu links to entities, routes to their own thing, and then add a layer on top of them that bridges from one to the other if needed. I'm not sure about some of the other bits in there, but that's as you say separate from this issue.

#1835902: Allow modules to register links for menus, breadcrumbs and tabs (if not with hook_menu)

catch’s picture

Just a note on memory, the worst thing during a full menu rebuild is indeed loading every single view on a site to figure out if it has memory menu items - that's one reason why routes-as-config and views then using config API crud hooks instead of hook_menu_alter() to define routes would be a good thing (although there's also #853864: views_get_default_view() - race conditions and memory usage which tries to reduce the memory issue in a different way).

I'm going to tag this with 'revisit before release' due to the manual YAML writing issue, if that proves to be an annoyance we should definitely revisit it before code freeze, but I'm not against it in principle.

I do think there's an issue with collapsing adding dynamic routes with any ability to alter() them, but I haven't reviewed the patch to see if it tries to deal with that at all yet.

Crell’s picture

Issue tags: +revisit before beta

I think catch meant this tag...

catch: The patch that's RTBC has a separate event for dynamic routes and altering routes.

donquixote’s picture

(we don't want to get into detail about every specific module, but views seems like a good example)

Just a note on memory, the worst thing during a full menu rebuild is indeed loading every single view on a site to figure out if it has memory menu items - that's one reason why routes-as-config and views then using config API crud hooks instead of hook_menu_alter() to define routes would be a good thing

This is also what I remember (although I was mostly just looking at performance, not memory).
Imo this should be solved in views itself:
- on views save or rebuild, it should dump the routes-to-be-created to some intermediate storage.
- on menu rebuild, it should look into that intermediate storage, instead of views_get_all_views().

I am a bit sceptical about config.
As I see it, the idea of config is that users (= the site builder) can edit it.
At some point it can happen that user and modules want to edit the same piece of config. There are probably ways to solve that in 90% of use cases, but still it is easier to just not let the user mess with it.
In case of views, we want the user to configure the urls on views level, not on some lower level. Unless we want to change the way that views work.

What I could imagine is to have a config level on top of the existing route API. Then those routes that are created within this config level can be modified by the user via config. But other routes that are defined by modules are free from modification.
But this config layer would be independent of the module-provided yml files.

Otherwise, if all module-provided yml files are config, then suddenly modules need to babysit their routes with crud operations hook_update(). And code could no longer expect those routes to be present. This is quite undesirable for the average module.

This said: My knowledge of our new config system is quite limited.

Crell’s picture

don: Let's leave that question be for the time being. This issue just needs a commit, then we can move on.

effulgentsia’s picture

FWIW, I'm RTBC+1 on this. It's a nice improvement to what's in HEAD now, and further refinements brought up in this issue can be done in follow ups to the extent that people want to keep pushing on them.

sun’s picture

Looks good to me.

Although I'm very negatively impressed by how this seemingly simple functionality is scattered across so many different classes and files — that's going to be pain to debug. I'm starting to question whether Symfony's Event component is the right choice. But of course, that's a separate issue.

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
@@ -57,15 +72,38 @@ public function rebuild() {
+      $routing_file = DRUPAL_ROOT . '/' . drupal_get_path('module', $module) . '/' . $module . '.routing.yml';

As a follow-up, could we rename $module.routing.yml to $module.routes.yml?

I guess "routing" was an attempt to retain naming parity between the service/component and the "include" file, but as a developer, that name doesn't really make much sense to me, whereas "routes" would clearly denote that this file defines my module's routes.

effulgentsia’s picture

As a follow-up, could we rename $module.routing.yml to $module.routes.yml?

I chose routing for parity with Symfony's documentation, but do not object to us renaming it for Drupal.

moshe weitzman’s picture

I agree with RTBC here. I reviewed the code and am happyish with it. Its a DX regression as sun says but we apparently just accept those now if Fabien wrote it.

catch’s picture

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.phpundefined
@@ -57,15 +72,38 @@ public function rebuild() {
+    $this->dispatcher->dispatch(RoutingEvents::DYNAMIC, new RouteBuildEvent($collection, 'dynamic_routes'));

hmm I really don't like using EventDispatcher for this vs. the YAML files, and I think we should seriously discuss a wholesale move to config (with no public compile/alter phase at all) in a side issue before this goes in. That would give us a single API (the config one), not two and a half.

An alter hook that's not really an alter hook because it's a Symfony event feels like it's going to be very weird for both existing and new developers compared to the rest of core. I know some people want to go wholesale EventDispatcher and get rid of hooks in 9.x but I'm not currently one of these people and this is like wedge for doing that.

effulgentsia’s picture

@catch: what do you think of #134 then? That results in just 1 event, which we could rename from 'alter' to 'load'. It is a standard part of Symfony and other event architectures that event listeners can alter the information attached to an event.

Whether to make routes into config got stuck on #30 and the several comments following that: mostly around CMI not yet having solved how/whether module updates should propagate config changes. Has that changed since that comment?

Crell’s picture

I've been discussing this issue on the Symfony-Dev mailing list. Thread here:

https://groups.google.com/forum/?hl=en&fromgroups=#!topic/symfony-devs/7...

There doesn't seem to be a great deal of pushback on the concept, including the two-event approach, although it's not something Symfony typically does. The suggestions so far seem to be "info is a silly name, what about Collector" and "have an ->addRoutes() method on the event itself".

This issue is currently blocking the access control issue (which is rolled against it), which in turn is blocking our ability to convert any routes over to the new router without opening security holes. This issue was already time boxed to a deadline that passed 2 weeks ago. I don't want to sit and keep discussing things over again when there are issues backed up against this. Enough silver, we need to commit the gold.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

But I thought the whole point of forcing us to timebox a decision on the DX of an incomplete API is you only wanted to convert things once? If that's the case, isn't it more optimal to actually reach a decision here, rather than just commit something for the sake of moving forward and then have to re-roll all of those conversions later? And if it's holding up everything else that much, let's just roll those patches against hook_router_info() and be done.

This doesn't seem RTBC to me at the moment, given #157. I'd also like to see http://drupal.org/node/1822626 updated to reflect what's actually being proposed for commit here.

catch’s picture

Status: Needs review » Reviewed & tested by the community

@Crell:

This issue was already time boxed to a deadline that passed 2 weeks ago

Yeah not by me... we already committing the initial router patch with this a @todo, and left the access control as a @todo. If we're going to commit this follow-up and have yet another follow-up open to change it again then it's not fun. There's also no direct real dependency with the access control issue that I know of, just that "it'd be nice if they were both fixed for conversions" no?

@effulgentsia:

On #158, having a single event at least doesn't confuse the _alter() issue so much, but then you have to play with listener priorities if you're altering. That seems better than the false distinction at least for now but in general the concept still feels wrong to me, it's like having hand-written CMI files + a CMI alter/dynamic hook, + no CRUD operations.

Whether to make routes into config got stuck on #30 and the several comments following that: mostly around CMI not yet having solved how/whether module updates should propagate config changes. Has that changed since that comment?

I just re-read #30 and the following issues and that discussion seems to have stalled quickly and not really been answered satisfactorily.

I really don't think the CMI default updates is such a problem - Views needs an answer to that, it's already the case for hook_image_styles() and many other things which have moved from info hook + db/cache to CMI, using CMI here would just mean another impetus to solve it - but it doesn't add a new problem that we don't already have.

Crell’s picture

How is separating the definition from the alter a false distinction? It's a distinction we have in a dozen places in core already, today. And we cannot merge them into a single event while still keeping module-at-a-time rebuild for static routes.

catch: If you want to press for routes-as-CMI now, you are welcome to do so. I've given up on that for now, and picking my battles. As I said in #141, blocks-as-plugins-and-controllers-and-classes-oh-my from SCOTCH may impact this question so we may want to revisit then, but those aren't in yet. Until they do, that's not a question.

We're at comment #162. I'm done talking. As initiative lead, I have nothing further to say on this issue at this time. Either commit or mark postponed. But I am not OK with Drupal 8 shipping with the temporary hook_route_info().

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

@sun, @beejeebus, @yched, and anyone else who was against routes as config: care to respond to #161?

frob’s picture

Status: Needs review » Reviewed & tested by the community

I have gotten a little confused about this issue in general. I thought that this issue was supposed to be about figuring out how best to structure the API. That is, I thought this was just about DX.

It seems like this has become a discussion about implementation of an API.

All we needed to do is choose between: YAML, Events, or Annotations right? I thought that we had chosen a YAML/Event based approach right? So what does this new system look like to developers? How will we have to create and alter routes?

What I am saying is lets get some example code posted of what a developer will have to do in order to use this API. Once that is decided then the issue should be closed and the implementation of that API can be worked into whatever form makes the most sense. All I care about at the moment is that the API is friendly to use.

frob’s picture

Status: Reviewed & tested by the community » Needs review

whoops, sorry I didn't mean to do that. Must have posted at the same time.

effulgentsia’s picture

I think there's general agreement on YAML for static routes, which covers 90% or so of module use-cases, and is therefore where DX matters a lot. The "API" in this sense is the structure of the YAML file, which hasn't changed since #6. All a developer needs to do to use this API is write out the YAML file. Some on this issue have argued for a PHP info() hook instead of YAML, and some have argued for class annotations instead of YAML, but I think YAML is the leading option.

The remaining issue, then, is how to handle the use cases where routes need to be dynamic or where a module needs to alter another module's routes. While good DX would be nice for this, it's much more important that this part be architecturally sound. I'm tempted to say that we should commit #142 in order to get the YAML structure into core and in front of people who can then give us feedback on it, and in parallel with that, have a follow up for whether we should use CMI (which comes with its own API for managing insertions and alterations) instead of the custom 2 event approach in that patch. But perhaps catch has a point that it would be better to move to CMI in one shot, if there aren't objections to it.

@frob: does that help?

klausi’s picture

@frob: you can see the DX in the router_test module. Please read the patch again.

Anonymous’s picture

re #163 i don't remember why i objected, please don't hold this up on my account.

donquixote’s picture

#151 expresses why i am sceptical of config.

Also we should think about the use cases.
How likely is it that a user (site builder) wants to dive into config to change some routes? What can the site builder change there safely, and what will make a disaster?
How likely is it that a module wants to change the routes of another module?

Lastly, #157 sounds to me like:
Crell does not like hooks, so we can't use hook_route_info_alter(), but do it with an event instead. Catch does like events, so we cannot use either of that.
In the end we switch to config. Not because we expect any user to actually configure any routes manually, but because we cannot agree on alter event vs alter hook.
Please tell me I'm wrong.

Our traditional pattern was info + alter hook. We should either stick with that or do sth equivalent with events (which is the current plan, it seems). If we move to config, we need a really good and convincing reason.

sun’s picture

I still do not consider routes as configuration, and I still don't think the configuration system would be able to reliably manage and maintain updates to routes if they were in configuration.

As soon as configuration is installed, it is user data. Modules do not own it anymore.

Modules may attempt to update user data, but they cannot make any assumptions about the data anymore.

Because the user owns the data, the user can legitimately change and delete configuration, including configuration that was originally supplied as default configuration. This applies to views, to image styles, and everything else.

Thus far, to my knowledge, the entire system remains functional, even if the user would manually go in and delete almost all configuration files (only the deletion of system.module* and system.theme* will blow up).

Furthermore, the staging aspect is still applies and is still the same; if routes were configuration, then updating module code would not be sufficient, and would always have to be complemented with a proper import of configuration (i.e., from the config staging directory). That, in turn, would get us into an interesting and troublesome interdependency problem space with regard to running update.php.

It is not even clear yet how we're going to handle necessary updates to user configuration. That is still to be figured out in #1677258: Configuration system does not account for API version compatibility between modules and will most likely result in an "api" key in all configuration objects, so as to allow modules to determine whether and how a config object needs to be updated.

I have little interest in making that problem space even more complex by introducing a second type of configuration that the user would not own, or perhaps not own, or perhaps actually own (because why would it be configuration otherwise?), but then again not, because it also must be owned by modules and has to be manipulated by modules to ensure that their routes are functional, so who owns it, and who is allowed to perform what kind of manipulations?

frob’s picture

Thank you effulgentsia that makes more sense now. I don't see any reason not to use YAML for simple routes, I'm not suggesting that we make routes Configuration though. Its important that we get the YAML in soon so that we will have a chance to test it. I would assume that we will be adding to the syntax as we use it.

@klausi, I see it now thanks.

Before I start I want to be clear. I am coming at this as a user of the APIs.

router_test/lib/Drupal/router_test/RouteTestSubscriber.php


/**
 * Definition of \Drupal\router_test\RouteTestSubscriber.
 */

namespace Drupal\router_test;

use \Drupal\Core\Routing\RouteBuildEvent;
use \Drupal\Core\Routing\RoutingEvents;
use \Symfony\Component\EventDispatcher\EventSubscriberInterface;
use \Symfony\Component\Routing\Route;

/**
 * Listens to the dynamic route event and add a test route.
 */
class RouteTestSubscriber implements EventSubscriberInterface {

  /**
   * Implements EventSubscriberInterface::getSubscribedEvents().
   */
  static function getSubscribedEvents() {
    $events[RoutingEvents::DYNAMIC] = 'dynamicRoutes';
    $events[RoutingEvents::ALTER] = 'alterRoutes';
    return $events;
  }

My main concern with the above code is that most of the above will likely be rewritten every-time this Interface is implemented. (mainly getSubscirbedEvents method) If might be better if an abstract class that would be Extended that already implemented this EventSubscriberInterface. That would leave a developer with only dynamicRoutes to define.

Something like (I've never done this before so I am not sure that this will work):

abstract class RouteTestSubscriber implements EventSubscriberInterface {

  /**
   * Implements EventSubscriberInterface::getSubscribedEvents().
   */
  static function getSubscribedEvents() {
    $events[RoutingEvents::DYNAMIC] = 'dynamicRoutes';
    $events[RoutingEvents::ALTER] = 'alterRoutes';
    return $events;
  }

  public function dynamicRoutes(RouteBuildEvent $event);
  public function alterRoutes(RouteBuildEvent $event);
}

It would be nice for developers if the below two methodes where all that is required to implement.


  /**
   * Adds a dynamic test route.
   *
   * @param \Drupal\Core\Routing\RouteBuildEvent $event
   *   The route building event.
   *
   * @return \Symfony\Component\Routing\RouteCollection
   *   The route collection that contains the new dynamic route.
   */
  public function dynamicRoutes(RouteBuildEvent $event) {
    $collection = $event->getRouteCollection();
    $route = new Route('/router_test/test5', array(
      '_content' => '\Drupal\router_test\TestControllers::test5'
    ));
    $collection->add('router_test_5', $route);
  }

  /**
   * Alters an existing test route.
   *
   * @param \Drupal\Core\Routing\RouteBuildEvent $event
   *   The route building event.
   *
   * @return \Symfony\Component\Routing\RouteCollection
   *   The altered route collection.
   */
  public function alterRoutes(RouteBuildEvent $event) {
    if ($event->getModule() == 'router_test') {
      $collection = $event->getRouteCollection();
      $route = $collection->get('router_test_6');
      // Change controller method from test1 to test5.
      $route->setDefault('_controller', '\Drupal\router_test\TestControllers::test5');
    }
  }
}

Sun has some very good points in #170, and frankly I don't consider module defined routes as configuration. I like the YAML/Event model. If we can make the Event Interface a bit more DX friendly I would consider this RTBC.

Crell’s picture

Base classes for event subscribers are a separate issue. There's plenty of events that we're introducing (CMI even has one or two of its own already, in addition to the ones inherited from Symfony), and by design a subscriber class can listen to any event it wants, or even listen to the same event multiple times; we have examples of the latter in core already. It's totally legit for a module to provide a subscriber that listens to dynamic routes, to kernel.request, and to CMI's save event twice. There are even cases where that would be an optimal implementation.

I'm not against some utility base classes like that, but that's out of scope for this issue when we don't even have any dynamic routes defined yet. That's a non-blocking issue for later.

catch’s picture

Status: Needs review » Reviewed & tested by the community

@Crell, we have a standard pattern on info + alter hook - defining then altering the same data structures both in PHP.

YAML file + dynamic 'build' event + 'alter' event isn't that pattern - in fact it's a pattern that we don't have anywhere else, which is why I'm pushing back on this at the moment. Flouncing from the issue really doesn't help to resolve it.

@sun, if you delete all your routes, or all routes but one, then you still have a functioning system - just one that serves one or zero routes. Like effulgentsia I don't see how that's any different to anything else. In fact it's a feature request that Eaton has asked for numerous times (although there'd be other ways to achieve it like completely removing the default router implementation via the DIC).

Also this isn't true:

As soon as configuration is installed, it is user data. Modules do not own it anymore.

Modules will still update changed default values as they do now in hook_update_N() for variables (and this will probably remain inconsistent since there's no way to know if a site doesn't care, or explicitly left something at its default value) . Modules can rename or delete configuration objects or sub-keys altogether. If the module is uninstalled,the configuration should also be uninstalled. So at best it's 'shared ownership'.

Part of the intended features of the router system, assuming the generator goes in, is that you'll be able to go in and do things like re-assign the /node/%node route to /content/%node and everything should still work. Not via hook_url_outbound_alter() or similar but just reassigning where it points to in the router itself, since the generator creates a link from a router name + arguments, not from a url string, meaning everything should still work. The fact that Drupal lets you go around and delete content types, fields etc. that your site's structure and content is entirely dependent on isn't very clever, but it's not a problem that's introduced with router items by any means.

I can sympathise with committing this to get conversion of static router items going, since even if the location of the YAML might change, the structure of it shouldn't (or not soon).

I'm a bit confused why people think config === 'the user can edit it', that's never been the case in Drupal. If there's no UI for something, then you either have to create a UI (requires code, or installing someone else's written for that purpose), or you need to write code to change it. If you remove the UI from the equation then it comes down to how you define and change the data structure.

catch’s picture

OK so how about this:

1. I'll assign this to Dries for a second opinion. I haven't seen him in his assigned issues since BADCamp so not sure how available he is. Dries if you think this is fine, please go ahead and commit it, if not please say so here. I don't want to hold this issue up if Dries isn't available, and he's already said he's fine with Crell making the decision (but that was between YAML and annotations and before there was a working patch to look at so a bit of a different discussion), so that leads to:

2. I'll try to write up my objections to raw YAML + events in a new issue and link it from this one so we can discuss that there. That can be a follow-up to this one so we can press ahead with static router item conversion sooner rather than later. This week has been a bit scarce for core time so not sure when that'll be - but once that's done (or if someone else takes it on), then I'll probably commit this to keep things moving whether or not Dries gets to it. I don't think there's any controversy on the static routes at the moment (apart from niggles about hand-written YAML) so we can always make the follow-up critical but at least it's a smaller area than this one.

catch’s picture

Assigned: Unassigned » Dries
webchick’s picture

Alex and I have a call w/ Dries tomorrow so can put this on his radar, but http://drupal.org/node/1822626 hasn't been updated yet. What is actually RTBC here?

Crell’s picture

The patch in #142.

webchick’s picture

Yes, duh. :P The point of http://drupal.org/node/1822626 is to show how #142 applies to a real, actual use case. I wrote 90% of it already, and would really appreciate you taking the time to update the last 10% so we can talk about something real.

Dries’s picture

Having caught up with this, I find myself most aligned with catch. To me, it makes sense for routes to leverage CMI. Let's go ahead with this patch, and move to a CMI based approach in a follow-up patch. As I understand it, it wouldn't force us to re-do the conversions if we switch to CMI later; it should be a matter of just moving the YAML files instead of rewriting them. Hence we benefit from committing this patch now.

Dries’s picture

Assigned: Dries » Unassigned
Status: Reviewed & tested by the community » Fixed

Per #179, committed to 8.x. Thanks all! Yay to making progress. :)

Dries’s picture

Issue summary: View changes

Update with a formal summary.

xjm’s picture

Title: DX: Replace hook_route_info() with YAML files and RouteBuildEvent » [HEAD BROKEN] DX: Replace hook_route_info() with YAML files and RouteBuildEvent
Category: task » bug
Priority: Major » Critical
Status: Fixed » Active

The commit for this issue broke the REST tests. I've confirmed they fail locally after the commit and pass before it.

xjm’s picture

See: #1816354: Add a REST module, starting with DELETE. That patch went in since this one was last tested.

xjm’s picture

Issue tags: +Needs change record

Also I know nothing about this issue but it seems like something that needs a change notice?

xjm’s picture

Followup: #1840914: Convert routes to CMI

Having these random yaml files that are not config is really odd.

Crell’s picture

Should be easy to convert the REST tests to yaml files instead of hook_route_info(). I don't have time at the moment, but I'll check in IRC tonight and see if anyone else has gotten to it first.

xjm’s picture

Assigned: Unassigned » xjm
xjm’s picture

Assigned: xjm » Unassigned
catch’s picture

Title: [HEAD BROKEN] DX: Replace hook_route_info() with YAML files and RouteBuildEvent » DX: Replace hook_route_info() with YAML files and RouteBuildEvent
Status: Active » Needs work

I've rolled this back for now to un-break HEAD.

xjm’s picture

Category: bug » task
Priority: Critical » Major
Crell’s picture

Status: Needs work » Needs review
FileSize
21.19 KB

There's still a config() call in the new event that I cannot get to work as an injected object. I have no idea why, but we can figure that out later. This is green on my system.

Crell’s picture

Correction, this one...

klausi’s picture

Fixed injected config factory call, documentation, removed rest_route_info().

Re-ordered use statements in REST route subscriber.

Pushed to WSCCI sandbox 1801570-routing-dx-klausi branch.

The last submitted patch, routing-dx-1801570-192.patch, failed testing.

klausi’s picture

disasm’s picture

FileSize
4.59 KB

Attached is an interdiff all the way back to #142 making it easier to review the changes that fix the rest tests.

klausi’s picture

Added a doc block for the RouteSubscriber constructor.

disasm’s picture

Status: Needs review » Reviewed & tested by the community

All the new patch changes is adding a RouteSubscriber to create dynamic routes instead of using hook_route_info().

Since the last patch was green, and the most recent one only adds the docblock, I'm marking this RTBC.

Crell’s picture

+++ b/core/modules/rest/lib/Drupal/rest/EventSubscriber/RouteSubscriber.php
@@ -40,22 +38,19 @@ public function __construct(ResourcePluginManager $manager, ConfigFactory $confi
    * @return \Symfony\Component\Routing\RouteCollection
-   *   The route collection that contains the new dynamic route.
+   *   The route collection that contains the new dynamic routes.

I missed this, too. The event does not return anything. It just adds routes to the collection in the event.

+++ b/core/modules/rest/lib/Drupal/rest/Tests/DBLogTest.php
@@ -52,16 +52,12 @@ public function testWatchdog() {
-    /*
     $result = db_select('watchdog', 'w')
       ->condition('type', 'rest_test')
       ->fields('w', array('wid'))
       ->execute()
       ->fetchCol();
     $id = $result[0];
-    */
-
-    $id = db_query_range("SELECT wid FROM {watchdog} WHERE type = :type ORDER BY wid DESC", 0, 1, array(':type' => 'rest_test'))->fetchField();

Rolling this back is wrong. db_select() is entirely unnecessary here. Sure it's a test, but still, don't use db_select() when it's not needed. It's not needed here.

(I'm OK with the above being follow-ups to unblock other things.)

klausi’s picture

Patch updated per Crell's comments.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs issue summary update, -revisit before beta, -WSCCI, -Configuration system, -Needs change record, -Dependency Injection (DI), -YAML

The last submitted patch, routing-dx-1801570-199.patch, failed testing.

g.oechsler’s picture

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Yes.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, re-committed and pushed to 8.x.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Title: DX: Replace hook_route_info() with YAML files and RouteBuildEvent » Change notice? DX: Replace hook_route_info() with YAML files and RouteBuildEvent
Status: Closed (fixed) » Active

Did this ever get change-notified?

tim.plunkett’s picture

Title: Change notice? DX: Replace hook_route_info() with YAML files and RouteBuildEvent » DX: Replace hook_route_info() with YAML files and RouteBuildEvent
Status: Active » Fixed
Issue tags: -Needs issue summary update, -Needs change record

hook_route_info() existed for all of 2 months, over a year ago, and either never had an original change notice or had it deleted.
So we don't need to document the removal of a brand new thing.

RouteBuildEvent and the YAML files are documented in https://drupal.org/node/1800686

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

catch’s picture

Issue tags: -revisit before beta

This was tagged due to the necessity of manually writing YAML files. Haven't heard a single complaint about that for routes at least, and can't see us changing the API at this point.