Fluid interface for building routes in PHP #15778

weaverryan · 2015-09-14T01:16:49Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	not yet...

This - along with #15742 - attempts to making adding routes in PHP (via an actual class+method) not only possible, but also useful.

The two classes - Route and RouteCollectionBuilder are based off of Silex's Controller and ControllerCollection. The RouteCollectionBuilder is basically a RouteCollection that's able to import other resources. Here are the goals:

A) Import routes easily

$routes->import('routing.yml');

B) Fluid addition of routes into the collection

$routes->add('/admin', 'AppBundle:Admin:index', 'admin_index')
    ->setMethods(['GET']);

C) Ability to create routes with auto-generating names

D) Ability to add a "sub-collection" (kind of like an import, without pointing to another file). Included is the ability to set the controller class:

$blogRoutes = $routes->createBuilder('/blog')
   ->setControllerClass('AppBundle\Controller\BlogController');
$blogRoutes->add('/', 'indexAction');
$blogRoutes->add('/{id}', 'editAction');
$routes->addBuilder($blogRoutes);

E) The collection options can be set before or after the routes. With RouteCollection, if you set something - e.g. a prefix or a default - and THEN add more routes, those options are not passed to those routes. This is by design, but not ideal for building routes (e.g. in the previous code example, the controllerClass would not be applied using the opposite logic, since it's set before adding the routes).

Thanks!

weaverryan · 2015-09-14T01:17:39Z

src/Symfony/Bundle/FrameworkBundle/Routing/RouteCollectionBuilder.php

+    /**
+     * @param Loader $loader
+     */
+    public function __construct(Loader $loader)


Like in #15742, this could be LoaderInterface instead... which I believe would be ok, since I think we could still "import", just using the longer syntax below (which the user wouldn't need to be bothered with).

IMO, the interface should be used. We should not encourage typehinting against implementations

sstok · 2015-09-14T10:00:15Z

👍 for this feature, really like this :)

stloyd · 2015-09-14T10:31:23Z

Hmmmm... http://ocramius.github.io/blog/fluent-interfaces-are-evil/

stof · 2015-09-14T10:36:32Z

@stloyd read the blog post you linked. Builders are precisely the place where they are not evil

stloyd · 2015-09-14T10:44:34Z

@stof I read that blog post & I checked the proposed code, as you can see that there is builder (for collection) & new class Route which is not real builder (if it would be real builder I would not mention this post)...

weaverryan · 2015-09-16T01:57:45Z

Ping @symfony/deciders

There are some deprecated warnings being triggered by this PR on some PHP version - I haven't tracked that down yet, but it's a non-issue.

stof · 2015-09-16T07:56:16Z

@weaverryan IMO, it is not a non issue

stof · 2015-09-16T07:57:47Z

src/Symfony/Bundle/FrameworkBundle/Routing/RouteCollectionBuilder.php

