Using a service as a router resource #15742

weaverryan · 2015-09-09T15:52:19Z

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

Hi guys!

This adds the ability to use a service as a routing resource. In other words, instead of loading routing.yml, you could load my_route_loader, and then a method would be called on your service to return a RouteCollection.

Specifically, I'm interested in this because it would allow a user to point their main router resource to the kernel itself, making it possible to load routes inside the kernel (making a single-file full-stack app more possible).

Thanks!

jakzal · 2015-09-09T16:26:54Z

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

+     */
+    public function supports($resource, $type = null)
+    {
+        return $type == 'service';


Should be return 'service' === $type.

stof · 2015-09-09T16:55:50Z

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

+     *
+     * @return RouteCollection
+     */
+    public function getRouteCollection(Loader $loader);


please don't typehint an implementation

My problem is that the Loader has import on it, which is really what the user will want. The LoaderInterface doesn't have this. What do you think about creating a new interface for loaders with import to avoid type-hinting the implementation?

weaverryan · 2015-09-10T14:47:18Z

@stof I decided to type-hint against the existing LoaderInterface, as it's the smallest change. I'd still love to have import guaranteed, but I believe you can "import" manually by calling getResolver()->resolve(...)->load() on the LoaderInterface. If there's support for adding some sort of an ImportingLoaderInterface with the import() method, I would be very happy to add that.

I've taken all the feedback into account. This is still ready for review :)

weaverryan · 2015-09-14T00:58:17Z

src/Symfony/Component/Routing/Loader/RouteLoaderInterface.php

+     *
+     * @return RouteCollection
+     */
+    public function getRouteCollection(LoaderInterface $loader);


I've type-hinted the interface, but in reality, I'm passing in the concrete ServiceRouteLoader itself. The difference is that ServiceRouteLoader (like all loaders), has a really handy import() method, while LoaderInterface does not. I'd prefer to type-hint something with the import() method - either the concrete Loader or a new interface.

weaverryan · 2015-09-14T00:58:24Z

Ping @symfony/deciders

dunglas · 2015-09-14T05:26:12Z

Great! Would allow us to remove some magic regarding the router in API platform as well.

lsmith77 · 2015-09-14T05:52:48Z

interesting. would also allow for simple integration of dynamic resources.

/cc @dbu

sstok · 2015-09-14T09:44:57Z

👍 for this feature!

dupuchba · 2015-09-14T10:06:37Z

👍 really great

weaverryan · 2015-09-17T01:43:12Z

The test failure is because FrameworkBundle now depends on a new class inside symfony/router that's also included in this PR (but the tests only pull committed code to symfony/router). Does anyone know if that test is safe to ignore - or if there's a way to fix it?

Apart from fixing or finding out about the test, this is ready for feedback or voting.

dbu · 2015-09-18T08:00:50Z

src/Symfony/Component/Routing/Loader/ServiceRouterLoader.php

+/**
+ * A route loader that executes a service to load the routes.
+ *
+ * The depends on the DependencyInjection component


typo: "The depends on"

dbu · 2015-09-18T08:11:15Z

indeed, good idea!

stof · 2015-09-18T08:21:12Z

src/Symfony/Component/Routing/Loader/ServiceRouterLoader.php

+    private function addClassResource(\ReflectionClass $class, RouteCollection $collection)
+    {
+        do {
+            $collection->addResource(new FileResource($class->getFileName()));


you should check for $class->getFileName() not being false (or null for HHVM) to handle the case of classes extending a class defined in a PHP extension IMO.

This part was stolen from ContainerBuilder: https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/DependencyInjection/ContainerBuilder.php#L275. Is your comment still true? If so - that would be a problem to fix in both places.

xabbuh · 2015-09-23T14:03:08Z

@weaverryan The failure for the low tests looks strange. Does it make a difference when you mock the ContainerInterface instead?

xabbuh · 2015-09-23T14:12:40Z

What I do not understand is the reason to have a container aware routing loader. Wouldn't it be easier if this loader were more like a registry where you could register RouteLoaderInterface instances with arbitrary keys (the FrameworkBundle could ship with a compiler pass so that you can just tag your services and they would be registered automatically)?

weaverryan · 2015-09-23T15:18:04Z

@xabbuh I understand your thought, but the purpose of this is different. I'm not aiming to make it possible to have RouteLoaderInterface services automatically called because they're registered+tagged. I want to keep loading routes explicit, just add one more way to load routes - via PHP in a real method+service (currently, you can only add routes via PHP in a flat-file).

weaverryan · 2015-09-24T02:45:26Z

@xabbuh yes - you were right about one of the tests - I've fixed that now.

I believe the last failing test should be ignored: the functional tests in FrameworkBundle are not able to fetch the router service because it depends on the ServiceRouteLoader, which isn't included in the Symfony's core (since it's part of this PR). I don't think there's way to fix that (someone let me know if I'm wrong), but the tests should pass fine once merged.

Thanks!

xabbuh · 2015-09-24T06:51:03Z

@weaverryan I just mean that it would only be possible to use these services in your routing configuration if they have been registered that way before. The workflow would still be the same then with your approach. The only difference is that you would not try to locate the service in the container (the drawback might be that you then won't be able to lazy load the routing loader services though).

weaverryan · 2015-09-26T14:27:01Z

@xabbuh Ah, I understand better now. I'd still prefer to keep it this way - it feels logical to me that I should only need to register a class as a service and point to it in the config - the tagging seems unnecessary.

xabbuh · 2015-09-27T14:44:52Z

👍 (being able to load services lazily and not having to tag your services are good reasons for keeping the container awareness)

…component

weaverryan · 2015-09-30T23:13:16Z

Adding a comment that got covered up:

We could remove the RouteLoaderInterface and instead have this method return a callable.

The difference would then be that we'd probably need to include the method name in the "resource" when using this:

admin_routes:
    resource: 'admin_route_loader::loadRoutes'
    type: service

That would solve the need for the RouteLoaderInterface on the "Micro" kernel. It's also consistent with how event listeners, for example, work.

fabpot · 2015-10-01T06:37:05Z

@weaverryan Indeed, that would be much better. IIRC, when using a service name, the convention is to use only one colon: admin_router_loader:loadRoutes

fabpot · 2015-10-01T13:38:41Z

#15990 has been updated to work with the new service loader.

weaverryan · 2015-10-01T13:55:27Z

@fabpot I've already done work locally on this PR for the kernel:methodName syntax - I'll be able to push it within the next 2 hours.

weaverryan · 2015-10-01T18:15:14Z

@fabpot a little later, but I just updated the PR:

This uses the service_name:method notation and is enforced in the ObjectRouteLoader, instead of allowing for more generic usage, where sub-classes could resolve strings of any format. I was trying to limit it to work just one way. For example, if you're not worried about the cached container and want to load routes via some "Callable", you cannot do it via the ObjectLoader: use the ClosureLoader.

ping also @symfony/deciders

aitboudad · 2015-10-01T18:27:55Z

👍 , can you update the CHANGELOG.

weaverryan · 2015-10-01T18:35:06Z

done!

fabpot · 2015-10-01T19:23:18Z

src/Symfony/Component/Routing/Loader/ObjectRouteLoader.php

+        $loaderObject = $this->getServiceObject($serviceString);
+
+        if (!is_object($loaderObject)) {
+            throw new \LogicException(sprintf('%s:getServiceObject() must return an object: %s returned', get_class($this), $loaderObject));


Here, $loaderObject can be anything; I would return the type instead.

very good catch - that's what I had meant to do. Fixed now!

fabpot · 2015-10-01T19:25:09Z

I made a small comment; 👍

weaverryan · 2015-10-01T19:27:23Z

comment addressed!

fabpot · 2015-10-01T19:28:25Z

I think this one is ready now, ping @symfony/deciders

This PR was squashed before being merged into the 2.8 branch (closes #15778). Discussion ---------- Fluid interface for building routes in PHP | 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 ```php $routes->import('routing.yml'); ``` B) Fluid addition of routes into the collection ```php $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: ```php $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! Commits ------- 15ba2e8 Fluid interface for building routes in PHP

fabpot · 2015-10-01T20:34:36Z

Thank you @weaverryan.

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

jakzal reviewed Sep 9, 2015
View reviewed changes

jakzal added the FrameworkBundle label Sep 9, 2015

stof reviewed Sep 9, 2015
View reviewed changes

weaverryan force-pushed the service_route_loader branch from 21cc518 to 6fb9d91 Compare September 14, 2015 00:53

weaverryan reviewed Sep 14, 2015
View reviewed changes

weaverryan mentioned this pull request Sep 14, 2015

Fluid interface for building routes in PHP #15778

Closed

weaverryan mentioned this pull request Sep 17, 2015

Adding a MicroKernel that holds service configuration #15820

Closed

dbu reviewed Sep 18, 2015
View reviewed changes

stof reviewed Sep 18, 2015
View reviewed changes

weaverryan force-pushed the service_route_loader branch 2 times, most recently from 608f304 to 764722d Compare September 26, 2015 20:20

weaverryan added 7 commits September 30, 2015 19:07

Fixing comment typo

ee90412

Fixing test - bad mock

de4143a

Fixing bad mock of "abstract" class

eb47a69

Changing this to be an abstract ObjectRouteLoader

3cb24eb

Adding a ServiceRouterLoader that works with the DependencyInjection …

e431fc7

…component

Renaming abstract method and going back to the key "service" as the type

43e9ad2

fabbot

7ed22ce

weaverryan force-pushed the service_route_loader branch from c9e31aa to 7ed22ce Compare September 30, 2015 23:07

Updating ObjectRouteLoader to use a service_id:methodName format

6b0a5c6

weaverryan force-pushed the service_route_loader branch from cd40cad to 6b0a5c6 Compare October 1, 2015 18:10

updating CHANGELOG

a4af474

fabpot reviewed Oct 1, 2015
View reviewed changes

Fixing missing gettype

a2cb797

fabpot closed this in 99745e1 Oct 1, 2015

xabbuh mentioned this pull request Oct 1, 2015

Ability to load routes from objects/services symfony/symfony-docs#5747

Closed

fabpot mentioned this pull request Nov 16, 2015

Release v2.8.0-BETA1 #16550

Merged

xabbuh mentioned this pull request Mar 30, 2016

[RFC] Provide event for collecting routes from bundles symfony-cmf/routing-bundle#343

Closed

Uh oh!

Using a service as a router resource #15742

Using a service as a router resource #15742

Uh oh!

Conversation

weaverryan commented Sep 9, 2015

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!

weaverryan commented Sep 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan commented Sep 14, 2015

Uh oh!

dunglas commented Sep 14, 2015

Uh oh!

lsmith77 commented Sep 14, 2015

Uh oh!

sstok commented Sep 14, 2015

Uh oh!

dupuchba commented Sep 14, 2015

Uh oh!

weaverryan commented Sep 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Sep 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Sep 23, 2015

Uh oh!

xabbuh commented Sep 23, 2015

Uh oh!

weaverryan commented Sep 23, 2015

Uh oh!

weaverryan commented Sep 24, 2015

Uh oh!

xabbuh commented Sep 24, 2015

Uh oh!

weaverryan commented Sep 26, 2015

Uh oh!

xabbuh commented Sep 27, 2015

Uh oh!

weaverryan commented Sep 30, 2015

Uh oh!

fabpot commented Oct 1, 2015

Uh oh!

fabpot commented Oct 1, 2015

Uh oh!

weaverryan commented Oct 1, 2015

Uh oh!

weaverryan commented Oct 1, 2015

Uh oh!

aitboudad commented Oct 1, 2015

Uh oh!

weaverryan commented Oct 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Oct 1, 2015

Uh oh!

weaverryan commented Oct 1, 2015

Uh oh!

fabpot commented Oct 1, 2015

Uh oh!

fabpot commented Oct 1, 2015

Uh oh!

Uh oh!