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
merged 1 commit into from Mar 5, 2017
Merged

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

merged 1 commit into from Mar 5, 2017

Conversation

ogizanagi
Copy link
Member

@ogizanagi 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.

@carsonbot carsonbot added Status: Needs Review DependencyInjection Routing DX DX = Developer eXperience (anything that improves the experience of using Symfony) Bug Feature labels Feb 26, 2017
@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Feb 27, 2017
@weaverryan
Copy link
Member

👍 Nice work!

private $parameters;

/**
* @param array $parameters The container parameters values to track
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove "values" here.

return false;
}
}
} catch (ParameterNotFoundException $exception) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not calling hasParameter() instead?

@ogizanagi
Copy link
Member Author

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

@xabbuh
Copy link
Member

xabbuh commented Mar 5, 2017

👍

@fabpot
Copy link
Member

fabpot commented Mar 5, 2017

Thank you @ogizanagi.

@fabpot fabpot merged commit fad4d9e into symfony:master Mar 5, 2017
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 feature/container_parameters_resource branch March 5, 2017 19:48
@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
Labels
Bug DependencyInjection DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Routing Status: Needs Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants