Skip to content

[RFC] [Config] Use a Factory for ConfigCache instances #7781

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

Closed
wants to merge 3 commits into from
Closed

[RFC] [Config] Use a Factory for ConfigCache instances #7781

wants to merge 3 commits into from

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented Apr 21, 2013

This PR has been replaced by #14178.

@mpdude
Copy link
Contributor Author

mpdude commented Nov 24, 2013

Rebased on master.

By using such an exchangeable ConfigCache factory it becomes possible to implement different resource (freshness) checking strategies, especially service-based ones.

The goal is to be able to validate Translators and Routers generated by database-based loaders. It might also help with symfony/AsseticBundle#168.

I think this is the smallest set of changes from the long-standing #7230 necessary to implement the rest of it as a bundle outside of Symfony itself (in case you don't want/like it).

Tests will follow once I've got some feedback on this. The relevant code sections in the Router (with ConfigCache enabled) seem to be not covered by tests at all at the moment.

@jderusse
Copy link
Member

@mpdude mpdude changed the title Delegate creation of ConfigCache instances to a factory. [RFC] [Config] Use a Factory for ConfigCache instances Oct 7, 2014
@fabpot
Copy link
Member

fabpot commented Apr 1, 2015

👍

ping @symfony/deciders

@fabpot
Copy link
Member

fabpot commented Apr 1, 2015

@mpdude Can you create a doc issue (a PR would be even better) on how to use the new classes; I think it can be a quick cookbook entry.

@@ -308,7 +332,7 @@ public function getGenerator()
/**
* @return GeneratorDumperInterface
*/
Copy link
Member

Choose a reason for hiding this comment

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

Can you add an @internal tag here as making it public is only because of limitations in PHP 5.3. This should be converted back to protected in 3.0.

@aitboudad
Copy link
Contributor

@fabpot for the translator wouldn't be better to use #13986 ?

@xabbuh
Copy link
Member

xabbuh commented Apr 1, 2015

In #11373, the cache handling was moved to the Translator of the component. Does that need to be reflected here (the changes done here may also conflict with the recent change in the 2.7 branch).

@fabpot
Copy link
Member

fabpot commented Apr 1, 2015

@mpdude is going to update this PR to make it work for 2.7 really soon now.

@Tobion
Copy link
Contributor

Tobion commented Apr 1, 2015

What is the use-case? In general looks ok but without a different implementation of the factory, it doesn't seem that useful alone.

@mpdude
Copy link
Contributor Author

mpdude commented Apr 1, 2015

@Tobion see the comment above. The ultimate goal is something along the lines of #7230. This PR covers only the minimum change needed to make the flex point available so the rest could be provided as a 3rd party bundle (at least for the beginning).

@mpdude
Copy link
Contributor Author

mpdude commented Apr 2, 2015

I just created #14178 for this change on the 2.7 branch. (Or can I somehow change the base branch for a PR in GitHub?)

@fabpot
Copy link
Member

fabpot commented Apr 2, 2015

@mpdude No, you cannot, closing this one.

@fabpot fabpot closed this Apr 2, 2015
fabpot added a commit that referenced this pull request Apr 8, 2015
… a factory. (mpdude)

This PR was squashed before being merged into the 2.7 branch (closes #14178).

Discussion
----------

[Config] Delegate creation of ConfigCache instances to a factory.

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes (refactoring, new flex point)
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | we'll see :-)
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | symfony/symfony-docs#5136

In the Routing/Router and Translation/Translator, delegate creation of ConfigCache instances to a factory. The factory can be setter-injected but will default to a BC implementation.

The ```ConfigCacheFactoryInterface``` is designed in a way that captures the common ```$cache = new ...; if (!$cache->isFresh()) { ... do sth }``` pattern. But more importantly, this design allows factory implementations to take additional measures to avoid race conditions before actually filling the cache.

By using an exchangeable ConfigCache factory it becomes possible to implement different resource (freshness) checking strategies, especially service-based ones.

The goal is to be able to validate Translators and Routers generated by database-based loaders. It might also help with symfony/AsseticBundle#168. This PR only contains the minimum changes needed, so the rest could be implemented in a bundle outside the core (at least for the beginning).

Component/HttpKernel/Kernel::initializeContainer still uses the ConfigCache implementation directly as there is no sensible way of getting/injecting a factory service (chicken-egg).

This is a pick off #7230. It replaces #7781 which was against the master branch. Also see #7781 for additional comments/explanations.

## Todo

* [ ] Allow `symfony/config` `~3.0.0` in `composer.json` for the HttpKernel and Translator component as well as TwigBundle once this PR has been merged into the master branch (fail deps=high tests for the time being).

Commits
-------

6fbe9b1 [Config] Delegate creation of ConfigCache instances to a factory.
@mpdude mpdude deleted the issue-7230/picks/cache-factory branch April 8, 2015 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants