Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

@ogizanagi
Copy link
Member

ogizanagi commented Feb 26, 2017

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.

@ogizanagi ogizanagi force-pushed the ogizanagi:feature/container_parameters_resource branch 2 times, most recently from b6989c0 to 8423456 Feb 26, 2017
@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Feb 27, 2017
@weaverryan

This comment has been minimized.

Copy link
Member

weaverryan commented Mar 4, 2017

👍 Nice work!

private $parameters;
/**
* @param array $parameters The container parameters values to track

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 5, 2017

Member

I would remove "values" here.

return false;
}
}
} catch (ParameterNotFoundException $exception) {

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 5, 2017

Member

Why not calling hasParameter() instead?

@ogizanagi ogizanagi force-pushed the ogizanagi:feature/container_parameters_resource branch from 8423456 to 5a3a2ce Mar 5, 2017
@ogizanagi ogizanagi force-pushed the ogizanagi:feature/container_parameters_resource branch from 5a3a2ce to fad4d9e Mar 5, 2017
@ogizanagi

This comment has been minimized.

Copy link
Member Author

ogizanagi commented Mar 5, 2017

Thanks for the review. @xabbuh's comments addressed.

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Mar 5, 2017

👍

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Mar 5, 2017

Thank you @ogizanagi.

@fabpot fabpot merged commit fad4d9e into symfony:master Mar 5, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
fabpot added a commit that referenced this pull request Mar 5, 2017
…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
@ogizanagi ogizanagi deleted the ogizanagi:feature/container_parameters_resource branch Mar 5, 2017
@fabpot fabpot mentioned this pull request May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.