[Last Updated: 4 July 2012]

Problem/Motivation

We need a new route dumper/generator, as discussed in http://groups.drupal.org/node/220269

Proposed resolution

See http://groups.drupal.org/node/220269 for background. We will not repeat what is said there.

The current implementation defines a multi-stage matching system, NestedMatcher. A NestedMatcher has 3 parts:

  • One InitialMatcher, which matches a Request object against whatever logic it wants.
  • Zero or more PartialMatchers, which are always assigned a RouteCollection from the previous stage and return a RouteCollection.
  • One FinalMatcher, which is always assigned a RouteCollection from the previous stage and returns an array of attributes to add to the Request, like any other Matcher.

Every sub-matcher is required to throw an appropriate exception if it comes up with no matching routes.

Right now we have an Initial matcher that mimics the Drupal 7 path-fit system, a Partial matcher for HTTP method, and a Final matcher for just returning the first remaining route.

Everything except the path-fit matcher will likely get moved out to either the Symfony CMF Routing component or the Symfony2 Routing component. Let's get it all sorted out here first, though.

Remaining tasks

#1793520: Add access control mechanism for new router system
#1705488: Implement a generator for Drupal paths
#1798214: Upcast request arguments/attributes to full objects
#1786460: Add the lock API to the DIC
#1798222: Use lock around router rebuild to avoid race condition
#1798296: Integrate CSRF link token directly into routing system
#1801356: Entity reference autocomplete using routes
#1801570: DX: Replace hook_route_info() with YAML files and RouteBuildEvent
#1859684: Expand routeBuilder unit test
#1908756: Separate Action Links (MENU_LOCAL_ACTION) from hook_menu()
#2023795: REGRESSION: hook_local_actions doesn't use title callback
#2004334: Separate Tabs (MENU_LOCAL_TASK) from hook_menu()

User interface changes

None, these are all API level changes.

API changes

Still TBD, but see:

http://groups.drupal.org/node/220269

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Issue tags: +WSCCI, +symfony

Tagging.

Crell’s picture

For those following along at home, I have a decent amount of code written for this, although right now it's not complete and is just passing unit tests. :-) See:

http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/routing

More details to come when I have it.

I am also talking to folks from Symfony CMF about this topic, as they have the same problem space. As a result, some of this code may end up split off to a separate stand-alone PHP library on GitHub that both we and they can pull in. Win for everyone.

dbu’s picture

hi crell

i read through the routing code and think the idea is interesting. looking at Symfony\Component\Routing\Matcher\UrlMatcher i feel that splitting that class into partial matchers would be benefitting. the logic to match a schema, request method, ... will probably always be the same, but there might be any requirement people could come up with. when we split the matchers in symfony, additional systems like the symfony cmf routing or drupal can add their own matchers and combine as needed in a nested matcher.

i wonder if it could be a good idea to also allow multiple initial matchers for the nested matcher? this would make the chain router from the symfony cmf obsolete. you could have arbitrary route sources and merge them together and filter them out. i guess drupal has multiple route sources? like the fix /node/{id} and /node/{id}/edit routes, admin routes provided by modules, and then the routes for content from the routing database tables.

i am a bit confused by the CompiledRoute class. i don't see it used anywhere in the code. the class bears no relation to the Symfony2 CompiledRoute, but the Drupal RouteCompiler claims to implement the Symfony2 RouteCompilerInterface - but it returns the Drupal CompiledRoute, not the Symfony CompiledRoute. what exactly is the drupal route compiler supposed to do?
maybe we should use a different name for this?

there are some details that could be optimized, like interrupting the partial matcher loop if we run out of candidates. and while the interface of FinalMatcherInterface::matchRequest looks right, NestedMatcher::matchRequest is probably wrongly documented. That should return the parameters for the route that was selected by the final matcher.

looking at what i do additionally in the Symfony cmf DynamicRouter[1]: where is the point at which you determine the content for the route? what do you think about the controller mappers that we have? or does drupal 8 not use routes in that way?

so, lots of comments but i think it makes sense what you did. how do we proceed with that code? do you think we could push the interfaces and general purpose implementations into the symfony routing component? if not, i would love to have it in the symfony cmf routing component.

[1] https://github.com/symfony-cmf/Routing/blob/master/DynamicRouter.php

Crell’s picture

Thanks, David.

The nested matcher concept was inspired by DynamicRouter, actually. I looked at its 2-stage approach on the plane on the way home after Symfony Live, and knee-jerk responded with "two? Why only two? n is the only number." Which led me to the nested approach. Symfony just recently (since Symfony Live Paris) added support for RequestMatchers, and there's another issue open^H^H^H^H that fabpot merged yesterday to remove the need for RequestContext on such matchers. (yay!) I figure nested matchers will always be dealing with a request, not a request context, so I'll switch over to that as soon as we get another pull from Symfony.

I don't like that I have to break it into 3 pieces. That seems too complex to me, but I needed some way to differentiate between "this needs a partially-filtered RouteCollection already / this has its own collection somewhere" and "this returns a RouteCollection / this returns route information", and do it separately. That also doesn't allow for multiple initial matchers. If you have a better suggestion for how to structure that I'm very open to restructuring that to make it simpler.

Another idea I toyed with was to make each incremental matcher a FilterIterator, mostly because I think such things are cool, but I couldn't figure out how to structure that so it made sense. :-)

I don't know what you mean by "content for the route". I also don't know that we're talking about the same thing for multiple route sources, so let me explain how Drupal works now/with this model, and you can tell us what you mean; that is of course very subject to change if appropriate.

In Drupal 7, all routes (along with lots of other information) are defined by modules using hook_menu(), which returns a big anonymous keyed array. That data is dumped to a database table along with "fit" information that's used to rank them. Then to look up a route, the incoming path is broken up and matched against possible paths in the DB table, ordered by fit, so that node/foo/bar wins over node/{placeholder}/bar when trying to match against node/foo/bar. The returned data includes access check rules and a function name to call, plus its arguments. That function is, effectively, the controller, with arguments.

For Drupal 8, the plan is that declaring routes will be done via hook, which returns a keyed array of Route objects or, possibly, a single RouteCollection. Whether we'll expose an easy way to leverage things like path prefixing, I have no idea yet. One step at a time. For matching, I plan to use ChainRouter to keep our old routing system in place while we build a new one in front of it and transition over; after we've moved over, we can eliminate the old one. We may leave ChainRouter in place at that point to offer sites the option of having optimized routers, but I haven't decided yet.

For nested routing, I expect we'll push a lot of stuff into the initial matcher: Path, possibly method, hypothetically domain, etc. A second-pass in-memory matcher would be responsible for content type negotiation, parameter type checking (eg, let string and int go to different routes), etc. That could be one matcher or several; I have no idea. And then Final... probably just matching the first option, like the code does now, because I don't know what else we'd do there. :-)

The controller we return will, I think, virtually always be a method of a class, usually one that matches the Block interface, which is in development as part of the Plugins system. That's how we then do an all-block, top-down, ESI-super-friendly layout model. In non-HTML cases it will map to a method of a class, period, have a nice day, which returns a Response like any other controller.

Does that make sense? Does it make it easier to explain what you mean by route content? Any suggestions on how to improve that design are very welcome at this point. It's still flexible enough that I'm fine with improving that approach if we can. There's definitely lots of room for optimization as you note above.

dbu’s picture

for the content: your block method would be a controller in symfony2 terms, right? it fetches content from somewhere and figures out the template and renders stuff and produces a response.

in the cmf, we wanted to avoid the need to write your own controller if all you do is fetch a content that is rendered by a twig template. so we have a default controller that is choosen if the route specifies just a content (an arbitrary object) and that controller just gives the content to the twig file you specify either in a mapping from content to twig or in the database.
after we had this, we noticed its actually very convenient for a custom controller to get a related content along with the request. for drupal this could be the entity for the main container block that is used at that url. but maybe it makes no sense in your context. i hope the idea is now clearer?

Crell’s picture

Issue tags: +wscci-hitlist

Ah, OK. Yeah, that's a concept that we don't yet have, although I can see the potential for later. So yes, the "route content" doesn't really have an equivalent in Drupal.

Yes, in the planned Drupal-speak Block = Controller that returns HTML or an HTML fragment. All Blocks are Controllers, but not all Controllers are Blocks.

Crell’s picture

Oh yes, and CompiledRoute is used in MatcherDumper::dump(). At fabpot's recommendation I made our own and modeled it conceptually on the one in Symfony; we could go without it if we wanted, but I figured start familiar.

dbu’s picture

ok, so the full dynamic router would be for later maybe. actually i think dynamic router basically is NestedRouter plus the enhancement (adding things to the defaults). we could extend the NestedRouter, or we could have some sort of route enhancement step after the final resolver. or the final resolver could actually do the enhancement, do some stuff on the route that was selected to add information to the result array (as the content in the cmf case).

what is your opinion on pushing nested router upstream? did you talk to fabpot if he would accept that in the symfony routing component? or should we put it into the cmf routing component?

Crell’s picture

I've not spoken to fabpot either way about it, but if he's OK with portions of this going all the way back to the Routing component then so am I. Naturally some implementations will be Drupal-specific, but the interfaces if nothing else could move as far upstream as we want.

Yes, I think FinalMatcher is where any "enhancing" should be done.

R.Muilwijk’s picture

Mind me for not joining into the conversation earlier. I'm mostly curious about the following practical thing, when working in a framework and creating two content types I'd rather create two entities like Page and News. Both with their custom router rules like /news/:title and /:menu_location. When working with Drupal this is fixed by aliases using the path module (or pathauto for that matter) translating /news/:title to /node/x and is saved for each node. In the new setup I would expect that instead of a per node store this would also be possible to do on a per content type basis, removing the node/x Route and instead of the two earlier mentioned Route's. How is this planned in the current architecture?

Crell’s picture

Re #10: I am not sure yet if that will be possible. What we'd need to do there is define a route-per-type to begin with, and then have them fold down to the same paths, if we can do that without breaking. However, that makes it harder to generate a link to "this node, whatever type it is". I agree it would be nice to enable that, but I don't think it's critical as long as we have path aliases available.

Crell’s picture

Status: Active » Needs review
FileSize
54.67 KB

First patch! This is just a diff off of the routing branch in the WSCCI sandbox, and totally not done, but it has a bunch of new passing unit tests. :-) I also updated the summary with more details.

This needs architectural review, please!

Crell’s picture

Issue summary: View changes

Add a real summary.

hinikato’s picture

I would like to suggest one more addition. I don't know is it proper place to post, but I will try.

It would be cool IMHO to add the following feature to Drupal. I will describe it on following concrete example:

  1. User enters the "/foo/bar/something" URI.
  2. System will check that the "edit-in-place" mode is on.
  3. If mode is on the visual editor should be displayed similar to panels_ipe. Using this editor user will be able to "Create Page here." and assign some blocks/regions for this newly created page. System will assign route to that page internally when user will click the "Create Page here" button/link. It will be similar to wiki but for URIs.
  4. If mode is off the 404 page is displayed.

What you think about that?

Crell’s picture

I don't know what "edit in place mode is on" means, exactly. It sounds like you're talking about a wiki-style "go to the page that you want to create and create it on the fly" type behavior, but with IPE. That's... interesting. I don't know if we can support that with a pattern-first matcher. Possibly as a second chained router that checks for a non-existent URI and a specific custom mime-type? That could be kind of cool, actually, but not something for the initial implementation.

dbu’s picture

Status: Needs review » Active

I gave this some more thought and have a few doubts about the current implementation:

This proposal introduces more new classes than necessary. Merging it upstream into the core symfony routing component would produce unnecssary BC breaks. Instead of the NestedRouter, we could keep the cmf DynamicRouter, but refactor the core symfony UrlMatcher to be composed of partial matchers and the final matcher instead of the current monolitic one. That way, the change could go into symfony core without changing any public methods at all. (i guess we would also do a RequestMatcher and there would need to be PartialUrlMatcherInterface and PartialRequestMatcherInterface).

I should refactor the DynamicRouter to chain arbitrary route enhancers after the matching route is found. In the CMF we would use them to do set the _controller field and the _content field, but this logic should not be hardcoded in the DynamicRouter.

The other thing i noticed is that the UrlMatcher of symfony will only filter through the route collection until it finds the first valid match. The PartialMatchers however will each loop through all routes and try to match them.
To reproduce this behaviour with separated classes, we could refactor the PartialMatchers to act as filters on the collection, rather than take one collection and return a shortened one. This will be faster when the InitialMatcher gives a lot of results - at the cost of more method calls.

If you are interested in this approach, i suggest we open an issue on github.com/symfony/symfony and ask fabien if he would be interested in such a pull request. if not, we need not worry about BC with the core Symfony2 and can just push the nested router into symfony cmf.

dbu’s picture

in summary:

