Updated: Comment #29
Problem/Motivation
Defining custom routes as a tagged service is weird, it would be nice to have them in the routing.yml
Proposed resolution
Taking cues from TitleResolver, this allows a route to be defined like this:
route_callbacks:
- '\Drupal\mymodule\Routing\SomeClass::someMethod'
And it can be removed from the *.services.yml file.
The method will return an array of Route objects keyed by route name:
Before
Takes a RouteCollection and modifies it, returning nothing.
Only $collection->add() should be used here, but all methods are available.
protected function routes(RouteCollection $collection) {
$directory_path = file_stream_wrapper_get_instance_by_scheme('public')->getDirectoryPath();
$route = new Route(
'/' . $directory_path . '/styles/{image_style}/{scheme}',
array(
'_controller' => 'Drupal\image\Controller\ImageStyleDownloadController::deliver',
),
array(
'_access' => 'TRUE',
)
);
$collection->add('image.style_public', $route);
}
After
No parameters, returns a traversable keyed by route name, containing Route objects.
This can either be an associative array keyed by route name of Route objects:
use Symfony\Component\Routing\Route;
public function routes() {
$routes = array();
$routes['mymodule.route_name'] = new Route(
'/mymodule/route',
array(
'_form' => '\Drupal\mymodule\Form\MyForm',
),
array(
'_permission' => 'access content',
)
);
return $routes;
}
Or return a RouteCollection object:
use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;
public function routes() {
$route_collection = new RouteCollection();
$route = new Route(
'/mymodule/route',
array(
'_form' => '\Drupal\mymodule\Form\MyForm',
),
array(
'_permission' => 'access content',
)
);
$route_collection->add('mymodule.route_name', $route);
return $route_collection;
}
If a RouteCollection needs to be modified outside of adding routes, an EventSubscriber for RoutingEvents::ALTER should be used.
Remaining tasks
N/A
User interface changes
N/A
API changes
The RoutingEvents::DYNAMIC event is removed.
A subsection of the *.routing.yml can be declared with route_callbacks
as the key, and a callable in the controller notation. These callables can use ContainerInjectionInterface if needed.
Comment | File | Size | Author |
---|---|---|---|
#57 | interdiff.txt | 8.25 KB | tim.plunkett |
#57 | routes-2145041-57.patch | 50.24 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettThis has a couple conversions done. Remaining tasks in the issue summary.
Comment #2
msonnabaum CreditAttribution: msonnabaum commentedI like the general idea, but this should just expect an array to be returned from the callback, exactly how you'd make static routes in the yaml file, and then they can get turned into a RouteCollection by whatever collects them.
Comment #3
larowlannice
Comment #5
tim.plunkettI still didn't do anything about weights, but this adopts msonnabaum's idea, which I wholeheartedly agree with.
Comment #6
tim.plunkettComment #8
tim.plunkettRounded out the unit test.
Comment #9
dawehnerJust to express my thoughts:
Why the hell do we so active try to hide the symfony routing system from the users.
Comment #10
tim.plunkettThe most important part to me is that we have the pointer to the dynamic route provider in the routing.yml file.
You'll notice that the first patch still passes around a RouteCollection. That's still up for debate.
However, I personally think that returning an array is better for a couple reasons:
Comment #11
dawehnerHaving a route object helps you sooooo much to know how things are called etc.
Comment #12
tim.plunkettWe discussed more in IRC, and @dawehner proposed a better structure for the routing.yml, which makes RouteBuilder much clearer.
Also, the super complex route subscribers now just use RoutingEvents::ALTER, the DYNAMIC is removed.
Only rest and config_translation are left in between right now.
I'm going to look into allowing callbacks to still manipulate a route collection if they want...
Comment #13
dawehnerJust to be fare, this could have been done before as well. The only reason why it was not, was that other people can alter those routes easily as well.
Let's at least ensure that $routes is not empty.
Comment #14
damiankloip CreditAttribution: damiankloip commentedI wouldn't say the route subscribers were super complex before :-)
Doing this just seems like we're regressing slightly from what we currently have with collections. If we go much further, we may as well just bring back hook_menu.
It also seems wrong to move everything into alter.
Comment #15
tim.plunkettAs @dawehner pointed out, this is possible without the other changes here. And I'd argue that this is proof it belongs in alters.
As I said:
This is still a WIP.
Comment #18
tim.plunkettHere are two patches.
One just fixes the bugs.
The second passes the RouteCollection through to the callback.
If there is consensus that it is better, than that's fine by me.
Comment #21
tim.plunkettOkay!
So there was a huge miscommunication, and it turns out that @msonnabaum was *not* advocating for returning an array of associative arrays, but really just an array of Route objects. That way they don't have to interact with the RouteCollection.
I think this is a reasonable compromise.
Also I had to adjust the priority on EntityRouteAlterSubscriber and ParamConverterSubscriber, their previous ones made no sense (almost as though they were supposed to be weights?).
Comment #23
tim.plunkettI didn't realize that config_translation's dynamic routes used the route provider internally, so that has to stay as a subscriber. Added a comment to that effect.
Removed some leftover tests that are now covered by unit tests, and fixed views.
Comment #24
dawehnerI am little bit worried about the need of so many "magic" priorities.
Just as a futurure todo: It would be cool to have a generic object in another namespaces which does that, given that using the ControllerResolver is kind of semantically wrong here. We have a specialized resolver (TitleResolver) already.
Can we use 'callback' maybe instead? Controller feels kind of weird.
In case we would return a route collection this code would just be $collection->addCollection($controller_collection);
That line has always been wrong.
Can't we just use an interface and specify just the class? Just wondering, what is the usecase of a class with multiple route callbacks.
Should we also move the files into another directory so they are easier to find, especially because they are not event subscribers anymore.
What is the reason to remove that test here?
Comment #25
Crell CreditAttribution: Crell commentedSwitching from a DYNAMIC_ROUTE event to a callable: +1
Having that callable return Route objects rather than arrays: +1
Having that callable return an array of route objects rather than a RouteCollection: -1
Moving definitions from dynamic routes to route alter: + Er, why?
Due to the way PHP works, the code as written here would allow both a RouteCollection and an array to work, I think, since RouteCollection is iterable. That said, from an API definition perspective I far prefer returning a RouteCollection than "an array keyed by name, or something that could be iterated as such". That's less of an API-by-design and more an API-by-implementation, which is in most cases inferior.
Comment #26
tim.plunkettRerolled for #2102417: Change Drupal\Core\Routing\RouteBuildEvent::getModule() to getProvider(), and addressed the review:
24.1
Those were already magic numbers, I'm just changing them.
24.2
TitleResolver still uses ControllerResolver. Unless you mean specifically writing a RouteResolver to do this logic inside RouteBuilder? Sounds like a follow-up worth debating
24.3
Good idea
24.4
I think it's confusing to have two different return types.
24.5
Yep
24.6
I don't know that an interface is really needed, @msonnabaum was against it. It receives no parameters... And a class could have multiple sets of routes, and use different dependencies for each.
24.7
Done
24.8
This is covered by the unit test, and it was provided by a route subscriber that didn't need to exist
Comment #28
tim.plunkettWhoops, I rerolled an older patch, it was missing the last interdiff. No other changes.
Comment #29
tim.plunkettUpdating the issue summary.
Because we are removing the RoutingEvents::DYNAMIC event, it is an API break.
Comment #30
tim.plunkettComment #31
dawehnerThe @inheritdoc is a lie ;)
Comment #32
tim.plunkettSo it is :)
Comment #33
damiankloip CreditAttribution: damiankloip commentedI think returning an array of route objects is slightly nuts :/ What do we gain from that? Seems like we just lose?
This started out as an issue to keep things more in line with the YAML equivalent? Then we started returning arrays of route objects...huh? :)
I really think that is a bad thing (having no interaction with a collection). Being able to modify them is a fair point raised, only alters should be allowed to do that.
Comment #34
tim.plunkettComment #35
tim.plunkettOkay, the last 10 days have led me to remember why I worked on this in the first place. We need to provide a non-tagged-service way to do this, the DX is just awful. Bumping to major.
We have pretty strong consensus on a callback, that much is clear. But what parameters (if any) should the callback be passed? And what (if anything) should the callback return?
So here are the options we've discussed (which I duplicated in the issue summary):
a) No parameters, returns an associative array keyed by route name of arrays matching the structure of *.routing.yml files:
b) No parameters, returns an associative array keyed by route name of Route objects:
c) No parameters, returns a RouteCollection:
d) Is passed a RouteCollection, adds to it, returns nothing.
c) and d) both require knowledge of a RouteCollection, which is not actually relevant to dynamic routes. Remember the goal is to have as simple a corollary to *.routing.yml as possible.
b) requires knowing the order of parameters to a Route, which is fine when writing it with an IDE, but is harder to read later, even with an IDE.
a) seems the most Drupal-y, except it is just the PHP array version of the *.routing.yml syntax.
I can no longer remember who prefers which of these, but I remember most of people disagreeing. My first choice is a), with b) as a compromise. I do not think RouteCollection should be needed for these. Remember, we're not removing the ability for route subscribers to use the ALTER event to interact with the RouteCollections directly.
Comment #36
dawehnerMy first choice would be b) It does not required you to know about the route collection but it lets you use a route object, which really helps you to know what exactly can be passed in.
Comment #37
Crell CreditAttribution: Crell commentedC is my strong preference. Nearly everywhere else in code we use RouteCollections for "a set of routes"; Writing a new route filter requires collections; writing a route alter requires collections; using a RouteCollection here is consistent with that. Not doing so is inconsistent.
Failing that, B I can tolerate. At least it's still using Route objects.
D makes no sense at all, frankly. And A is right out unacceptable IMO.
Comment #38
tim.plunkettI should also point out that D is what core does, and what all subscribers (alters) will continue to do :)
Comment #39
msonnabaum CreditAttribution: msonnabaum commentedB is the best option.
If all you're doing is providing a list of routes (not altering an existing set), I see absolutely no value in forcing people to use a RouteCollection. This is not an issue of consistency because this isn't working with a collection of routes, it's just route discovery. Nothing in the RouteCollection interface is adding value here, it's just requiring knowledge of another class for no benefit.
Comment #40
tim.plunkettEveryone so far has also supported B as the best or second best choice.
That happens to be the direction taken in the last patch, which I've just rerolled (I have a branch for it in my sandbox).
Please, I invite more discussion!
Comment #41
amateescu CreditAttribution: amateescu commentedFrom the options in the issue summary, I like B as well. All of them still have the ugly '_stuff' but that's another battle :)
Edit: the patch looks good too but I prefer to let someone more familiar with the routing system RTBC it...
Comment #42
larowlan+1 b
Comment #43
damiankloip CreditAttribution: damiankloip commentedFrom the title, this issue is a good idea. However, I still don't understand this burning desire to return an array? Can someone explain that to me? :)
From what I can see, there are not really any benefit of doing this, as I mentioned above (in my previous comment) - we still just lose. Except, this is Drupal...
Sorry, not trying to be awkward. honest.
Comment #44
Crell CreditAttribution: Crell commenteddamian: Well, a module can define multiple dynamic routes. That means either an array of routes, or a collection object that holds multiple routes. Which of those we do seems to be the only point of contention remaining. :-)
Comment #45
damiankloip CreditAttribution: damiankloip commentedThanks, I get that, and understand the issue ;) I guess you mis interpreted my comment. I was basically saying I don't agree with returning an array.
See my previous comment to that; I'm advocating keeping the use of RouteCollection objects.
Comment #46
xjmFor what it's worth I agree with @msonnabaum's remarks in #39. And there's definitely an improvement regardless.
Also, nice diffstat on the patch.
Comment #47
Crell CreditAttribution: Crell commentedJust a note, strictly speaking since RouteCollection implements IteratorAggregate over the internal routes list, the foreach loop here will work with both an associative array *and* a RouteCollection. The code as written supports both equally well. So really, all we're debating is what to put in the documentation for which you're "supposed" to do. Or we just document it as "an iterable of route names and route objects, such as an associative array or RouteCollection."
Comment #48
tim.plunkettSo we have @dawehner, @msonnabaum, @amateescu, @larowlan, and @xjm with B as their first choice.
@Crell and I both explicitly said we'd pick B as our second choice.
Only @damiankloip disagrees at this point, but as Crell points out, this actually still supports RouteCollections.
We don't have a doc group for routing stuff, but when we do have a place to document *.routing.yml better, we can note that.
But core needs to pick one, and I think B is it.
Now that we've gotten the most consensus I think we can hope for, does anyone have a reviews for the code? Or an RTBC?
Comment #49
tim.plunkettDiscussed more in IRC.
This adds documentation explaining the expectations and usage of route_callbacks, and adds test coverage for a callback returning a RouteCollection.
So we document and test both B and C, satisfying all parties above, but core modules use B, satisfying the majority from above.
Comment #50
Crell CreditAttribution: Crell commentedAs discussed in IRC, I don't think the dynamic_routes pseudo-provider is even needed anymore. Dynamic routes from foo module will get passed through the ALTER event along with all other foo module routes.
We should remove the dynamic_routes alter call entirely, which means this check (and other references to dynamic_routes in this patch) should be able to just go away.
Aside from that (which includes references not quoted here), I'm fine with the rest.
Comment #51
tim.plunkettThat's a very good point. Views was relying on that, but I fixed it by actually reverting the code to be much closer to the original.
The real problem here is config_translation.
\Drupal\config_translation\Routing\RouteSubscriber calls ConfigMapperManager::getMappers().
ConfigMapperManager::getMappers() instantiates all of the mappers.
Each mapper gets the route provider *that is still being built* and tries to inspect it for other routes.
Each plugin then stores the route object it will base its routes on, even though it only ever calls getPath().
This only works in HEAD (and in the above patch) because this subscriber *happens* to run after all of the static routes.
Now that there is no final stage, it is supposed to run for each provider and inspect those routes only. But I can't come up with a good way (yet) to do that.
The entire architecture of these mappers is absolutely mind-boggling. They actively duplicate methods from the UrlGenerator, duplicate the request and call inbound path processing and request matching manually, and completely bypass the best practices of both the plugin and routing system.
Here is a patch fixing everything except for that. Work continues in my sandbox.
Comment #53
dawehnerPlease try to keep the scope of the issue in place. (Who will be able to understand all the effects of this patch)
Removing an alter event on any dynamic created is so wrong.
We rely on the alter event for more than just defining routes. ParamConverterSubscriber is just one of those examples.
You won't be able to alter any REST/Views/Panels routes.
Additional this is an unneeded API change.
Can't we just add the callback support, keep the dynamic events support and be happy?
I have been complaining about that approach since a long long time. What still should work from my perspective is using alter events + dynamic events exactly like views is doing it. Yes this is quite an effort though it solves the problem from ground up.
Comment #55
tim.plunkettPer #53, please review #49 and disregard #50/#51.
I will file a separate bug report for config_translation.
Comment #56
dawehner@tim
The reason why I posted was to not let you run into a horrible mistake.
Not sure whether you really want to let me review this patch.
Do we really have to add new routes on alter? This opens a new level of complexity, new bugs etc.
Same as before.
This makes it harder to find examples of callbacks :(
So why does views not use a callback in the routing.yml file to define the dynamic non-alter routes?
Comment #57
tim.plunkett1/2) They can only add when they have the information they need from other routes. In HEAD they introspect the route provider, here they introspect each collection. I think we both prefer this.
3) I consider them all not being named "RouteSubscriber" a feature. Also, they are listed in *.routing.yml, which is the first place you should look
4) We should. Adding that here.
I also added a @todo about #2158571: Routes added in RouteSubscribers cannot be altered and removing the ALTER.
The interdiff is against #49
Comment #58
Crell CreditAttribution: Crell commentedAfter some discussion in IRC, I'm going to RTBC this.
1) It is a DX improvement on its own.
2) There is no new bug introduced here. Rather, there's some conversion that cannot be fully completed due to pre-existing bugs in config_translation. config_translation is already buggy because it's trying to use the route provider within the route build process, which is a circular dependency. That it hasn't already blown up is purely due to luck.
3) There is already a follow-up to sort out config_translation, which is referenced from this patch.
So let's let this land, then finish fixing config_translation separately. That way we still get the DX improvement for normal dynamic routes now as a Christmas gift to contrib developers. :-)
Comment #59
dawehnerOkay.
Comment #60
Dries CreditAttribution: Dries commentedIt looks like there is consensus, and I find myself in agreement that (b) is the best options. I'm not sure I'd call it 'Major' but it is certainly a DX improvement. Committed to 8.x. Thank you.
Comment #61
sunI'm getting the following error/exception today, even when trying to rebuild via the rebuild script.
Scanning the commit log, it seems to be caused by this change, as it appears to be the only one that touched this area in the past days.
(FWIW, I don't know why the exception is thrown twice, and why the first has a better backtrace and why the second has a better exception message...)
Comment #62
tim.plunkettYes, the provider for those routes has changed, so an old install will get DB collisions.
Comment #63
sunCreated #2160811: Router rebuild fails (even via rebuild script), in case the provider of existing routes has changed
Comment #64
yched CreditAttribution: yched commentedThe "Providing dynamic routes and altering existing routes" handbook page at https://drupal.org/node/2122201 might need an update ?
Comment #65
jibranWe have to update or add new change notice for this.
Comment #66
tim.plunkettAnyone can write this, I might have time soon but I don't want to discourage anyone.
Comment #67
star-szrTagging to write up the change record.
Comment #68
kim.pepperProposed change notice:
Dynamic Routes can be defined in routing.yml
Previously, if you wanted to create routes dynamically, you'd need to create a class that extends RouteSubscriberBase and create a service definition tagged with 'event_subscriber' for it to be picked up.
Now you can declare a dynamic routing method directly in your routing.yml file that returns an array of \Symfony\Component\Routing\Route objects or a Symfony\Component\Routing\RouteCollection object.
Example snippet from my_module.routing.yml:
Comment #69
jibranI think we should add SomeClass.php to the change notice as well.
Comment #70
jibranI am just asking that what is the class definition of
\Drupal\mymodule\Routing\SomeClass
or how the method\Drupal\mymodule\Routing\SomeClass::routes
looks like?Comment #71
kim.pepperAh ok. I'll just add the examples in the issue summary.
Comment #72
kim.pepperMarking fixed.
Comment #73
jibranThank you @kim.pepper for the change notice and update.
Comment #74
damiankloip CreditAttribution: damiankloip commentedWell, we can still use the event based method using a Route subscriber too.