+        $controller = $route->getDefault('_controller');
+
+        // only apply controller class to a (non-empty) string
+        if (!is_string($controller) || !$controller) {


I would say '' === $controller. It would be more readable

It's not quite the same thing - I'm also filtering for other things, like array()s

weaverryan · 2015-09-16T14:05:09Z

@stof without looking into it too far, you don't know why deprecation warnings would be thrown only when "deps" isn't set (deps=high and deps=low looks good)? I can't trigger the same deprecation warnings locally with this code, nor can I see how it's possible that this part of the code is being executed. It is an issue that will need to be resolved before merging, but fundamentally, the PR won't need to use deprecated functionality. I just have to track it down :)

weaverryan · 2015-09-17T02:57:56Z

Nevermind - tests should now be green.

xabbuh · 2015-09-24T06:40:11Z

src/Symfony/Bundle/FrameworkBundle/Routing/RouteCollectionBuilder.php

+     */
+    public function addResource(ResourceInterface $resource)
+    {
+        $this->resources[] = $resource;


I think we should check before if the configured loader actually supports the given resource.

This purposefully mirrors RouteCollection::addResource(), and I think we should keep it dumb: I could choose to add a "resource" here for the purposes of cache-clearing, even if my loader doesn't support that resource (perhaps I know better than the loader, that this resource was in fact used to determine some route information).

xabbuh · 2015-09-27T09:41:25Z

LGTM 👍

weaverryan · 2015-09-28T23:55:18Z

Status: Reviewed

I'm really hoping this will be considered for 2.8 to make adding PHP routes easier. I've already trimmed it down a lot to make it an easier merge. If I need to, I can remove the setControllerClass() functionality, as it looks like the most controversial part.

fabpot · 2015-09-29T06:37:25Z

src/Symfony/Bundle/FrameworkBundle/Routing/RouteCollectionBuilder.php

+    /**
+     * @param LoaderInterface $loader
+     */
+    public function __construct(LoaderInterface $loader)


What about making $loader optional? If not passed, throw an exception when the user calls import(). Everything else would still work.

…ut how to write this

weaverryan · 2015-09-30T23:33:27Z

Status: Needs Review

fabpot · 2015-10-01T06:24:07Z

👍

xabbuh · 2015-10-01T14:28:26Z

src/Symfony/Component/Routing/RouteCollectionBuilder.php

+    public function import($resource, $type = null)
+    {
+        /** @var RouteCollection $collection */
+        $collection = $this->load($resource, $type);


The load() message can throw a FileLoaderLoadException which should be documented in the docblock.

Do we add that here, even though it's a second method call that throws the exception? I'm actually not sure what the standard is here.

I'd say we should do that here. The nested method call is a private method and people will never realise they may have to be aware of the exception if we don't document that here.

makes sense to me - I've added it - easy to remove if others don't want it

xabbuh · 2015-10-01T19:29:17Z

src/Symfony/Component/Routing/RouteCollectionBuilder.php

+     * @param string      $controller The route's controller
+     * @param string|null $name       The name to give this route
+     *
+     * @return Route


This is a bit counter-intuitive as almost all other methods return the builder itself. Though probably there is no better solution.

I almost wrote the same comment but I think it makes sense for this one to return the route.

The name is disturbing, you don not expect a Route. I think add should do what addRoute does (just adding a route to the collection), and then this one, like in the ContainerBuilder should be named register or something like that ? cf https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/DependencyInjection/ContainerBuilder.php#L728

I tried to make this method the most easily guessable, since you will call this much more often than addRoute(). If we called this register(), I think people would be looking for a method like add() or addRoute(), and might try calling addRoute() instead.

It also makes sense, as there is no set method in the ContainerBuilder too (it's directly setDefinition)

When looking at the code written with this builder, add() looks much better and I think it's quite intuitive.

xabbuh · 2015-10-01T19:29:44Z

👍

Status: Reviewed

fabpot · 2015-10-01T19:32:02Z

👍

aitboudad · 2015-10-01T19:47:22Z

👍

fabpot · 2015-10-01T20:33:07Z

Thank you @weaverryan.

Tobion · 2015-10-26T11:09:25Z

src/Symfony/Component/Routing/RouteCollectionBuilder.php

+    }
+
+    /**
+     * Creates the final ArrayCollection, returns it, and clears everything.


ArrayCollection?

wouterj · 2015-10-28T19:19:57Z

These exciting features really deserve some docs before the stable release 😉

weaverryan · 2015-10-28T20:06:54Z

I will definitely be taking care of that :)

…d (xabbuh) This PR was merged into the 2.8 branch. Discussion ---------- [Routing] fix docblock description for the build() method | Q | A | ------------- | --- | Fixed tickets | #15778 | License | MIT Commits ------- ca32ed4 fix docblock description for the build() method

Tobion · 2015-10-30T03:00:43Z

src/Symfony/Component/Routing/RouteCollectionBuilder.php

+    public function add($path, $controller, $name = null)
+    {
+        $route = new Route($path);
+        $route->setDefault('_controller', $controller);


The routing component doesn't know anything about a _controller. So this seems misplaced. Such assumption can only be made in the framework bundle.

@Tobion you're right that the Routing component doesn't know about _controller. But HttpKernel does know about this, and I wanted this to be available for things like Drupal. Since there is no bridge between HttpKernel and Routing, I put it here, which purposefully makes this class useable only by people who follow this convention. It's not perfect, but it's the best place. Of course, if you wanted to use this in another system, you could override this method.

If HttpKernel does, why not putting it in HttpKernel instead?

If we put it in HttpKernel, someone will ask why we don't put it in Routing :)

And then we have a clear answer. HttpKernel contains classes that add just a little "HttpKernel convention sauce" on top of classes provided by other components (DI Extension, File locator, ...)

Good point. My counterpoint is that it makes sense to use this without HttpKernel - maybe you have a system where _controller is meaningful or you override this method. That should be possible without requiring http-kernel, which this class doesn't actually depend on.

So there's no perfect spot, but I think this is best

…tionBuilder (weaverryan) This PR was merged into the 2.8 branch. Discussion ---------- Re-adding the ability to add a resource to the RouteCollectionBuilder | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a At the end of #15778, I removed the ability to add a resource to the RouteCollectionBuilder. But, this is needed (at least internally): if you import, the returned RouteCollection may have resources, which need to be brought forward into the new RouteCollectionBuilder (the code in `import()` to do this already exists, but is calling an undefined `addResource()` method). This adds the test with minimal code to fix. P.S. Fabien told me to remove `addResource` originally... so isn't this *kind of* his fault? Commits ------- 3b67daa Re-adding the ability to add a resource to the RouteCollectionBuilder

carsonbot added the Status: Needs Review label Sep 14, 2015

weaverryan reviewed Sep 14, 2015
View reviewed changes

weaverryan mentioned this pull request Sep 15, 2015

Introduction of syntax/shortcut aware Definition #15800

Closed

weaverryan force-pushed the route_builder branch 2 times, most recently from f7dd327 to c87515a Compare September 16, 2015 01:41

stof reviewed Sep 16, 2015
View reviewed changes

xabbuh reviewed Sep 24, 2015
View reviewed changes

weaverryan force-pushed the route_builder branch from b309f39 to 678e622 Compare September 26, 2015 20:16

weaverryan mentioned this pull request Sep 27, 2015

[WIP] Route building in the kernel #15948

Closed

weaverryan force-pushed the route_builder branch from 678e622 to 0a74034 Compare September 28, 2015 23:53

carsonbot added Status: Reviewed and removed Status: Needs Review labels Sep 28, 2015

fabpot reviewed Sep 29, 2015
View reviewed changes

weaverryan added 2 commits September 30, 2015 18:56

stretching out logic into multiple lines: there's some discussion abo…

83f3194

…ut how to write this

Fabbot

26d656b

carsonbot added Status: Needs Review and removed Status: Needs Work labels Sep 30, 2015

xabbuh reviewed Oct 1, 2015
View reviewed changes

Adding throws

42e73a2

xabbuh reviewed Oct 1, 2015
View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 1, 2015

fabpot closed this in 54e3d71 Oct 1, 2015

xabbuh mentioned this pull request Oct 1, 2015

Documentation for the route builder symfony/symfony-docs#5748

Closed

Tobion reviewed Oct 26, 2015
View reviewed changes

weaverryan mentioned this pull request Oct 28, 2015

Re-adding the ability to add a resource to the RouteCollectionBuilder #16361

Merged

xabbuh mentioned this pull request Oct 28, 2015

[Routing] fix docblock description for the build() method #16374

Merged

Tobion reviewed Oct 30, 2015
View reviewed changes

weaverryan deleted the route_builder branch November 5, 2015 16:18

fabpot mentioned this pull request Nov 16, 2015

Release v2.8.0-BETA1 #16550

Merged

Tobion mentioned this pull request Jun 28, 2019

[Routing] deprecate RouteCollectionBuilder #32240

Closed

Uh oh!

Fluid interface for building routes in PHP #15778

Fluid interface for building routes in PHP #15778

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!