* i suggest we propose to symfony core if they would accept us refactoring UrlMatcher to use the partial mappers you suggest. we might need to change the logic to avoid wasting time on filtering more entries than necessary, as there is no initial matching in the core, but just a potentially large collection of routes
* if symfony core does not want that, we could move the features of NestedRouter to DynamicRouter (i'd prefer the name Dynamic, because along with ChainRouter, the nested name could be misleading).
* i should refactor DynamicRouter to have an additional route enhance step after the final matcher, instead of the currently hardcoded handling to map a controller and potentially content.

sun’s picture

Issue tags: +API change

Tagging.

pwolanin’s picture

Per discussion with Crell, starting some work on the generator piece here: #1705488: Implement a generator for Drupal paths

dbu’s picture

note that crell created an issue over at symfony github to discuss adjusting their code to become more flexible: https://github.com/symfony/symfony/issues/5014

Dries’s picture

Priority: Major » Critical

I'd really like to see a router patch land by the end of DrupalCon Munich. Crell and others, do you think that is feasible?

I've tried to help facilitate that by focusing my time on patches that enable that, and would be happy to do more if you point me in the right direction.

Crell’s picture

Thanks, Dries.

The bundle patch helps with that. The next big blocker is that I discovered recently (and noted elsewhere but forgot to put in this thread; my oversight) that ChainRouter requires RouterInterface, which requires UrlGeneratorInterface, which still requires a RequestContext, not a Request. So that upstream work is not done yet.

At the moment I think we should probably move forward with a super simple ChainMatcher that does just the matching part, but the same basic concept, on the assumption that we will replace it later as we refactor things. Meanwhile, pwolanin is working on a new Generator class, which we'll need as well for a full Router object.

I'll try to squeeze some time in to try and wire the matcher up on its own with a custom chain matcher ASAP, when I'm not working on Munich presentations. :-)

moshe weitzman’s picture

Weekend project for @Crell?

I also want to make sure this patch can handle the http header that triggers PJAX response thats described at http://groups.drupal.org/node/247073

Crell’s picture

Status: Active » Needs review
FileSize
83.18 KB

OK, new patch.

Since ChainRouter isn't viable for us yet, I went ahead and ported it to a ChainMatcher. Same logic, just Request-Matcher specific. I also went through and wired the new matcher into the system, and wrote up a webtest with sample module. It all works and passes. It doesn't have a way to handle access control yet, so it's not done, but it's closer to it.

Can someone besides dbu give me a real review here? At this point I think we'll have to move forward with the architecture as-is, period, since no one has had any comment on it in the 2 months it's been available.

Status: Needs review » Needs work

The last submitted patch, 1606794-routing.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
FileSize
83.57 KB

Of course it's a failure that only happens with testbot and not on my local system. It wouldn't be any fun otherwise.

catch’s picture

Status: Needs review » Needs work

Haven't reviewed the whole patch yet, and managed to lose some of my review in dreditor, so this is a bit all over the place, but some thoughts at least:

