[FrameworkBundle][Router][DX] Invalidate routing cache when container has changed #21443

ogizanagi · 2017-01-28T14:54:41Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#21426
License	MIT
Doc PR	N/A

This tries to fix #21426 by adding the container class as a resource of the main RouteCollection, allowing the router to invalidate its cache if the container has changed.

There are many places & ways this could have been done:

From the component Router class, by adding a router option/method to accept arbitrary resources to add.
From the framework bundle Router class, by adding a specific container_class option.
From the framework bundle DelegatingLoader class.
From the framework bundle Router class, directly from the container parameters (current implementation)

WDYT?

~~I consider this as a DX enhancement rather than a real bug fix though, hence this PR targets master.~~

sstok · 2017-01-29T11:51:42Z

👍 for Router::addResource method, I actually faced a similar problem in the https://github.com/rollerworks/RouteAutowiringBundle which I solved by registering the resources with each RouteCollectionBuilder.

Being able to do this in at the router service directly is much simpler, more flexible, (and in my case) reduces the method calls for each collection.

nicolas-grekas · 2017-01-29T19:13:58Z

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

@@ -52,6 +53,10 @@ public function getRouteCollection()
    {
        if (null === $this->collection) {
            $this->collection = $this->container->get('routing.loader')->load($this->resource, $this->options['resource_type']);
+
+            $containerClassPath = "{$this->container->getParameter('kernel.cache_dir')}/{$this->container->getParameter('kernel.container_class')}.php";


wouldn't sprintf be better here?

nicolas-grekas · 2017-01-29T19:15:13Z

why not consider this as a bug fix?

ogizanagi · 2017-01-29T19:20:25Z

@nicolas-grekas : Mainly because it may require a new feature and it only targets a developer experience enhancement. But the current implementation is fine if we consider it a bug fix and could go to lower branches (the other implementations aren't).

nicolas-grekas · 2017-01-29T19:54:49Z

Which other implems?
Anyway, if this applies to lower branches, I'd personally be OK for bug fix.

ogizanagi · 2017-01-29T19:57:15Z

Other implementations proposed in the PR description, along with @sstok's one (which is the first suggestion in PR description) 😄

But if this one if fine for everyone, let's target 2.7 as a bug fix.

ogizanagi · 2017-01-29T20:06:48Z

Rebased on 2.7 and PR description updated.
Let's see if everyone agrees.

nicolas-grekas

👍

fabpot · 2017-02-01T00:09:33Z

In your commit, there is a src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Router/var/cache/appContainer.php file that should not be there.

ogizanagi · 2017-02-01T07:00:28Z

This file is actually required by the test case, in order to not throw on FileResource instantiation from the Router and the test case itself. It's empty as I don't need more as I only test the resource is added to the route collection. But I could add some content to be more explicit on its purpose? (Or creating it from the test case setUp if you prefer that)

ogizanagi · 2017-02-12T19:54:21Z

src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Router/var/cache/appContainer.php file removed. Creating the file in tmp dir during setUpBeforeClass instead, so it won't fail on upper branches where FileResource expects the file to exist.

chalasr · 2017-02-12T20:06:49Z

src/Symfony/Bundle/FrameworkBundle/Tests/Routing/RouterTest.php

+
+    public static function tearDownAfterClass()
+    {
+        (new Filesystem())->remove(static::$cachePath);


I believe this is not supported before 5.4

… has changed

nicolas-grekas · 2017-02-14T14:33:06Z

Just wondering: did we try hard enough to not invalidate when not required? It's not an issue to invalidate often - until DX is affected by the time it takes to recompile everything (routes + container).

weaverryan · 2017-02-22T19:47:53Z

I just had a user hit this bug, so happy to see the PR! But I was also wondering the same thing as @nicolas-grekas - it's kind of a shame to always rebuild the routing cache when the container is rebuilt, just to catch the edge case of someone modifying a parameter only. A more targeted method would be to only clear when the parameters themselves change. But, this PR is really easy because it took advantage of existing functionality (just add the container as a resource, done!).

Would we be interested in only clearing when parameters change? And if so, how could we do that in some simple way? Would we need to suddenly dump some sort of parameters file separately? Could/should the framework Router create its own file (e.g. with parameters in them) and add that as a resource?

ogizanagi · 2017-02-22T23:26:34Z

A more targeted method would be to only clear when the parameters themselves change. But, this PR is really easy because it took advantage of existing functionality (just add the container as a resource, done!).

Indeed, I wanted to do so, but I didn't know how to achieve it simply...yet.
Do you think bf29783 would be a decent solution?

Another one: the PhpDumper could accept a new parameters_file option and:

retrieve the list of parameters through Container::getParameterBag()->all()
compare it to a previously dumped list of parameters if the file exists.
dump the parameters file if different, so the file timestamp is only changed when the parameters changed.

Then, the router (or anyone else) can add it as a FileResource.

But maybe I'm missing a more obvious way.

On another hand, I'm not sure rebuilding the routing cache when the container changes would affect that much DX experience in a medium sized application. So whether or not we opt for a new feature to solve this issue more cleverly, should we merge this one to make lower branches benefits from this fix? (depending on the answers, I may open another PR on master, or replace this one)

weaverryan · 2017-02-25T21:19:17Z

Actually, I had an idea: in Router::resolve(), we resolve the %parameters%. Could we somehow get a list of all of the parameters that were resolved (and their values) and create a new type of ResourceInterface class (e.g. ContainerParameterResource) with these inside. We would also then need a new "resource checker" (class that implements ResourceCheckerInterface) that's capable of handling the new ContainerParameterResource. It would compare the current parameter+values to the ones cached in ContainerParameterResource to see if there are any differences.

This would add 2 new classes... but would fix this bug without ever unnecessarily rebuilding the routing cache.

ogizanagi · 2017-02-25T22:19:07Z

Indeed, that should do it and allows to track only required parameters.
I've pushed a new commit to master...ogizanagi:feature/container_parameters_resource and will submit a new PR on master with tests tomorrow if it matches your expectations.

Or I can replace this one if you do think we should really avoid tracking the whole container on lower branches.

ogizanagi · 2017-02-26T10:54:04Z

Closing in favor of #21767 :)

…er parameters changed (ogizanagi) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI][Router][DX] Invalidate routing cache when container parameters changed | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21426 | License | MIT | Doc PR | N/A Supersedes #21443 but only for master. Indeed, this implementation uses a new feature: a `ContainerParametersResource` which compares cached containers parameters (collected at some point, here by the `Router`) with current ones in the container. On the contrary of the previous PR targeting 2.7, this will only invalidate routing cache when parameters actually used in the routes changed and will avoid always rebuilding the routing cache when the container is rebuilt, just to catch the edge case of someone modifying a parameter. Commits ------- fad4d9e [DI][Router][DX] Invalidate routing cache when container parameters changed

carsonbot added Status: Needs Review FrameworkBundle Routing DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature labels Jan 28, 2017

ogizanagi mentioned this pull request Jan 28, 2017

Cache is not invalidated for routes with container parameters #21426

Closed

nicolas-grekas reviewed Jan 29, 2017

View reviewed changes

ogizanagi changed the base branch from master to 2.7 January 29, 2017 20:04

nicolas-grekas approved these changes Jan 31, 2017

View reviewed changes

nicolas-grekas added this to the 2.7 milestone Jan 31, 2017

chalasr reviewed Feb 12, 2017

View reviewed changes

[FrameworkBundle][Router][DX] Invalidate routing cache when container…

827c12a

… has changed

ogizanagi mentioned this pull request Feb 26, 2017

[DI][Router][DX] Invalidate routing cache when container parameters changed #21767

Merged

ogizanagi closed this Feb 26, 2017

ogizanagi deleted the feature/router/add_container_as_res branch April 12, 2017 21:39

Uh oh!

[FrameworkBundle][Router][DX] Invalidate routing cache when container has changed #21443

[FrameworkBundle][Router][DX] Invalidate routing cache when container has changed #21443

Uh oh!

Conversation

ogizanagi commented Jan 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sstok commented Jan 29, 2017

Uh oh!

nicolas-grekas Jan 29, 2017

Choose a reason for hiding this comment

Uh oh!

ogizanagi Jan 29, 2017

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jan 29, 2017

Uh oh!

ogizanagi commented Jan 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Jan 29, 2017

Uh oh!

ogizanagi commented Jan 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogizanagi commented Jan 29, 2017

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

fabpot commented Feb 1, 2017

Uh oh!

ogizanagi commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogizanagi commented Feb 12, 2017

Uh oh!

chalasr Feb 12, 2017

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Feb 14, 2017

Uh oh!

weaverryan commented Feb 22, 2017

Uh oh!

ogizanagi commented Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weaverryan commented Feb 25, 2017

Uh oh!

ogizanagi commented Feb 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogizanagi commented Feb 26, 2017

Uh oh!

Uh oh!

ogizanagi commented Jan 28, 2017 •

edited

Loading

ogizanagi commented Jan 29, 2017 •

edited

Loading

ogizanagi commented Jan 29, 2017 •

edited

Loading

ogizanagi commented Feb 1, 2017 •

edited

Loading

ogizanagi commented Feb 22, 2017 •

edited

Loading

ogizanagi commented Feb 25, 2017 •

edited

Loading