+++ b/core/lib/Drupal/Core/LegacyUrlMatcher.phpundefined
@@ -98,8 +99,8 @@ class LegacyUrlMatcher implements UrlMatcherInterface {
+  public function matchRequest(Request $request) {
+    if ($router_item = $this->matchDrupalItem($request->attributes->get('system_path'))) {
       $ret = $this->convertDrupalItem($router_item);

What's a DrupalItem?

+
+  /**
+   * Constructor.
+   */
+  public function __construct() {
+    // We will not actually use this object, but it's needed to conform to
+    // the interface.
+    $this->context = new RequestContext();
+  }
+
+  /**
+   * Sets the request context.
+   *
+   * This method is just to satisfy the interface, and is largely vestigial.
+   * The request context object does not contain the information we need, so
+   * we will use the original request object.
+   *
+   * @param Symfony\Component\Routing\RequestContext $context
+   *   The context.
+   *
+   * @api
+   */
+  public function setContext(RequestContext $context) {
+    $this->context = $context;
+  }
+
+  /**
+   * Gets the request context.
+   *
+   * This method is just to satisfy the interface, and is largely vestigial.
+   * The request context object does not contain the information we need, so
+   * we will use the original request object.
+   *
+   * @return Symfony\Component\Routing\RequestContext
+   *   The context.
+   */
+  public function getContext() {
+    return $this->context;
+  }
+

This is really messy, is there a PR to help us get rid of it?

+/**
+ * A PartialMatcher works like a UrlMatcher, but will return multiple candidate routes.
+ */
+interface FinalMatcherInterface {
+
+  /**

Is this a stale comment? FinalMatcher is neither of those is it?

+   */
+  public function generate($name, $parameters = array(), $absolute = false) {
+    $route = $this->connection->select($this->tableName, 'r')
+      ->fields('r', array('route'))
+      ->condition('name', $name)
+      ->execute()
+      ->fetchField();
+
+    if (!$route) {
+      throw new RouteNotFoundException(sprintf('Route "%s" does not exist.', $name));
+    }
+
+    $route = unserialize($route);
+

So I understand the generator is supposed to decouple us from the actual router path (i.e. url('node/1'), but having to individually query the router item and load all the information for it just to print a url() looks like a too heavy price to pay for that.

Before doing our own implementation because we're worried about the scalability of our massive router table, I'd rather look to see if there's ways we can mitigate the massive number of router items we have - possibly enough we can use the same or a similar implementation to what Symfony does already.

For example I just had a quick look in the router table of a large Drupal 6 site.

It has 1,673 router items:

mysql> SELECT COUNT(*) FROM menu_router;
+----------+
| COUNT(*) |
+----------+
|     1673 | 
+----------+
1 row in set (0.00 sec)

But of these, 1,296 of them are admin/* paths.

mysql> SELECT COUNT(*) FROM menu_router WHERE path LIKE 'admin%';
+----------+
| COUNT(*) |
+----------+
|     1296 | 
+----------+
1 row in set (0.00 sec)

Out of that 280 remaining paths, 30 of them are node/add/type paths (this is on D6, hopefully we no longer loop over content types in Drupal 8, or if we do we should stop that). There's also over 42 paths at user/% for things like signups and profile categories etc.

Not sure how viable this is, but a couple of ideas:

- add a highly optimized generator for non-admin paths, fall back to what we have for admin paths - can you chain generators together?

- try to reduce the number of actual routes, for example a admin/forms/%form + menu links instead of individual router items for every form.

+
+  /**
+   * Dumps a set of routes to a PHP class.
+   *

+    // Convert all of the routes into database records.
+    $insert = $this->connection->insert($this->tableName)->fields(array(

Comment needs updating, it's not dumping them to a class anymore.

+
+    // Delete any old records in this route set first, then insert the new ones.
+    // That avoids stale data. The transaction makes it atomic to avoid
+    // unstable router states due to random failures.
+    $txn = $this->connection->startTransaction();
+
+    $this->connection->delete($this->tableName)
+      ->condition('route_set', $options['route_set'])
+      ->execute();
+
+    $insert->execute();
+

This is leaving a potential race condition when there's an empty router collection before the insert happens, not sure if transaction isolation might save this but it probably varies, so we might need our own lock there and/or something to handle an empty collection in calling code.

I don't have massive alarm bells going off about anything except the generator, although it's going to take some more read throughs to see how it all falls together.

catch’s picture

Status: Needs work » Needs review

dreditor status change, this obviously needs more review.

Crell’s picture

Your dreditor seems to be very cranky lately. :-)

DrupalItem is a menu router item. LegacyUrlMatcher is already in core. It's just a legacy wrapper around the current router. The only changes here are to swap the interface it uses so that we can use NestedMatcher.

The extraneous request context stuff is gross, I agree. It's better than it used to be. I asked Niklas Fiekas to look into further refining the Request-based rather than Context-based routing in Symfony, since he's the one that was already working on it. I don't think he's had as much time lately, so I don't know where that stands.

FinalMatcher comment: Yep, I need to update that.

Ignore the generator code for now. pwolanin is working on that in another branch (#1705488: Implement a generator for Drupal paths). What I've got there is just a first start I didn't feel like trying to filter out of the patch. :-)

For reducing the number of routes, yeah, the Symfony people keep telling me to do that, too. :-) We may be able to, but that would require mapping the concept of a Route to something other than a $menu_item. ChainRouter would, in theory, allow us to put different routes in different tables, or query a single table in pieces, etc. if we were so inclined. (I didn't realize admin pages were quite that high a percentage.) There's probably other things we can think of to restructure that, too. For now, I'm going for a 1:1 mapping for simplicity but I'm definitely open to reworking that later as we start converting things.

Not sure about the race condition, but if we can get the lock system to be injectable then I can easily throw that around it.

adamdicarlo’s picture

Every sub-matcher is required to throw an appropriate exception if it comes up with no matching routes.

Does that mean that, on a typical page load/request, a bunch of exceptions might be thrown simply during routing? Why would throwing exceptions be a normal part of the request process? (i.e. exceptions used for non-exceptional circumstances, or "vexing" exceptions?) Even if Drupal is returning a 404 page, I'm not sure it makes sense -- there's nothing exceptional about 404 requests on the web. (Am I missing something here?)

pwolanin’s picture

I'm confused about some of the use statements - seems like you are including one too many levels?

e.g. use Drupal\Core\Routing\MatcherDumper in the test.

I'd think that should just be use Drupal\Core\Routing ?

It seems like symfony has one level deeper directory structure than so far in Drupal - was the goal to match theirs?

Crell’s picture

Exceptions as a way to handle 403 and 404 and such is deliberate, and is inherited from Symfony's design. We're already doing that, and drupal_not_found() etc. have already been replaced by exceptions.

pwolanin: use Drupal\Core\Routing would "use" the namespace, and then inline we'd have to say Routing\MatcherDumper for a class name. Symfony does that a lot, actually, but Drupal coding standards say to list individual classes specifically.

pwolanin’s picture

GeneratorTest has failures for me locally, and is hanging on my branch, maybe due to the constructor changing.

pwolanin’s picture

The CompiledRoute class looks odd to me - I don't understand why it's constructor and methods are so tightly coupled to the DB routing, and missing some methods from symfony

julien’s picture

I did run the tests and all went fine for me.
I was wondering if you manage required and optional arguments in the path, because i saw that in symphony it is required by default, so a path like my-path/* will not route to my-path, arg has to be declared as optional in the route config file apparently.
Looking at the db table structure, i assume it will go in the json string somewhere?

pwolanin’s picture

Status: Needs review » Needs work

I'm not sure why the Drupal implementation of the route compiler is so different - I think we need to maybe to use something that behaves more like the symfony one.

Crell’s picture

Regarding the route compiler: We probably need to switch to a subclass of the symfony compiler and compiled route for the generator, as I discussed with pwolanin. I didn't do that here because I didn't want to conflict with anything he was doing on the generator. The Symfony default approach is rather tightly coupled to their default implementation, too. That's why the compiler class is a setting on the Route objects. It is backend-specific.

Regarding paths: Yes, implicit optional tail arguments are going away. They're a bad idea to begin with. :-) That will affect search and private files, but both are straightforward to resolve and should have just been query parameters in the first place anyway.

pwolanin’s picture

FYI, there is a lot of desire by people to use search keywords even facet parameters as path components for search. So, I think we will need something like menu_tail but it should be an explicit choice, not allowed for all paths.

donquixote’s picture

So, I think we will need something like menu_tail but it should be an explicit choice, not allowed for all paths.

"variadic path argument" I would name this. It doesn't even need to be the end of the path, but it should be only one per route. Otherwise it gets difficult.
Another one could be "optional arguments".

But I agree, it should be explicit, not implicit.

dbu’s picture

hi crell,

i am back from holiday and would love to help out making the code you write here reusable.

i created a pull request against symfony to make the url matching code reusable: https://github.com/symfony/symfony/pull/5351

and i changed the cmf ChainRouter to use matchRequest (but still work with url matchers when given)
https://github.com/symfony-cmf/Routing/pull/14 - does this make the ChainRouter usable again for you?

what else is needed to only have drupal specific things in this patch, and all else upstream? once the symfony core PR is decided, we could try to merge the NestedRouter things with the cmf DynamicRouter. what do you think?

dbu’s picture

ok so fabpot is not happy with the approach and does not have time to discuss what should be done instead. i propose we just copy-paste the code into a partialmatcher of the cmf component then, but isolate it in a way that would allow to switch to an upstream solution if it becomes available at some point.

Crell’s picture

dbu: I'll check with a few people on our end to make sure they wouldn't object to using the CMF Router component, and if not I'll send you a PR as soon as I have a chance to. (At the moment we're only chaining the matcher, as we don't have a generator yet; would that still be viable?)

donquixote: Wow, that name is abstract and theoretical even by my standards. :-)

dbu’s picture

thanks crell.

* for the matching, i propose we change DynamicRouter to follow your NestedMatcher concept (i think we do not need a NestedMatcherInterface, just the InitialMatcherInterface (should we call it RouteLoaderInterface btw to make it more obvious?), PartialMatcherInterface and FinalMatcherInterface. plus maybe a RouteEnhancerInterface where I would outsource the code the current DynamicRouter does to add more information to the route match array.
* probably we should outsource the generator from DynamicRouter as well then and just inject it so you can use your drupal generator with it.

RE only chaining the matcher: are you saying you can't use the cmf ChainRouter anymore, even though it now matches Requests instead of URL? i think we should keep with the model of chaining full routers. but with the DynamicRouter refactored, you could use one DynamicRouter to generate routes and others to match routes.

once we are clear what exactly needs to be done, I can also do things, you don't need to PR it all if you have no time. but before i start changing the code, we need to agree on how we do them :-)

effulgentsia’s picture

#25 doesn't apply to HEAD any more. Can we get an updated patch posted? Thanks.

katbailey’s picture

FileSize
83.21 KB

Here's a reroll, including some minor docblock fixes.
I can't do much else for this issue until I've done a bunch of reading about ChainRouters and things...

katbailey’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1606794-routing.45.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
83.21 KB
pounard’s picture

+    foreach (module_implements('route_info') as $module) {
+      $routes = call_user_func($module . '_route_info');
+      drupal_alter('router_info', $routes);
+      $dumper->addRoutes($routes);
+      $dumper->dump(array('route_set' => $module));
+    }

Seems weird to me, are you calling the 'route_info_alter' hook for each module implementing 'route_info' without specifying from which module it comes from in signature when invoking it? This means that, as a module developer, I cannot easily spot the route I want to alter.

+function router_rebuild() {
+  // We need to manually call each module so that we can know which module
+  // a given item came from.
+  $callbacks = array();

The $callback array is unused here.

I won't do a design review I don't know Symfony routing at all, so I trust you for the rest :)

Crell’s picture

Good call on both points in #49. I'll include those in my next new-functionality patch. (Coming this weekend, since it's a 3 day weekend. yay!)

Crell’s picture

FileSize
94.65 KB

OK, new patch!

This version does the following, in no special order:

1) Addresses the points raised above by catch and pounard.

2) Makes ContainerAware controllers automagically get a container injected into them. I may split that off to a separate patch since it's nice and self-contained, but it's here for now.

3) Rips out the generator code entirely for now.

4) Implements the "wrapping controller" concept as described in the Munich writeup. Any HTML page going through the new router now is, in fact, using a _content key, not _controller. _controller is provided as a default. That default, moreover, is ripe for replacement by SCOTCH. :-)

5) Add support for placeholders in paths, which, duh, I'd broken. :-) Mostly it's done via copy and paste of the regex code that Symfony uses, since such regexes are way above my level of competency.

6) Some other cleanup and refactoring. And oh yeah, it's all heavily unit tested.

While doing the above, I realized that, duh, I'd screwed up default placeholder values in the new path matcher. I've asked chx to help me unbork that, but I wanted to get the current work up for people to see. (It's also still in the router branch of the wscci sandbox, of course.) I also still need to add access control for the new paths, as we discussed in Munich.

dbu: I think at this point the best course of action is to plow ahead here as is, and then you and I can talk at Symfony Live SF in person (assuming you'll be there, I hope?) to see what the best way to push stuff upstream is. We can refactor stuff upstream after this is in more easily than we can stall other work on this patch. ;-)

Status: Needs review » Needs work

The last submitted patch, 1606794-routing.patch, failed testing.

effulgentsia’s picture

I'm about to be away for a week, so wanted to leave some initial thoughts here.

I haven't read #51 in-depth, so can't nitpick every line of code and docs, but at an architectural level, I think the patch is very good. I don't quite understand RouteCompiler and CompiledRoute yet, but I'm willing to trust others on reviewing that (or I will dig into it more when I get back if this hasn't been committed by then). It also seems like there's a lot here that should go upstream to Symfony, but per #51, Crell and dbu are working on that, and we shouldn't hold up getting this into Drupal for that.

I also haven't benchmarked the performance implications of this routing implementation, but it's hard to do so in a meaningful way without actually converting a whole bunch of our hook_menu() implementations into hook_route_info(), which this patch doesn't do. Unless someone has ideas for how to do meaningful performance analysis without holding this patch up on a lot of conversion work, I suggest just proceeding without that, and addressing performance issues in follow-ups.

+++ b/core/lib/Drupal/Core/ControllerResolver.php
@@ -0,0 +1,60 @@
+  protected function createController($controller) {
+    if (false === strpos($controller, '::')) {
+      throw new \InvalidArgumentException(sprintf('Unable to find controller "%s".', $controller));
+    }
+
+    list($class, $method) = explode('::', $controller, 2);
+
+    if (!class_exists($class)) {
+      throw new \InvalidArgumentException(sprintf('Class "%s" does not exist.', $class));
+    }
+
+    $controller = new $class();
+    if ($controller instanceof ContainerAwareInterface) {
+      $controller->setContainer($this->container);
+    }
+
+    return array($controller, $method);
+  }

Can the implementation of this be changed to call parent::createController() and then $result[0]->setContainer() rather than duplicating the whole function?

+++ b/core/lib/Drupal/Core/CoreBundle.php
@@ -52,12 +55,25 @@ class CoreBundle extends Bundle
     // @todo Replace below lines with the commented out block below it when it's
     //   performant to do so: http://drupal.org/node/1706064.
     $dispatcher = $container->get('dispatcher');
-    $matcher = new \Drupal\Core\LegacyUrlMatcher();
+    $matcher = new \Drupal\Core\Routing\ChainMatcher();
+    $matcher->add(new \Drupal\Core\LegacyUrlMatcher());
+
+    $nested = new \Drupal\Core\Routing\NestedMatcher();
+    $nested->setInitialMatcher(new \Drupal\Core\Routing\PathMatcher(Database::getConnection()));
+    $nested->addPartialMatcher(new \Drupal\Core\Routing\HttpMethodMatcher());
+    $nested->setFinalMatcher(new \Drupal\Core\Routing\FirstEntryFinalMatcher());
+    $matcher->add($nested, 5);
+
     $content_negotation = new \Drupal\Core\ContentNegotiation();
-    $dispatcher->addSubscriber(new \Drupal\Core\EventSubscriber\RouterListener($matcher));
+    $dispatcher->addSubscriber(new \Symfony\Component\HttpKernel\EventListener\RouterListener($matcher));
     $dispatcher->addSubscriber(new \Drupal\Core\EventSubscriber\ViewSubscriber($content_negotation));
     $dispatcher->addSubscriber(new \Drupal\Core\EventSubscriber\AccessSubscriber());
     $dispatcher->addSubscriber(new \Drupal\Core\EventSubscriber\MaintenanceModeSubscriber());
@@ -69,6 +85,11 @@ class CoreBundle extends Bundle
     $dispatcher->addSubscriber(new \Drupal\Core\EventSubscriber\ConfigGlobalOverrideSubscriber());
     $container->set('content_negotiation', $content_negotation);
     $dispatcher->addSubscriber(\Drupal\Core\ExceptionController::getExceptionListener($container));
+
+    $route_subscriber = new \Drupal\Core\EventSubscriber\RouteProcessorSubscriber();
+    $route_subscriber->setContainer($container);
+    $dispatcher->addSubscriber($route_subscriber);
+

Can we also update the commented out block that follows the above hunks, so that the @todo stays true.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcher.php
@@ -0,0 +1,149 @@
+    $collection = $this->initialMatcher->matchRequestPartial($request);
+
+    foreach ($this->partialMatchers as $matcher) {

sdboyer, Crell, and I discussed this at DrupalCon Munich and are concerned at the idea of running all partial matchers on every request. For this patch, we only have one partial matcher, so it's ok, but SCOTCH may need to add a bunch more. I'm ok with punting that problem to a follow-up though.

+++ b/core/modules/system/system.install
@@ -1279,6 +1279,125 @@ function system_schema() {
+  $schema['registry'] = array(
...
+  $schema['registry_file'] = array(

I hope this is a merge error, and that you're not actually trying to bring back the registry :)

+++ b/core/modules/system/tests/modules/router_test/router_test.module
@@ -0,0 +1,28 @@
+function router_test_route_info() {
+  $collection = new RouteCollection();
+
+  $route = new Route('router_test/test1', array(
+    '_controller' => '\Drupal\router_test\TestControllers::test1'
+  ));
+  $collection->add('router_test_1', $route);
+
+  $route = new Route('router_test/test2', array(
+    '_content' => '\Drupal\router_test\TestControllers::test2'
+  ));
+  $collection->add('router_test_2', $route);
+
+  $route = new Route('router_test/test3/{value}', array(
+    '_content' => '\Drupal\router_test\TestControllers::test3'
+  ));
+  $collection->add('router_test_3', $route);
+
+  return $collection;
+}

Since this is our only hook_route_info() implementation at present, can we also add examples of specifying http method requirements? Better yet, can we add hook_route_info() documentation to system.api.php and do that there?

Crell’s picture

Status: Needs work » Needs review

Can the implementation of this be changed to call parent::createController() and then $result[0]->setContainer() rather than duplicating the whole function?

Possibly. I copied the whole function because that's what FrameworkBundle does; it's virtually identical code. I don't really have a preference here.

Can we also update the commented out block that follows the above hunks, so that the @todo stays true.

Well now you're asking me to learn the post-compile mechanism too! :-P Remind me of that once we're done messing with the DIC so that I only have to copy it down once. (Or get #1706064: Compile the Injection Container to disk committed before this is and force the issue.)

I hope this is a merge error, and that you're not actually trying to bring back the registry :)

Eeek! Merge error. O_o

Since this is our only hook_route_info() implementation at present, can we also add examples of specifying http method requirements? Better yet, can we add hook_route_info() documentation to system.api.php and do that there?

Once we figure out what that full API actually looks like, yes. I'm just adding tests there that actually work, as I go. I'm also considering moving from a hook to something more Symfony-esque, possibly even in the bundle classes or with an event. I'm open to suggestions there, but working with it the hook feels very out of place in this system.

Crell’s picture

I've forked ContainerAware controllers off to its own issue: #1777430: Allow for ContainerAware controllers

Once that lands I'll rebase that code out of this branch.

Crell’s picture

FileSize
92.72 KB

OK, let's try this again. Attached patch addresses the feedback from #53, and I think fixes the bug that was causing those test failures. Or at least some of them. :-) Bot?

Status: Needs review » Needs work

The last submitted patch, 1606794-routing.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
FileSize
101.44 KB

This should resolve the issue of optional placeholders, as well as another bug I introduced in subrequests. Bot, what else did I break?

Status: Needs review » Needs work

The last submitted patch, 1606794-routing.patch, failed testing.

Crell’s picture

We're blocked on #1777430: Allow for ContainerAware controllers. I can't rebase properly until that gets committed. :-(

Crell’s picture

Status: Needs work » Needs review
FileSize
98.73 KB

OK, one more time, and I go to bed...

Status: Needs review » Needs work

The last submitted patch, 1606794-routing.patch, failed testing.

pounard’s picture

Go to bed :)

cosmicdreams’s picture

Yea! looks like #1777430: Allow for ContainerAware controllers was committed!

cosmicdreams’s picture

Status: Needs work » Needs review
Issue tags: -API change, -WSCCI, -symfony, -wscci-hitlist

#61: 1606794-routing.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +API change, +WSCCI, +symfony, +wscci-hitlist

The last submitted patch, 1606794-routing.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
91.67 KB

Reroll against latest 8.x. This is from my routing-disasm on Crell's sandbox.

I haven't figured out exactly what's causing the tests to fail. But here's some information I did figure out:

I took apart one of the simplest test failures, The PageNotFoundTest. I found that when no custom error page is specified, it behaves correctly. However; as soon as you set a custom 404 page, it throws a NotFoundHttpException that is never caught. Oddly though, it then throws an UnexpectedValueException: The Response content must be a string or object implementing _toString(), "array" given.

The one thing that appears to be a similar warning in all the failed test I've looked at is invalid index backtrace.

This is happening in includes/errors.inc: _drupal_log_error. I'm still digging, but I think if we can find the source of the backtrace not being set in _drupal_log_error, we'll fix the majority of the test failures.

Marking issue needs review to trigger testbot.

Status: Needs review » Needs work

The last submitted patch, drupal-routing-1606794-67.patch, failed testing.

podarok’s picture

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -53,12 +55,25 @@ class CoreBundle extends Bundle
+    $container->register('router.dumper', '\Drupal\Core\Routing\MatcherDumper')
+      ->addArgument(new Reference('database'));
+    $container->register('router.builder', 'Drupal\Core\Routing\RouteBuilder')
+      ->addArgument(new Reference('router.dumper'));
+
     // @todo Replace below lines with the commented out block below it when it's
     //   performant to do so: http://drupal.org/node/1706064.
     $dispatcher = $container->get('dispatcher');
-    $matcher = new \Drupal\Core\LegacyUrlMatcher();
+    $matcher = new \Drupal\Core\Routing\ChainMatcher();
+    $matcher->add(new \Drupal\Core\LegacyUrlMatcher());
+
+    $nested = new \Drupal\Core\Routing\NestedMatcher();
+    $nested->setInitialMatcher(new \Drupal\Core\Routing\PathMatcher(Database::getConnection()));
+    $nested->addPartialMatcher(new \Drupal\Core\Routing\HttpMethodMatcher());
+    $nested->setFinalMatcher(new \Drupal\Core\Routing\FirstEntryFinalMatcher());
+    $matcher->add($nested, 5);
+
     $content_negotation = new \Drupal\Core\ContentNegotiation();
-    $dispatcher->addSubscriber(new \Drupal\Core\EventSubscriber\RouterListener($matcher));
+    $dispatcher->addSubscriber(new \Symfony\Component\HttpKernel\EventListener\RouterListener($matcher));
     $dispatcher->addSubscriber(new \Drupal\Core\EventSubscriber\ViewSubscriber($content_negotation));
     $dispatcher->addSubscriber(new \Drupal\Core\EventSubscriber\AccessSubscriber());
     $dispatcher->addSubscriber(new \Drupal\Core\EventSubscriber\MaintenanceModeSubscriber());
@@ -71,6 +86,10 @@ class CoreBundle extends Bundle

@@ -71,6 +86,10 @@ class CoreBundle extends Bundle
     $container->set('content_negotiation', $content_negotation);

possibly better
use \Drupal\Core\EventSubscriber\
use \Symfony\Component\HttpKernel\EventListener\
use \Drupal\Core\Routing\
use \Drupal\Core\

for make this part of code more human friendly?

effulgentsia’s picture

Re #69, in #1323082-53: Establish standards for namespace usage [no patch], I had suggested "use"ing namespaces rather than full class names, but that was rejected. Please comment on that issue if you want to make new arguments for it, but given that in #1567920: Naming standard for abstract/base classes, we agreed to have class names that are generally unambiguous even when appearing in code without their namespace, I no longer see much value to it.

But, you're correct that under normal circumstances, the code block you reference should have their (full) class names "use"d at the top. The reason they aren't in this case is that per the @todo, once we fix #1706064: Compile the Injection Container to disk, those classes will be supplied to $container->register() as string arguments, which can't benefit from a "use".

Crell’s picture

disasm: Hm, sorry. I had new code that I'd not pushed to the routing branch yet. :-( I just resynced it. Can you rebase your branch against the new routing branch, and I can try to have a look? Thanks.

Anonymous’s picture

FileSize
42.27 KB
4.6 KB

in debugging, I looked at the UserRegistrationTest, NodeBlockFunctionalTest, and ColorTest.

The common denominator was failure after requesting the front page. Instead of getting a full HTML page, they just get one part, the profile information.

When I switched the path from '<front>' to 'admin/people' on line 89 of ColorTest, the test passed.


Example Pass:

<hr />ID #16 (<a href="Drupal_node_Tests_NodeBlockFunctionalTest-15.html">Previous</a> | <a href="Drupal_node_Tests_NodeBlockFunctionalTest-17.html">Next</a>)<hr />GET request to: <hr />Ending URL: http://localhost/d8entity/<hr /><!DOCTYPE html>
<html lang="en" dir="ltr">
........
  <div class="content">
    <article class="profile" class="user-profile">
  <div class="form-item form-type-item">
  <label>Member for </label>
 40 sec
</div>
</article>
  </div>
.....

Example fail:

<hr />ID #16 (<a href="Drupal_node_Tests_NodeBlockFunctionalTest-15.html">Previous</a> | <a href="Drupal_node_Tests_NodeBlockFunctionalTest-17.html">Next</a>)<hr />GET request to: <hr />Ending URL: http://localhost/d8entity/<hr /><article class="profile" class="user-profile">
  <div class="form-item form-type-item">
  <label>Member for </label>
 52 sec
</div>
</article>
Crell’s picture

I suspect that's because /user is b0rked. The code for that page callback right now is doing a manual call to $kernel->handle() with a new subrequest, which doesn't work with the new view subscriber code. I've tried switching it to a $kernel->forward() call, which works but then the tabs don't appear on the page (which is just bizarre). I've also tried switching it to just send a redirect to user/$uid, which is really what it should have been all along, but that's causing multiple "request returned 0 bytes" errors in simpletest. I thought that's because simpletest is not handling the redirect properly, but chx tells me that it *should* be working.

If someone knows simpletest well, I could definitely use some help here...

Anonymous’s picture

I simplified the color test so that it was just a call to drupalGet(''), not having a user logged in or anything. This still results in user_page being called.

Do we intend to call user_page on every request to the front page?

EDIT: catch clarified that the testing profile sets user_page as the front page.

pounard’s picture

@Crell the forward approach seems the best, maybe the menu bug due to the forwarding should be fixed? It sounds like something that needs fixing anyway because contrib (or some other pieces of core) will probably do that someday.

moshe weitzman’s picture

The D7 code for /user did a menu_execute_active_handler('user/N'). I think the equivalent in Synfony is a subrequest, and thats how the code has been ported. If this patch regresses to the point where we can't do stuff that was easy in prior versions of Drupal, thats a problem. I would rather not paint over the problem with a 302 redirect. At minimum, we should understand whats going on in the kernel.

Crell’s picture

Moshe: /user magically being the same as /user/$id is, was, and has always been a bad idea. Fixing that to be a redirect is, IMO, RESTfully correct anyway. That it makes simpletest cry is a separate question. It works fine in a browser.

I agree we need subrequests to work correctly, but I'm not sure how much effort to put into making them work correctly right now when we want to change that approach for SCOTCH anyway, which this issue is the last blocker for. So I'm all for shortest-route-to-green here.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
99.43 KB

So I tried Crell's attempted solution, using a simple redirect (using drupal_goto). It fixed the one test that I was running, and I didn't get any "request returned 0 bytes" errors. I'm going to try it in the testbot to see if I'm missing something.

Status: Needs review » Needs work

The last submitted patch, 1606794-78-routing.patch, failed testing.

Anonymous’s picture

Ok, so that's down from 244 to 172 fails. From a cursory look, it doesn't introduce any new fails.

Are we ok with a redirect such as drupal_goto('user/' . $user->uid); as a temporary stopgap solution to the /user callback issue?

moshe weitzman’s picture

I'm OK with a redirect as a final solution, but I'm not with breaking subrequests. Until we have a test for subrequests, the /user page should not redirect IMO.

Crell’s picture

The tests for new-style controllers/content-callback-things are all testing subrequests already, because they're always using a subrequest inside HtmlPageController. If there's some other test you think is missing please look at the patch and specify what you think is missing. "Subrequests work" is too vague a thing to test.

Crell’s picture

I figured out the redirect problem, thanks to Lin's check. I was using RedirectResponse, not drupal_goto(), and was missing the full domain name. My browser handled that fine, but simpletest is more temperamental and didn't handle that properly. I fixed that now. It's using RedirectResponse, but correctly this time. Lin, I made sure you had a commit credit in there. :-)

Moshe is still investigating why the tabs weren't appearing with the subrequest approach, on the theory that it's probably a sign of another bug somewhere.

disasm: I looked at your branch in the repository, and there were no new commits on it. :-(

moshe weitzman’s picture

Thanks to Lin for implementing that redirect. I'm happy with that solution. The subrequest problem that I was worried about before turns out to be #1790366: drupal_static() should be request specific. Lets keep fixing tests here and work toward a speedy commit.

pounard’s picture

I'm happy with the redirect as long as we are in development phase, but we should move to the forward method before D8 get stable, because redirects on oftenly accessed logged in pages are performance killers.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
99.44 KB

Very possible that this breaks a host of other things, but it fixes the comment test on my machine.

I changed one occurance of drupal_render to drupal_render_page in ViewSubscriber.

Status: Needs review » Needs work

The last submitted patch, 1606794-86-routing.patch, failed testing.

Anonymous’s picture

Just for research purposes, switching to drupal_render_page didn't add any new fails and fixed the following tests:

  • Drupal\comment\Tests\CommentBlockTest
  • Drupal\system\Tests\Menu\TrailTest
  • Drupal\system\Tests\System\AccessDeniedTest
  • Drupal\system\Tests\System\PageNotFoundTest
  • Drupal\system\Tests\Update\UpdateScriptTest
Anonymous’s picture

Status: Needs work » Needs review
FileSize
100.26 KB

Since we use a redirect instead of a subrequest to show the user page at /user, we need to change the menu router test for that.

This will clear up all fails that aren't related to update/upgrade.

Status: Needs review » Needs work

The last submitted patch, 1606794-89-routing.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
100.68 KB

Crell pointed out that I hadn't rebased my branch to include his RedirectResponse changes from last night. Those changes don't affect the fail in Drupal\system\Tests\Menu\RouterTest, so this patch rebases to include his changes and maintains the change from the last patch.

--- a/core/modules/system/lib/Drupal/system/Tests/Menu/RouterTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/RouterTest.php
@@ -178,7 +178,7 @@ class RouterTest extends WebTestBase {

-    $this->assertTrue($this->url == url('user', array('absolute' => TRUE)), t("Logged-in user redirected to user on accessing user/login"));
+    $this->assertTrue($this->url == url('user/' . $this->loggedInUser->uid, array('absolute' => TRUE)), t("Logged-in user redirected to user on accessing user/login"));

Status: Needs review » Needs work

The last submitted patch, 1606794-91-routing.patch, failed testing.

Anonymous’s picture

Many of the remaining fails are due to the following error: The service definition "router.builder" does not exist. This happens in upgrade tests.

Crell pointed out that this is probably related to #1708692: Bundle services aren't available in the request that enables the module. Moving router.builder out of the DIC for now with a @todo may be a solution.

Anonymous’s picture

katbailey’s picture

To clarify re #94, it sounds like the problem is that the "bootstrap container" is being used somewhere instead of the kernel-built container (which has the router.builder service) - so the issues above are about 1) figuring out what the heck the bootstrap container is for, 2) getting to the point where it is hardly ever used, and 3) precompiling it once we have both of those nailed down.

An interim measure for now would be to add the router.builder service definition to the bootstrap container, i.e. inside the drupal_container() function. If I get a chance this evening I'll investigate whether this fixes the problem being encountered here.

effulgentsia’s picture

Status: Needs work » Needs review
diff --git a/core/modules/system/lib/Drupal/system/FileDownload.php b/core/modules/system/lib/Drupal/system/FileDownload.php
new file mode 100644
index 0000000..b55872c
--- /dev/null
+++ b/core/modules/system/lib/Drupal/system/FileDownload.php
@@ -0,0 +1,54 @@
+<?php
+
+namespace Drupal\system;
+
+use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
+use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
+
+/**
+ * Controller class for private file downloads.
+ */
+class FileDownload {

Is this new class used for anything? I don't see anything in the patch that references it.

effulgentsia’s picture

Status: Needs review » Needs work
effulgentsia’s picture

FileSize
2.17 KB
101.83 KB

The update test failures can be also be replicated by just visiting update.php and clicking "Continue" (even if no updates are needed). The problem is that update_script_selection_form() (correctly) calls drupal_flush_all_caches() which must then trigger a router rebuild.

Moving the router.rebuild service to the bootstrap container is problematic, because it depends on the dumper which depends on the database, so we'd need to add the database service there too, which takes us in the opposite direction as #1784312: Stop doing so much pre-kernel bootstrapping.

Instead, how about this?

effulgentsia’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1606794-98-routing.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.1 KB
101.65 KB

Ok, that failed. Here's another approach. interdiff is relative to #91.

Status: Needs review » Needs work

The last submitted patch, 1606794-101-routing.patch, failed testing.

Anonymous’s picture

It turns out if you switch back to drupal_goto for the user_page redirect, the BlockLanguageTest passes. I haven't figured out why there's a difference between the two in that single test.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
518 bytes
101.66 KB

Because failing to pass the path to url() meant it wasn't getting the language prefix.

Anonymous’s picture

Status: Needs review » Needs work

The issue here is that url has some extra logic that uses the current language to form the URL. So in the test, it results in the url http://localhost/d8entity/en/user/2.

$request->getUriForPath doesn't do this. Thus, the redirect to /user/$uid is reverting to a non-language-specific version of the page, and the language-specific block is not showing up.

Anonymous’s picture

Status: Needs work » Needs review

Whoops, crosspost

Status: Needs review » Needs work

The last submitted patch, 1606794-104-routing.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
102.8 KB

This should remove the remaining exceptions too. Will this be green? In case yes, here's the total interdiff from #91 in order to help sync up the sandbox.

Anonymous’s picture

The patch in #91 used drupal_render_page for subrequests, which is how I got the tests in #88 to pass. This should be revisited before it gets committed.

Lars Toomre’s picture

This is a review of the green patch in #108 focused on coding and documentation standards. I am unable to provide a patch that includes corrections for these points at present. Hopefully, someone else can.

+++ b/core/includes/common.incundefined
@@ -4,6 +4,7 @@ use Drupal\Component\Utility\NestedArray;
 use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
 use Drupal\Core\Cache\CacheBackendInterface;
+use Symfony\Component\DependencyInjection\Container;
 use Drupal\Core\Database\Database;

Is there any reason why this use statement was placed there? I was under impression multiple use statements should be in alphabetical order.

+++ b/core/lib/Drupal/Core/EventSubscriber/RouteProcessorSubscriber.phpundefined
@@ -0,0 +1,48 @@
+  /**
+   * Sets a default controller for a route if one was not specified.
+   */
+  public function onRequestSetController(GetResponseEvent $event) {

This needs a @param directive.

+++ b/core/lib/Drupal/Core/HtmlPageController.phpundefined
@@ -0,0 +1,63 @@
+   * @param type $_content
+   *   The body content callable that contains the body region of this page.
+   * @return \Symfony\Component\HttpFoundation\Response

Should be blank line before @return.

+++ b/core/lib/Drupal/Core/Routing/ChainMatcher.phpundefined
@@ -0,0 +1,159 @@
+   * Array of RequestMatcherInterface objects, sorted.
+   * @var type

Blank line needed before @var.

+++ b/core/lib/Drupal/Core/Routing/ChainMatcher.phpundefined
@@ -0,0 +1,159 @@
+    * @param int $priority
+    *   The priority of the matcher. Higher number matchers will be checked
+    *   first.

Should start with '(optional)' and state what the default value is.

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.phpundefined
@@ -0,0 +1,163 @@
+
+  protected $variables;
+  protected $tokens;
+  protected $staticPrefix;
+  protected $regex;

Each of these members need a docblock.

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.phpundefined
@@ -0,0 +1,163 @@
+   *
+   * @param Route  $route
+   *   A original Route instance.

Extra space before $route.

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.phpundefined
@@ -0,0 +1,163 @@
+  /**
+   * Returns the fit of this route

Missing period at end of line.

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.phpundefined
@@ -0,0 +1,163 @@
+    *
+    * @return array The options

'The options' belong on the next line after @return.

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.phpundefined
@@ -0,0 +1,163 @@
+    *
+    * @return array The defaults

ibid and same with @return in next function.

+++ b/core/lib/Drupal/Core/Routing/FinalMatcherInterface.phpundefined
@@ -0,0 +1,34 @@
+   * @return array
+   *   An array of parameters

Missing period after parameters.

+++ b/core/lib/Drupal/Core/Routing/FirstEntryFinalMatcher.phpundefined
@@ -0,0 +1,76 @@
+   *
+   * @param array $params
+   *  The parameters
+   * @param array $defaults
+   *   The defaults
+   *
+   * @return array
+   *   Merged default parameters

Missing periods at the end of each description.

+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.phpundefined
@@ -0,0 +1,163 @@
+
+  public function __construct(Connection $connection, $table = 'router') {
+    $this->connection = $connection;

Doesn't this constructor require a docblock that explains its arguments?

+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.phpundefined
@@ -0,0 +1,163 @@
+   *
+   * Available options:
+   *
+   *  * route_set:  The route grouping that is being dumped. All existing
+   *     routes with this route set will be deleted on dump.
+   *  * base_class: The base class name
+   *
+   * @param $options array
+   *   $options An array of options

Please eliminate the blank line after available options. List items should be a dash instead of *; also columns need to be adjusted. Finally @param directive needs to be cleaned up.

+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.phpundefined
@@ -0,0 +1,163 @@
+
+    //$compiled = $this->compileRoutes($this->routes, $route_set);

Should this be removed? Or if kept, add comment with @todo about why.

+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.phpundefined
@@ -0,0 +1,163 @@
+        // back downstream.
+       'route' => serialize(clone($route)),

Off by one space of indentation.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcher.phpundefined
@@ -0,0 +1,149 @@
+  /**
+   * The final matcher

Missing a final period.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcher.phpundefined
@@ -0,0 +1,149 @@
+   *
+   * @var RequestMatcherInterface

I think coding standards call for this to include the path details.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcher.phpundefined
@@ -0,0 +1,149 @@
+   * @param PartialMatcherInterface $matcher
+   *   A partial

Missing a final period.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcher.phpundefined
@@ -0,0 +1,149 @@
+    *
+    * @param Request $request The request to match
+    *
+    * @return array An array of parameters

Change to Drupal style @param directive (explanation on next line).

+++ b/core/lib/Drupal/Core/Routing/PathMatcher.phpundefined
@@ -0,0 +1,133 @@
+ */
+class PathMatcher implements InitialMatcherInterface {

I think there should be a blank line after the class declaration.

+++ b/core/lib/Drupal/Core/Routing/PathMatcher.phpundefined
@@ -0,0 +1,133 @@
+   *   The parts of the path for which we want candidates.
+   * @return array
+   *   An array of outlines that could match the specified path parts.

Missing blank line before @return.

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.phpundefined
@@ -0,0 +1,35 @@
+
+  protected $dumper;
+
+  public function __construct(MatcherDumperInterface $dumper) {

Missing docblock.

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.phpundefined
@@ -0,0 +1,35 @@
+
+  public function rebuild() {
+    // We need to manually call each module so that we can know which module
+    // a given item came from.

Missing docblock.

+++ b/core/lib/Drupal/Core/Routing/RouteCompiler.phpundefined
@@ -0,0 +1,239 @@
+   * Utility constant to use for regular expressions against the path.
+*/
+  const REGEX_DELIMITER = '#';

Missing three spaces before '*/'.

+++ b/core/lib/Drupal/Core/Routing/RouteCompiler.phpundefined
@@ -0,0 +1,239 @@
+  /**
+    * Compiles the current route instance.

Correct indentation of this docblock.

+++ b/core/lib/Drupal/Core/Routing/RouteCompiler.phpundefined
@@ -0,0 +1,239 @@
+    * @param \Symfony\Component\Routing\Route $route
+    *   A Route instance
+    *
+    * @return CompiledRoute
+    *   A CompiledRoute instance

Missing trailing periods. Also CompiledRoute should have path information.

+++ b/core/lib/Drupal/Core/Routing/RouteCompiler.phpundefined
@@ -0,0 +1,239 @@
+   *   The pattern for which we want a matching regex.
+   * @return type
+   * @throws \LogicException

Need blank lines before both @return and @throws directives.

+++ b/core/lib/Drupal/Core/Routing/RouteCompiler.phpundefined
@@ -0,0 +1,239 @@
+  /**
+    * Computes the regexp used to match a specific token. It can be static text or a subpattern.

Again docblock indentation is off by a column.

+++ b/core/lib/Drupal/Core/Routing/RouteCompiler.phpundefined
@@ -0,0 +1,239 @@
+    *
+    * @param array   $tokens        The route tokens
+    * @param integer $index         The index of the current token
+    * @param integer $firstOptional The index of the first optional token
+    *
+    * @return string The regexp pattern for a single token

Change directives to Drupal coding style including trailing periods.

+++ b/core/lib/Drupal/Core/Routing/RouteCompiler.phpundefined
@@ -0,0 +1,239 @@
+  private function computeRegexp(array $tokens, $index, $firstOptional) {
+      $token = $tokens[$index];
+      if ('text' === $token[0]) {
+          // Text tokens
+          return preg_quote($token[1], self::REGEX_DELIMITER);
+      } else {
+          // Variable tokens
+          if (0 === $index && 0 === $firstOptional) {
+              // When the only token is an optional variable token, the separator is required
+              return sprintf('%s(?<%s>%s)?', preg_quote($token[1], self::REGEX_DELIMITER), $token[3], $token[2]);
+          } else {
+              $regexp = sprintf('%s(?<%s>%s)', preg_quote($token[1], self::REGEX_DELIMITER), $token[3], $token[2]);
+              if ($index >= $firstOptional) {
+                  // Enclose each optional token in a subpattern to make it optional.
+                  // "?:" means it is non-capturing, i.e. the portion of the subject string that
+                  // matched the optional subpattern is not passed back.
+                  $regexp = "(?:$regexp";
+                  $nbTokens = count($tokens);
+                  if ($nbTokens - 1 == $index) {
+                      // Close the optional subpatterns
+                      $regexp .= str_repeat(")?", $nbTokens - $firstOptional - (0 === $firstOptional ? 1 : 0));
+                  }
+              }
+
+              return $regexp;
+          }
+      }
+  }

Indention is wrong here. Appears to be four spaces instead of two. Please adjust.

+++ b/core/lib/Drupal/Core/Routing/RouteCompiler.phpundefined
@@ -0,0 +1,239 @@
+   *
+   * @param \Symfony\Component\Routing\Route $route

Missing @param explanation.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/ChainMatcherTest.phpundefined
@@ -0,0 +1,111 @@
+ */
+class ChainMatcherTest extends UnitTestBase {
+  public static function getInfo() {

Missing blank line after class declaration.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/FirstEntryFinalMatcherTest.phpundefined
@@ -0,0 +1,116 @@
+  }
+  public function setUp() {
+    parent::setUp();

Missing blank line.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/HttpMethodMatcherTest.phpundefined
@@ -0,0 +1,109 @@
+  }
+  public function setUp() {
+    parent::setUp();

Another missing blank line.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/MockMatcher.phpundefined
@@ -0,0 +1,35 @@
+
+  protected $matcher;

Missing docblock.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/MockPathMatcher.phpundefined
@@ -0,0 +1,50 @@
+ *
+ * @author crell

I think this should be removed.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/MockPathMatcher.phpundefined
@@ -0,0 +1,50 @@
+
+  protected $routes;
+
+  public function __construct(RouteCollection $routes) {
+    $this->routes = $routes;

Missing docblocks.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/NestedMatcherTest.phpundefined
@@ -0,0 +1,69 @@
+  }
+  public function setUp() {
+    parent::setUp();

Another missing blank line.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/PathMatcherTest.phpundefined
@@ -0,0 +1,304 @@
+
+   parent::tearDown();

Wrong indentation; needs another space.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/PathMatcherTest.phpundefined
@@ -0,0 +1,304 @@
+
+    //debug($candidates);

Please remove.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/PathMatcherTest.phpundefined
@@ -0,0 +1,304 @@
+  /**
+   * Confirm that an exception is thrown when no matching path is found.

Should be 'Confirms'.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RouteTest.phpundefined
@@ -0,0 +1,61 @@
+    // Because "here" has a default value, it should not factor into the
+    // outline or the fitness.
+    $route = new Route('/test/{something}/more/{here}', array(

Rewrap to better fit 80 characters.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RoutingFixtures.phpundefined
@@ -0,0 +1,167 @@
+
+  public function createTables(Connection $connection) {
+    $tables = $this->routingTableDefinition();

Missing docblock.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RoutingFixtures.phpundefined
@@ -0,0 +1,167 @@
+
+  public function dropTables(Connection $connection) {
+    $tables = $this->routingTableDefinition();

Missing docblock.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RoutingFixtures.phpundefined
@@ -0,0 +1,167 @@
+
+  public function routingTableDefinition() {

Missing docblock.

larowlan’s picture

Assigned: Crell » larowlan

working on coding standards (as we speak) then will assign back to @Crell

larowlan’s picture

Assigned: larowlan » Crell
FileSize
104.77 KB
26.31 KB
larowlan’s picture

patch at 112 fixes all concerns from #110 except

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.phpundefined
@@ -0,0 +1,163 @@
+
+  protected $variables;
+  protected $tokens;
+  protected $staticPrefix;

I can't see where these are used so are unable to document them.

Lars Toomre’s picture

Thanks for the updated patch @larowlan.

In the interim, I tried to find documentation on ordering of use statements. I could not find any. However, checking some other code, it appears that alphabetical is common. Can you possibly adjust the patch to treat all of the groups of use statements in a consistent manner?

I will take a second look at your patch to see if there are other documentation or coding standards that we should address.

Lars Toomre’s picture

This is a further review of the green patch in #112 focused on coding and documentation standards (through the start of the tests). I am unable to provide a patch that includes corrections for these points at present. Hopefully again, someone else can.

+++ b/core/lib/Drupal/Core/HtmlPageController.phpundefined
@@ -0,0 +1,64 @@
+   *
+   * @var ContainerInterface

I think that this and other similar references to classes need the path as well.

+++ b/core/lib/Drupal/Core/HtmlPageController.phpundefined
@@ -0,0 +1,64 @@
+   *
+   * @param ContainerInterface $container
+   *   The service container this object should use.

Ibid.

+++ b/core/lib/Drupal/Core/HtmlPageController.phpundefined
@@ -0,0 +1,64 @@
+   *
+   * @param Request $request
+   *   The request object.
+   * @param type $_content

Ibid. Also just noticed type is not the right variable type for type hinting.

+++ b/core/lib/Drupal/Core/HtmlPageController.phpundefined
@@ -0,0 +1,64 @@
+   * @return \Symfony\Component\HttpFoundation\Response

Need a description here.

+++ b/core/lib/Drupal/Core/LegacyUrlMatcher.phpundefined
--- /dev/null
+++ b/core/lib/Drupal/Core/Routing/ChainMatcher.phpundefined

+++ b/core/lib/Drupal/Core/Routing/ChainMatcher.phpundefined
+++ b/core/lib/Drupal/Core/Routing/ChainMatcher.phpundefined
@@ -0,0 +1,160 @@

@@ -0,0 +1,160 @@
+<?php

Missing @file docblock at top of new file.

+++ b/core/lib/Drupal/Core/Routing/ChainMatcher.phpundefined
@@ -0,0 +1,160 @@
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\Routing\Matcher\RequestMatcherInterface;
+use Symfony\Component\Routing\Exception\ResourceNotFoundException;
+use Symfony\Component\Routing\Exception\RouteNotFoundException;
+use Symfony\Component\Routing\Exception\MethodNotAllowedException;
+use Symfony\Component\Routing\RequestContextAwareInterface;
+use Symfony\Component\Routing\RequestContext;

Another example of use statements seemingly in random order.

+++ b/core/lib/Drupal/Core/Routing/ChainMatcher.phpundefined
@@ -0,0 +1,160 @@
+   *
+   * @api

Is this needed? Seems like a stray directive.

+++ b/core/lib/Drupal/Core/Routing/ChainMatcher.phpundefined
@@ -0,0 +1,160 @@
+  /**
+   * Matches a request against all queued matchers.
+   *
+   * @param Request $request The request to match
+   *
+   * @return array An array of parameters
+   *
+   * @throws ResourceNotFoundException If no matching resource could be found
+   * @throws MethodNotAllowedException If a matching resource was found but the request method is not allowed

Needs to be changed to Drupal docblock format.

+++ b/core/lib/Drupal/Core/Routing/ChainMatcher.phpundefined
@@ -0,0 +1,160 @@
+    *
+    * @return \Symfony\Component\Routing\RequestMatcherInterface[]
+    *   An array of Matcher objects in the order they should be used.

I do not think trailing [] is appropriate here.

+++ b/core/lib/Drupal/Core/Routing/ChainMatcher.phpundefined
--- /dev/null
+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.phpundefined

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.phpundefined
+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.phpundefined
@@ -0,0 +1,171 @@

@@ -0,0 +1,171 @@
+<?php

Missing @file docblock for new file.

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.phpundefined
@@ -0,0 +1,171 @@
+  protected $variables;
+  protected $tokens;

Let's remove unused members then.

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.phpundefined
@@ -0,0 +1,171 @@
+   * @param Route $route
+   *   A original Route instance.

Route needs to be prefixed with path.

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.phpundefined
@@ -0,0 +1,171 @@
+   *  @param int $num_parts
+   *   The number of parts in the path.
+   *  @param string $regex
+   *   The regular expression to match placeholders out of this path.

@param directive lines are indented one space too many.

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.phpundefined
@@ -0,0 +1,171 @@
+    * @return Route
+    *   A Route instance.

@return Route needs a path prefix.

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.phpundefined
--- /dev/null
+++ b/core/lib/Drupal/Core/Routing/FinalMatcherInterface.phpundefined

+++ b/core/lib/Drupal/Core/Routing/FinalMatcherInterface.phpundefined
+++ b/core/lib/Drupal/Core/Routing/FinalMatcherInterface.phpundefined
@@ -0,0 +1,34 @@

@@ -0,0 +1,34 @@
+<?php

Needs @file docblock at top of new file.

+++ b/core/lib/Drupal/Core/Routing/FinalMatcherInterface.phpundefined
@@ -0,0 +1,34 @@
+   * @param RouteCollection $collection
+   *   The collection against which to match.
+   *
+   * @return FinalMatcherInterface
+   *   The current matcher.

Both RouteCollection and FinalMatcherInterface need path prefix.

+++ b/core/lib/Drupal/Core/Routing/FinalMatcherInterface.phpundefined
@@ -0,0 +1,34 @@
+   * @param Request $request
+   *   A Request object against which to match.

Request also needs path prefix.

+++ b/core/lib/Drupal/Core/Routing/FinalMatcherInterface.phpundefined
--- /dev/null
+++ b/core/lib/Drupal/Core/Routing/FirstEntryFinalMatcher.phpundefined

+++ b/core/lib/Drupal/Core/Routing/FirstEntryFinalMatcher.phpundefined
+++ b/core/lib/Drupal/Core/Routing/FirstEntryFinalMatcher.phpundefined
@@ -0,0 +1,77 @@

@@ -0,0 +1,77 @@
+<?php

Needs @file docblock for new file.

+++ b/core/lib/Drupal/Core/Routing/FirstEntryFinalMatcher.phpundefined
@@ -0,0 +1,77 @@
+   * @var RouteCollection

Needs a path prefix.

+++ b/core/lib/Drupal/Core/Routing/FirstEntryFinalMatcher.phpundefined
@@ -0,0 +1,77 @@
+   * @param RouteCollection $collection
+   *   The collection against which to match.
+   *
+   * @return FinalMatcherInterface
+   *   The current matcher.

Both need a path prefix.

+++ b/core/lib/Drupal/Core/Routing/FirstEntryFinalMatcher.phpundefined
--- /dev/null
+++ b/core/lib/Drupal/Core/Routing/HttpMethodMatcher.phpundefined

+++ b/core/lib/Drupal/Core/Routing/HttpMethodMatcher.phpundefined
+++ b/core/lib/Drupal/Core/Routing/HttpMethodMatcher.phpundefined
@@ -0,0 +1,53 @@

@@ -0,0 +1,53 @@
+<?php

Needs @file docblock for the new file.

+++ b/core/lib/Drupal/Core/Routing/HttpMethodMatcher.phpundefined
@@ -0,0 +1,53 @@
+   * @param Request $request
+   *   A Request object against which to match.
+   *
+   * @return RouteCollection
+   *   A RouteCollection of matched routes.

Needs path prefix on both Request and RouteCollection.

+++ b/core/lib/Drupal/Core/Routing/HttpMethodMatcher.phpundefined
--- /dev/null
+++ b/core/lib/Drupal/Core/Routing/InitialMatcherInterface.phpundefined

+++ b/core/lib/Drupal/Core/Routing/InitialMatcherInterface.phpundefined
+++ b/core/lib/Drupal/Core/Routing/InitialMatcherInterface.phpundefined
@@ -0,0 +1,22 @@

@@ -0,0 +1,22 @@
+<?php

Needs a @file docblock for new file. Just below need path prefixes for Request and RouteCollection.

+++ b/core/lib/Drupal/Core/Routing/InitialMatcherInterface.phpundefined
--- /dev/null
+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.phpundefined

+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.phpundefined
+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.phpundefined
@@ -0,0 +1,168 @@

@@ -0,0 +1,168 @@
+<?php

Need another @file docblock.

+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.phpundefined
@@ -0,0 +1,168 @@
+   * @param RouteCollection $routes

Needs path prefix.

+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.phpundefined
@@ -0,0 +1,168 @@
+   * - base_class: The base class name
+   *
+   * @param array $options
+   *   An array of options

Missing two final periods.

+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.phpundefined
@@ -0,0 +1,168 @@
+   * @return RouteCollection
+   *   A RouteCollection instance representing all routes currently in the

Needs path prefix.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcher.phpundefined
@@ -0,0 +1,150 @@
+   * @param PartialMatcherInterface $matcher
+   *   A partial.
+   *
+   * @return NestedMatcherInterface
+   *   The current matcher.

Both need path prefixes.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcher.phpundefined
@@ -0,0 +1,150 @@
+   * @param UrlMatcherInterface $final
+   *   The matcher that will be called last to ensure only a single route is

Needs path prefix.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcher.phpundefined
@@ -0,0 +1,150 @@
+   * @return NestedMatcherInterface
+   *   The current matcher.

Needs path prefix.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcher.phpundefined
@@ -0,0 +1,150 @@
+   * @param InitialMatcherInterface $matcher
+   *   An initial matcher.  It is responsible for its own configuration and
+   *   initial route collection
+   *
+   * @return NestedMatcherInterface
+   *   The current matcher.

Both need path prefixes.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcher.phpundefined
@@ -0,0 +1,150 @@
+    * @param Request $request
+    *   The request to match.

Needs path prefix.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcher.phpundefined
@@ -0,0 +1,150 @@
+    * @throws ResourceNotFoundException If no matching resource could be found
+    * @throws MethodNotAllowedException If a matching resource was found but the request method is not allowed

I think the explanation should go on the next line indented two spaces.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcher.phpundefined
@@ -0,0 +1,150 @@
+   *
+   * @param RequestContext $context The context

Needs both path prefix and formatting for Drupal standards. Include period at end.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcher.phpundefined
@@ -0,0 +1,150 @@
+   *
+   * @return RequestContext The context

Ibid.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcher.phpundefined
--- /dev/null
+++ b/core/lib/Drupal/Core/Routing/NestedMatcherInterface.phpundefined

+++ b/core/lib/Drupal/Core/Routing/NestedMatcherInterface.phpundefined
+++ b/core/lib/Drupal/Core/Routing/NestedMatcherInterface.phpundefined
@@ -0,0 +1,50 @@

@@ -0,0 +1,50 @@
+<?php

Needs @file docblock. Also many places in this file need path prefixes.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcherInterface.phpundefined
--- /dev/null
+++ b/core/lib/Drupal/Core/Routing/PartialMatcher.phpundefined

+++ b/core/lib/Drupal/Core/Routing/PartialMatcher.phpundefined
+++ b/core/lib/Drupal/Core/Routing/PartialMatcher.phpundefined
@@ -0,0 +1,35 @@

@@ -0,0 +1,35 @@
+<?php

Ibid.

+++ b/core/lib/Drupal/Core/Routing/PartialMatcher.phpundefined
--- /dev/null
+++ b/core/lib/Drupal/Core/Routing/PartialMatcherInterface.phpundefined

+++ b/core/lib/Drupal/Core/Routing/PartialMatcherInterface.phpundefined
+++ b/core/lib/Drupal/Core/Routing/PartialMatcherInterface.phpundefined
@@ -0,0 +1,34 @@

@@ -0,0 +1,34 @@
+<?php
+
+namespace Drupal\Core\Routing;

Ibid.

+++ b/core/lib/Drupal/Core/Routing/PathMatcher.phpundefined
@@ -0,0 +1,134 @@
+   * @param string $table
+   *   The table in the database to use for matching.

Needs to start with (optional) and explain what default is.

+++ b/core/lib/Drupal/Core/Routing/PathMatcher.phpundefined
--- /dev/null
+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.phpundefined

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.phpundefined
+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.phpundefined
@@ -0,0 +1,44 @@

@@ -0,0 +1,44 @@
+<?php
+
+namespace Drupal\Core\Routing;

Needs @file docblock.

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.phpundefined
@@ -0,0 +1,44 @@
+class RouteBuilder {
+
+  protected $dumper;

Needs a docblock for this member.

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.phpundefined
@@ -0,0 +1,44 @@
+   * Construcs the RouteBuilder using the passed MatcherDumperInterface

Missing final period.

+++ b/core/lib/Drupal/Core/Routing/RouteCompiler.phpundefined
@@ -0,0 +1,247 @@
+   * @return CompiledRoute
+   *   A CompiledRoute instance.

Needs path prefix.

+++ b/core/lib/Drupal/Core/Routing/RouteCompiler.phpundefined
@@ -0,0 +1,247 @@
+   * @return type

Is this a correct type hint? Seems wrong.

+++ b/core/lib/Drupal/Core/Routing/RouteCompiler.phpundefined
@@ -0,0 +1,247 @@
+   * The pattern outline is the path pattern but normalized so that all
+   * placeholders are equal strings and default values are removed.

Should be rewrapped for 80 characters.

+++ b/core/lib/Drupal/Core/Routing/RouteCompiler.phpundefined
--- /dev/null
+++ b/core/modules/system/lib/Drupal/system/FileDownload.phpundefined

+++ b/core/modules/system/lib/Drupal/system/FileDownload.phpundefined
+++ b/core/modules/system/lib/Drupal/system/FileDownload.phpundefined
@@ -0,0 +1,54 @@

@@ -0,0 +1,54 @@
+<?php
+
+namespace Drupal\system;

Missing @file docblock.

effulgentsia’s picture

Moshe and I chatted with Dries and webchick about this patch, and made the same case that Crell has also made, which is that it will help keep core moving to get this patch committed as soon as possible even if some of #115 and other feedback isn't complete and requires a follow up. Dries agreed to that, so will be looking to commit this this weekend, barring a major problem being uncovered.

@Crell: to allow this to be done as a merge that preserves sandbox history, please sync up the sandbox with the interdiffs in #108 and #112 and the branch from #91 .

The biggest concern that Dries raised is that this issue introduces a new hook (hook_route_info()) that isn't documented in system.api.php. That's generally a no-no, so it would be great to get that added today by someone who understands Route objects well enough to write that documentation.

I'm also considering moving from a hook to something more Symfony-esque, possibly even in the bundle classes or with an event. I'm open to suggestions there, but working with it the hook feels very out of place in this system.

That sounds like a fine follow up, but so long as we have a hook in the meantime, we need to document it. That won't be wasted effort, since whatever documentation we add explaining Route objects (or linking to Symfony docs if there are any) will be applicable to wherever else their registration gets moved to.

Dries had some other minor feedback, but dreditor lost it. If I can adequately remember it, I'll post it here later today, but it's nothing that can't happen in a follow up.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

And given #116, I'm going to mark this RTBC so people have a chance now to uncover major problems before commit.

Lars Toomre’s picture

Status: Reviewed & tested by the community » Needs review

Obviously core committers can commit whatever, but I must say that I am disappointed that such big patches are not held up to the standards that derail many other smaller patches.

There are significant documentation violations in the current patch. Why do not the documentation gate standards apply in this case????

Lars Toomre’s picture

Status: Needs review » Reviewed & tested by the community

Sorry did not mean to change the status.

moshe weitzman’s picture

Status: Reviewed & tested by the community » Needs work

We need to decide whether to do a drupal_render() or a drupal_render_page() when in a subrequest. Or maybe the subrequest should ahve a flag that says how this should be handled. The test failures in mentioned in #88 happen because our 403 handling happens in a subrequest and thus no blocks are sent we go through drupal_render().

@Lars - one reason might be that this patch blocks other important work - namely web services. The committers might choose to unblock that work and work on coding standards in a follow-up. Sounds like a wise decision to me.

Lars Toomre’s picture

Thanks @moshe... Can you then open up a follow-up issue to address whatever might have been overlooked in this patch that might violate 'normal' standards? I would think that is a requirement before this is committed. Please post that follow-up issue here. Thanks.

Crell’s picture

Status: Needs work » Needs review

Holy flipping lord you people have been busy today! :-)

I will review the interdiffs that effulgentsia mentioned and try to sync everything up. I hope I don't have to redo anything. :-( (Mixing patch and git workflows suck.)

Yes, some of the documentation et al wasn't done yet; I didn't see this patch as RTBC and finishing that up was a "wait for the code to work first" task.

Moshe is correct that there's still some details that need to be sorted out here to avoid fragility before it gets committed. I have a sneaky suspicion that at the moment the new router pages in the test module are breaking, but the test isn't catching it properly. (I was getting a page-within-a-page effect before.)

That said, I'm totally down with getting this in this weekend and continuing work in additional follow-ups. That should make it much easier to do with smaller patches.

Setting to needs review to keep people looking at it while I resync everything. Thanks, everyone!

larowlan’s picture

Anonymous’s picture

I committed everything up to #108 to routing-lin for Crell's review.

Lars Toomre’s picture

Thanks @linclark. At what point do Drupal documentation/coding standards apply?

Crell’s picture

Lars: They always apply, it's just not always productive to apply them *yet*. :-) Those who've been around Drupal long enough know that I'm rather OCD about the level of API documentation I tend to include, but since the code was still shifting heavily until very recently I had not gone through to ensure that every function was perfectly documented. That is a final step. The same applies to hook_route_info(), since I've been strongly considering replacing that with something more OOP-ish. It feels, well, off to have this one legacy definition hook in a system that is otherwise 100% Symfony-based OO. That's why it's not fully documented yet, either.

In some larger patches (for some undefined definition of larger), it's simply more practical to get a baseline issue in knowing there will be follow-ups, both for coding style and documentation and for functionality. For instance, this patch doesn't, yet, include access control on routes in the new router. I have a pretty good idea of how to do that, but we can get this in now and then layer that on in a later patch. That minimizes time spent "chasing HEAD" and allows other people (eg, SCOTCH) to continue working.

So yes, pedantry about coding and documentation standards is good and welcome, it just needs to be applied correctly. Also, for many larger issues lately we're using git branches, not patches, so I cannot actually make use of patches posted here directly. I will see if I can make useful git commits out of #112 above, but if not, that can always be a smaller follow-up.

cosmicdreams’s picture

@Lars: Also, if I have time this weekend I'll try to handle the documentation notes you've mentioned.

That probably won't happen tonight, but maybe tomorrow.

Lars Toomre’s picture

Thanks @Crell for your explanation. I guess those of those of us who do not work in git branches should just keep quiet and hope that what results is OK. I am really discouraged about reviewing resulting green patches in the future.

Please note my comment at start of #110. I was just trying to help once this issue finally turned green.

Crell’s picture

Lars: Don't keep quiet! As I said, nitpicky reviews are a *good* thing, once an issue reaches the "ready for nitpicking" stage. Reviewing in the issue is where it should be; it's just posting of new patches that is problematic when an issue is being developed via branch.

I don't want to discourage you. Rather, I would hope that you'd focus pedantry-reviews (which I fully support) on issues that are "ready" for them. Just because a posted patch is green doesn't mean it's ready for that level of tweaking. The patch up in, say, #12 would have been a horribly bad place for a nitpick review because the code hadn't even settled down enough yet to bother documenting.

I was actually rather shocked to see webchick set this issue to RTBC, since I didn't think it was yet. IMO that was premature. That said, we have no way right now to flag an issue as "ready for nitpicking", so it's not always clear when an issue is "ready" for it.

tim.plunkett’s picture

I think the key is to actually post a reroll once in a while, and not continually nitpick.

Lars Toomre’s picture

Thanks @Crell for your further explanation. I saw that this was a critical issue and was trying to help that this might reach commit stage (knowing something about Drupal standards).

When I saw documentation issues on a green patch, I spoke up. I had no idea that it needed further refinement nor would I today if I was encountering this issue for the first time.

sun’s picture

  1. This patch adds a 'router' table and the infrastructure for the new router, as well as a hook_router_info(), but there is no real example implementation or the like. Therefore, it is incredibly hard to review and verify the architectural design and implementation.
  2. All router paths are rooted (prefixed with a / slash). I do not see where the case of Drupal being installed in a subdirectory is handled.
  3. Drupal\Core\EventSubscriber\RouterListener does not seem to be used anymore with this patch, but it is not removed either?
    (There are also many stale references to it in other code/classes.)
  4. Am I interpreting the patch wrongly, or is PathMatcher only a legacy construct to handle old menu_router definitions stored in the database? If so, shouldn't it be named LegacyPathMatcher, pending later removal?
  5. The relationship, behavior, and difference of NestedMatcher::setInitialMatcher(), NestedMatcher::setFinalMatcher(), and NestedMatcher::addPartialMatcher() is entirely unclear to me. The phpDoc is identical, they appear to do exactly the same, with the only difference that there can only be one initial and one final matcher. This looks like a giant overhead in terms of code and API to me. Why aren't the initial and final matchers simply added like any other matcher and simply use a very high/low weight?
  6. There are two magic request attributes, '_controller' and '_content', which don't seem to be documented anywhere.
    Also, HtmlPageController contains this line, which is really weird:
        $controller = $_content;
    
  7. RouteBuilder contains a comment that says "it cannot be tested, because it uses the module system", but I don't see why - that's what WebTestBase is for?
  8. At least one new class is added directly to the Drupal\Core\ namespace without a component. I don't understand why we started this and why we're continuing to do it.

This is a very incomplete review, but as I mentioned initially, I think it's all that can be reasonably reviewed on the current state without a real implementation/integration.

I'm very surprised to see that there is no major slowdown in testbot performance on this patch, despite rebuilding and storing the menu router twice. (unless I got that part wrong)

I also must say that this code appears to be even more complex than our current router, whereas I always thought it would barely be possible to beat that complexity. The amount of abstractions and übersmall bits of logic happening in separate classes really worries me. We already had problems to properly maintain the old router system due to its complexity. This proposal goes far beyond that complexity, and while the high-level abstractions/separations into building/compiling/matching make sense, the further internal abstractions are worrisome (and leave a uncomfortable touch of over-engineering).

Lastly, there's one must-fix mistake in this branch; it adds an empty /core/vendor/Routing directory. (dunno how you managed to add an empty directory, but yeah) (Also, the latest patch in this issue adds a /core/includes/common.inc.orig copy.)

Speaking of, I'd actually prefer to see a patch committed instead of the branch being merged, since we still don't have any clear rule for what is getting merged into core and what isn't. In essence, I don't see why this branch should be merged, whereas almost all other sandbox branches are not merged.

andypost’s picture

it is incredibly hard to review and verify the architectural design and implementation.

Is this system allows re-ordering matchers for example make a some static list of path aliases to work first?

Crell’s picture

Re #132:

1) The included unit tests are a sample implementation. See router_test.module. No currently existing paths are converted yet because they all need access control, and that hasn't been implemented yet. It sounds from the last few comments like we're going to save that to a second patch. The FileDownload class was my original attempt to convert a "real" path, but I realized that ran into other issues and a pure webtest was the correct way to test the system anyway. (I'll remove that class in a bit.)

2) The / prefix on routes is a Symfony thing. That's just how its routing tools work. Drupal still works fine in a subdirectory, and in fact my dev copy on my laptop is in a subdirectory just to make sure I don't inadvertently break that. (Testbot runs in a subdirectory, too.)

3) Possibly. I'll have to verify. I believe that was a stopgap until Symfony's routing system improved, but we improved it so it can probably go.

4) You're interpreting it wrong. :-) PathMatcher is a port of the old path-matching logic from menu.inc into a form that the new architecture supports. The algorithm is about 95% the same, because it's a good algorithm. (I had to tweak the way optional values are handled slightly because Symfony routes require them to be named while in Drupal 7 they're implicit, but not by much.) You'll note it is still running against the new router table, not the old menu_router table.

5) The three types of submatcher are documented in comment #4. A Matcher as Symfony sees it must return metadata off of a single route. That's the FinalMatcher here. The Initial and Partial matchers return a RouteCollection, not metadata. InitiaMatcher gets its dataset from the database; PartialMatcher from a passed RouteCollection. That's why they cannot be the same. dbu and I have been trying to come up with something simpler, but so far it does not exist if we want a pluggable, multi-stage matcher. We want to try and simplify it if we can when this architecture gets pushed upstream to the Symfony CMF Routing component as planned.

6) _controller is a Symfony thing. It's what the Symfony ControllerResolver looks for. _content is our "subcontroller" as discussed at length here. This is another one of those things that I wasn't looking at intense documentation for until we were done with the actual code (as above).

7) It cannot be unit tested. Honestly I don't consider tests that only run when the entire system is setup and we have to verify a full global state to be of much value. Once we have better control over the modules that a test instance sees (cf #1331486: Move module_invoke_*() and friends to an Extensions class), we can put an actually useful test around this class.

8) We have a couple of classes there for, really, lack of anywhere else to put them. We can move them later once we get a sense for what they all are. That should not hold up this patch.

9) We are not building and storing the menu router data twice. The current menu router works exactly as it does now and has not been touched. We are adding a completely new, independent routing system that ignores the menu system entirely and doesn't even know it's there; they are both active courtesy of ChainMatcher, which as documented is a ripoff of the ChainRouter in Symfony CMF. Ideally, once we have a Generator in place and Symfony's Generator support expands to include Request-based rather than RequestContext-based logic, we can drop ChainMatcher and pull in the Symfony CMF Routing component, where much of this logic will migrate.

10) The extra vendor/Routing directory: That's my local checkout of the Symfony CMF Routing component. I have no idea why it's in the patch. :-) I'll sort that out. Ibid for common.inc.orig. Probably a result of a patch applied somewhere along the line.

11) This router is more lines of text file than the current one yes. However, I don't think it's more complex, except for where it necessarily is because it does more. The chief thing that makes the current menu router complex is that it does a lot of different things all bundled up in the same place. (Routing, access control, fit computation, menu links, menu link denormalization, code autoloding, etc.) All of that is separate in this model. It is more lines of text because the code is more cleanly separated. (LOC is not a good measure of complexity.)

12) I absolutely 100% oppose applying the patch instead of merging. Merge process is fundamentally superior, as has been discussed to death elsewhere. I've been doing a rebase workflow for this patch, even though Drupal.org makes that unnecessarily difficult, specifically to make it easier to merge in the future. If we're not merging most big sandbox branches, that's because we're doing it wrong there. We should be merging more often, not less.

Re #133:

What you're describing is ChainMatcher, which lets you wire up multiple matchers to run in serial. Whichever one actually finds and returns information first wins. At the moment we're not using it for site-defined highly-optimized paths, but that's what its conceptual parent, ChainRouter, is for. (Eg, dumping your very common routes to apache mod_rewrite rules so that your main matcher never even hits the database.) That's not something I'm worried about right now but is a conceptual possibility in the future.

Crell’s picture

FileSize
112.9 KB

New patch.

This patch incorporates as commits the fixes from Lin and Effulgentsia above. I also merged in the interdiff from #112 (with commit credit), and manually included most of the fixes noted in #115. It also addresses the addressable issues raised in #132.

The drupa_render() vs. drupal_render_page() question is annoying. :-) Previously the tests were not catching the fact that with drupal_render_page(), the new-style routes were all getting page-within-a-page inception treatment. With chx's help I modified the tests to catch that, and then fixed it by flagging legacy routes with a _legacy attribute. Then in ViewSubscriber we check for that and fork the the handling so we call the right render function. It's fugly but works, and that class is expected to go away eventually so I'm happy with it for now.

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.

Assuming this is green, I think we're ready to commit and continue working on follow ups in later issues. I'll add a few of them now while waiting on testbot.

Dries/catch: Please do NOT commit this issue. Merge from the routing branch in the WSCCI sandbox.

Lars Toomre’s picture

Here are some initial thoughts for @crell after reviewing the most recent patch.

I am confused about whether type hints should start for example with 'Drupal' or '/Drupal'. From review of other patches, it would seem 'Drupal' is the preferred variety.

+++ b/core/lib/Drupal/Core/HtmlPageController.phpundefined
@@ -0,0 +1,69 @@
+   * Controller method for generic HTML pages.
+   *
+   * @param Request $request
+   *   The request object.
+   * @param callable $_content
+   *   The body content callable that contains the body region of this page.
+   *
+   * @return \Symfony\Component\HttpFoundation\Response
+   *   A response object.

I think Request needs to be qualified with a path.

Also, I have no idea what a callable type hint means.

+++ b/core/lib/Drupal/Core/LegacyUrlMatcher.phpundefined
@@ -98,8 +99,8 @@ class LegacyUrlMatcher implements UrlMatcherInterface {
    *
    * @api

Carry-over from elsewhere?

+++ b/core/lib/Drupal/Core/Routing/ChainMatcher.phpundefined
@@ -0,0 +1,165 @@
+   *
+   * @param Request $request The request to match
+   *
+   * @return array An array of parameters

Needs to be transformed into Drupal docblock standards.

+++ b/core/lib/Drupal/Core/Routing/ChainMatcher.phpundefined
@@ -0,0 +1,165 @@
+    * @param MatcherInterface $matcher
+    *   The matcher to add.
+    * @param int $priority

Needs a path prefix.

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.phpundefined
@@ -0,0 +1,172 @@
+   * @return Route
+   *   A Route instance.

Needs to be prefixed with path.

+++ b/core/lib/Drupal/Core/Routing/FirstEntryFinalMatcher.phpundefined
@@ -0,0 +1,82 @@
+   *
+   * @var RouteCollection

My understanding is that this needs a path prefix.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcher.phpundefined
@@ -0,0 +1,153 @@
+   * @return NestedMatcherInterface
+   *   The current matcher.

Needs a path prefix.

pounard’s picture

http://php.net/manual/en/language.types.callable.php
The callable type is a valid type hint since PHP 5.4.

dbu’s picture

@pounard: is drupal 8 going to be php 5.4 only?

pounard’s picture

Nope, but "callable" for type hinting callbacks is used in PHP doc by many projects, and in PHP official documentation itself since a long time (maybe PHP 4). I'd rather use the right term here, callable is a valid documentation type hint.

catch’s picture

Merge from the routing branch in the WSCCI sandbox.

I'm not going to merge from that branch unless it has a clean history with meaningful commit messages. Does it?

General note: I've been seriously failing to find time to sit down properly with this patch so far :(

Two questions for now:

- despite the lack of access control, could we convert one real router item to the new system - it's OK if that's an access bypass for !user_access('access content'), we just need to track it in a critical to fix it (which we'll have to do anyway since we can't ship the new router without access control anyway).

- it'd be good to have a clear-ish plan for access control documented somewhere before this goes in. That's a major part of the current routing system and it does make it harder to review this one without seeing how it pieces together.

Crell’s picture

I've been doing a rebase workflow for the routing branch specifically to keep it as tidy as possible, even though I think that's excessive. It's cleaner than the kernel branch, which was also a merge.

For access control: #1793520: Add access control mechanism for new router system

I'm skeptical about "convert something and break it". What's the value of that? We have integration tests in the patch that show how it works, and that it does work. We don't need user-facing code to operate as a fake validation test when we have an architecture that supports for-reals validation tests.

If you really want to do that I'd need an indication of what you're willing to break. An autocomplete callback seems like the logical choice to me, since that has no SCOTCH dependency.

catch’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/EventSubscriber/ViewSubscriber.phpundefined
@@ -46,13 +47,43 @@ class ViewSubscriber implements EventSubscriberInterface {
+    elseif ($request->attributes->get('_legacy')) {
+      // This is an old hook_menu-based subrequest, which means we assume
+      // the body is supposed to be the complete page.
+      $page_result = $event->getControllerResult();
+      if (!is_array($page_result)) {
+        $page_result = array(
+          '#markup' => $page_result,
+        );
+      }
+      $event->setResponse(new Response(drupal_render_page($page_result)));
     }
     else {
-      $event->setResponse(new Response('Unsupported Media Type', 415));
+      // This is a new-style Symfony-esque subrequest, which means we assume
+      // the body is not supposed to be a complete page but just a page
+      // fragment.
+      $page_result = $event->getControllerResult();
+      if (!is_array($page_result)) {
+        $page_result = array(
+          '#markup' => $page_result,
+        );
+      }
+      $event->setResponse(new Response(drupal_render($page_result)));

We're adding support for subrequests here, but is that actually being tested? For one I'm not sure what'll happen with #attached and drupal_add_*() calls. I have a feeling they'll still work because they're still ultimately affecting global statics (i.e. within the drupal_render() call) but it feels a bit tenuous. That's a whole issue that needs to be tackled by itself and I had a long conversation with msonnabaum about it at DC in relation to the HttpCache issue but eventually we're going to have to collect more information here from the subrequests than just a string, since they can legitimately affect all kinds of things in the html head at the moment (CSS, JavaScript, meta tags etc.)

+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.phpundefined
@@ -0,0 +1,174 @@
+        // This is only temporary. We need to strip off the compiled route from
+        // route object in order to serialize it. Cloning strips off the
+        // compiled route object. Remove this once
+        // https://github.com/symfony/symfony/pull/4755 is merged and brought
+        // back downstream.

Why are we compiling the route and serializing it in the db at all? Presumably for performance compared to compiling it on runtime? (but see question below)

+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.phpundefined
@@ -0,0 +1,174 @@
+    // Delete any old records in this route set first, then insert the new ones.
+    // That avoids stale data. The transaction makes it atomic to avoid
+    // unstable router states due to random failures.
+    $txn = $this->connection->startTransaction();
+
+    $this->connection->delete($this->tableName)
+      ->condition('route_set', $options['route_set'])
+      ->execute();

I'm still concerned about the potential race here for SELECT queries between the delete and insert.

We have all the rigmarole of variable_set('menu_rebuild_needed') and the entire lock system added to Drupal 6 to prevent that race condition and as far as I can see that's being reverted here - both for non-transactional dbs and depending on transaction isolation settings. I'm not prepared to regress that very long standing critical bug by committing this so would be good to tackle that, can probably be mostly copied and pasted.

+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.phpundefined
@@ -0,0 +1,174 @@
+
+    $this->connection->delete($this->tableName)
+      ->condition('route_set', $options['route_set'])

If someone wanted to swap out the db router with an alternate backend, is there a clean entry point for this? That's a feature request of course but I'd be interested in the answer.

+++ b/core/lib/Drupal/Core/Routing/NestedMatcherInterface.phpundefined
@@ -0,0 +1,55 @@
+  /**
+   * Adds a partial matcher to the matching plan.
+   *
+   * Partial matchers will be run in the order in which they are added.
+   *
+   * @param \Drupal\Core\Routing\PartialMatcherInterface $matcher
+   *   A partial matcher.
+   *
+   * @return \Drupal\Core\Routing\NestedMatcherInterface
+   *   The current matcher.
+   */

Why does ChainMatcher call matchers in sorted order, but NestedMatcher run them in the order they're added? If I wanted to inject a highly optimized matcher at the top of the chain, am I stuck then?

+++ b/core/lib/Drupal/Core/Routing/PathMatcher.phpundefined
@@ -0,0 +1,134 @@
+    $routes = $this->connection->query("SELECT name, route FROM {{$this->tableName}} WHERE pattern_outline IN (:patterns) ORDER BY fit", array(
+      ':patterns' => $ancestors,
+    ))

Missing db_escape_table().

+++ b/core/lib/Drupal/Core/Routing/PathMatcher.phpundefined
@@ -0,0 +1,134 @@
+    $collection = new RouteCollection();
+    foreach ($routes as $name => $route) {
+      $route = unserialize($route);
+      if (preg_match($route->compile()->getRegex(), $path, $matches)) {
+        $collection->add($name, $route);
+      }

I'm confused here. We're compiling and serializing the route in the database. But then we're compiling it again here before calling getRegex(). Is that compile a no-op or does it really need to be compiled during dumping then compiled when it's loaded? If it doesn't, then this needs a @todo to match the one where it's serialized.

+++ b/core/lib/Drupal/Core/Routing/RouteCompiler.phpundefined
@@ -0,0 +1,248 @@
+  /**
+   * Generates a regular expression that will match this pattern.
+   *
+   * This regex can be used in preg_match() to extract values inside {}.
+   *
+   * This algorithm was lifted directly from Symfony's RouteCompiler class.
+   * It is not factored out nicely there, so we cannot simply subclass it.
+   * @todo Refactor Symfony's RouteCompiler so that it's useful to subclass.
+   *
+   * @param \Symfony\Component\Routing\Route $route
+   *   The route object.
+   * @param string $pattern
+   *   The pattern for which we want a matching regex.
+   *
+   * @return string
+   *   A regular expression that will match a path against this route.
+   *
+   * @throws \LogicException
+   */

So this looks like the successor to menu_get_ancestors() or similar, however it also looks like it could be a lot less performant than that. Has anyone done any profiling of this?

In terms of an example router item, there's currently no "this is what it used to look like, this is what it looks like now" in the patch, it's just to see those lines used and removed. I'd also like to be able to do before/after profiling on a real route with XHProf.

Crell’s picture

Discussed some with catch in IRC:

- Yes, subrequest handling is going to need to get much more robust. At this point, I consider that part of SCOTCH's scope, which will be replacing ViewSubscriber and HtmlPageController with something much more robust. The code in both of those is at this point a stopgap to get things lined up for the SCOTCH team.

- Route compilation: We're not serializing the compiled route, because that breaks horribly. :-) The linked Symfony issue is in so I can remove that comment/workaround now. The compiled route is just a collection of derived information. I modeled that process directly on Symfony, and stole some of its code verbatim.

- We discussed the race condition and need for locking. I'm going to fold #1786460: Add the lock API to the DIC into this issue so that we can make the lock system injected into the dumper.

- Specified order of nested matcher: Eh, no particular reason. I can add that.

- menu_get_ancestors() turned into getCandidateOutlines() almost verbatim. The regex is a Symfony thing. If we were matching in memory instead of the database then we could use that, but I don't know how to do that sort of regex directly in the DB portably. I'm using the regex for the additional requirements on routes, such as "this value has to be an int". There's likely a future optimization we could make to fold those two things together, or push the regex into the database. That may even be Database-specific, which, per the next note, is fine.

- Because everything is wired through the DIC, it should be completely possible to replace the dumper with something Mongo-backed, and swap in a corresponding initial matcher as well. The storage and algorithm those use can be whatever chx wants them to be. :-) In fact, alternate SQL-based backends should be equally possible, so we can even experiment in contrib with alternate matching algorithms. (That's why I've been harping on separate injectable objects so much.)

- Additional router item: Without ACL, the only option we have here that I'd be comfortable with is autocompletes. I can try to do that but I blame catch if it causes issues with our critical count. :-P

Dries’s picture

I've reviewed this patch a couple of times and read the recent reviews; both sun and catch have some good points. All things considered, I think there is a lot of follow-up work that needs to be done, however, I'd be comfortable committing this patch as is. We'd benefit from getting this in, so we can unblock the web services and layouts work, and we can make faster progress cleaning things up. We just can't afford to have this patch stall another 2-3 weeks.

So how about catch or I commit this patch on Monday, October 1st? That will give us exactly two months for web services and layouts to get in.

Dries’s picture

Status: Needs work » Reviewed & tested by the community

Bringing my last comment to people's attention.

chx’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update, +API change, +WSCCI, +symfony, +wscci-hitlist

The last submitted patch, 1606794-routing.patch, failed testing.

chx’s picture

Sure, I am all for progress. I will open a bunch of bug reports after. Ceterum autem censeo merge delendam esse.

chx’s picture

Status: Needs work » Reviewed & tested by the community
cweagans’s picture

Status: Reviewed & tested by the community » Needs work

Patch no longer applies.

plach’s picture

Ceterum autem censeo merge delendam esse.

LOL

(I just learnt that "merge" is feminine ;)

cweagans’s picture

Status: Needs work » Reviewed & tested by the community

But I guess that doesn't matter. I was just informed that this is going to be merged. Ugh.

Crell’s picture

FileSize
114.01 KB

Revised patch to ensure no collisions. This should still be merged, in whatever language. :-)

I also fixed up a few small cleanups and added the nested matcher ordering catch asked about, because they were easy. Per #144 I didn't mkae any other changes.

Dratch: To merge, add this repository:

http://drupal.org/project/1260830/git-instructions

The "routing" branch is what you want to merge. It should have a nice clean history, and I just rebased it again.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1606794-routing.patch, failed testing.

Crell’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
114.03 KB

Stupid update functions...

Crell’s picture

Issue summary: View changes

Add last updated date.

catch’s picture

I'm OK with this going in and adding follow-ups to unblock things, but at the least the race condition in the router dumper needs to be opened as a critical bug aftwards, since that's a straight regression of #251792: Implement a locking framework for long operations (the router race condition was the main reason the locking framework was added in the first place).

Crell’s picture

I've added various followups to the summary and marked the "use locking" one in particular as a critical.

Crell’s picture

Issue summary: View changes

Add links to follow-up tasks.

sun’s picture

Component: base system » routing system

I'm also OK with moving forward, and I don't think there's a need to wait.

Though I do hope we will try hard to reduce complexity and simplify wherever possible in follow-up issues. Which should perfectly include considerations to make things in the new routing system less swappable and less customizable. Bear in mind that our current router is not swappable, not pluggable, and not customizable at all. This code runs in the critical path, so it's not only about human/code complexity but also about performance and architectural overhead.

chx’s picture

Some minimal pluggability IS present currently. Not a lot, not nice, but it is there. require_once DRUPAL_ROOT . '/' . variable_get('menu_inc', 'includes/menu.inc');

Berdir’s picture

When committing this, use --whitespace=fix to get rid of the trailing whitespaces.

Dries’s picture

Berdir, Crell: it would be helpful if you could provide the right merge commands. If not, I'll figure it out on Monday.

webchick’s picture

Assigned: Crell » Dries

Assigning to Dries now.

Git commands to do the merge would be helpful, so we don't need to waste time figuring those out (and probably getting them wrong).

Crell’s picture

I haven't tested this to make sure it's exactly right, but it should be something like:

git clone --recursive --branch 8.x Dries@git.drupal.org:project/drupal.git
cd drupal
git remote add wscci http://git.drupal.org/sandbox/Crell/1260830.git
git fetch --all
git checkout 8.x
git merge --no-ff wscci/routing
git push

tim.plunkett’s picture

I would revise that to

git clone --recursive --branch 8.x Dries@git.drupal.org:project/drupal.git
cd drupal
git remote add wscci http://git.drupal.org/sandbox/Crell/1260830.git
git fetch --all
git checkout 8.x
git merge --no-ff wscci/routing -m 'Issue #1606794 by Crell, linclark, effulgentsia, katbailey, disasm, larowlan: Implement new routing system.'
git push

Though, likely, you won't need to make a new clone, just start from adding the remote after "git pull" in your existing repo.

Crell’s picture

If you want to mention names in the merge message, I'd throw dbu in there as well.

Dries’s picture

I tried to do the merge but got a conflict:

Auto-merging core/modules/system/lib/Drupal/system/Tests/Menu/RouterTest.php
CONFLICT (content): Merge conflict in core/modules/system/lib/Drupal/system/Tests/Menu/RouterTest.php
effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update, +API change, +WSCCI, +symfony, +wscci-hitlist

The last submitted patch, 1606794-routing.patch, failed testing.

Lars Toomre’s picture

Guessing from the CONFLICT message, the merge is probably failing due to #1797452: Remove t() from asserts for M includes system tests which was committed earlier today. If so, check for the t() somewhere in the tests of the branch being merged. Those t() wraps around assert messages should be removed.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
114.02 KB

I rebased the routing branch (I hope I didn't mess anything up). Here's the patch for bot. If it comes back green, and I get confirmation from d.o. that the rebase succeeded, I'll set this to RTBC. Please don't merge until then.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

I verified the rebase both by recloning into an empty local workspace, and via today's timestamps on http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/routing. Unfortunately, at the time I write this, http://drupal.org/node/1260830/commits still shows pre-rebased commits, but that shouldn't affect the merge, and will presumably get updated when d.o. lets go of whatever cache is driving that page.

webchick’s picture

Issue tags: +Avoid commit conflicts

Tagging, too, so in case Dries doesn't get to this now for a couple of days it doesn't happen again.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Awesome! Did a git merge and pushed it to 8.x. This should unblock the web services and layouts work, to name just two. Great work.

tim.plunkett’s picture

Title: Implement new routing system » Change notification for: Implement new routing system
Category: feature » task
Status: Fixed » Active
Issue tags: +Needs change record

A change notice should be added to help those understand how to use the new system.
I certainly know that VDC is interested in this.

cosmicdreams’s picture

Every pro Drupal / non-Drupal dev will be interested in this as well. So awesome!

Crell’s picture

Title: Change notification for: Implement new routing system » Implement new routing system
Category: task » feature
Status: Active » Fixed

Change notice added: http://drupal.org/node/1800686

Yay!

geerlingguy’s picture

For the change notice, it's great to have all the examples to highlight what's new, but first, 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...

Crell’s picture

Given that half of what's in hook_menu now is not yet in hook_router_info, I'm not entirely sure how to do that effectively. Feel free to edit if you do know. :-)

dbu’s picture

great to see this moving forward! thanks a lot to everybody who made this possible!

dbu’s picture

Issue summary: View changes

Add another follow-up task.

disasm’s picture

Issue summary: View changes

Updated issue summary.

effulgentsia’s picture

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Added a link to another follow up issue

effulgentsia’s picture

FYI: I added yet another follow up to the issue summary: #1859684: Expand routeBuilder unit test.

chx’s picture

Issue tags: -Avoid commit conflicts

Removing Avoid commit conflicts tag

chx’s picture

try #2.

YesCT’s picture

cleaning up tags a bit.

YesCT’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

Issue summary: View changes

add 2004334 and 2023795

pwolanin’s picture

Issue summary: View changes

add 1908756