Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Config] Delegate creation of ConfigCache instances to a factory.#14178
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -44,6 +44,7 @@ public function __construct($file, $debug) | |||
| * Gets the cache file path. | |||
| * | |||
| * @return string The cache file path | |||
| * @deprecated since 2.7, to be removed in 3.0. Use getFilePath() instead. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
GetFilePath orgetPath (see the method name below)?
@fabpot Would it make sense to explicitly inject the (default) factory via the DIC in FrameworkBundle, so the factory is there as a service and can be changed easily? That would be easier than having to change the router/translator service definitions. |
| * | ||
| * @param bool $debug The debug flag to pass to ConfigCache. | ||
| */ | ||
| public function __construct($debug) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I suggest to use false as default value for $debug
| * | ||
| * @return ConfigCacheInterface $configCache The cache instance | ||
| */ | ||
| public function cache($file, $callable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we need to change file argument name here and to make it more generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
what about$resource?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't know... I did not intend to change the wayConfigCache works with this PR, so...?
| @@ -36,6 +36,7 @@ | |||
| "symfony/phpunit-bridge": "~2.7|~3.0.0", | |||
| "symfony/browser-kit": "~2.4|~3.0.0", | |||
| "symfony/console": "~2.6|~3.0.0", | |||
| "symfony/config": "~2.7|~3.0.0", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@fabpot is it right to add these? Otherwise a too low version of symfony/config is installed.
…ake tests pass.See for examplehttps://travis-ci.org/symfony/symfony/jobs/57160322 - wrong (too low) versions of symfony/config are installed.
The symfony/http-kernel ~2.7 requires symonfy/config ~2.7 to work. This dependency is not picked up correctly in the Travis builds, thus it is enforced this way.
That should resolve the other dependency problems as well.
…ransitive dependency
…have been merged into it
| @@ -40,6 +40,9 @@ | |||
| "symfony/translation": "~2.0,>=2.0.5|~3.0.0", | |||
| "symfony/var-dumper": "~2.6|~3.0.0" | |||
| }, | |||
| "conflict": { | |||
| "symfony/config": "<2.7" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we need this line? The requirement should been enough, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The require-dev does not work if symfony/config is followed as a transitive dependency during dep=... builds. Soconflict avoids makingsymfony/config a hard requirement, but makes sure we get at least 2.7if we get it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@fabpot Do you prefer a PR with tests "all green", or shall I keep changes to a minimum at the cost of having dep= tests pass only once the change has been merged up to the master branch?
I think this is a special case here because changes are made across components that are then tested independently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok, I thoughtconfig was a hard requirement, makes sense then.
👍 |
@aitboudad I think we all agree that#13986 and this PR and not related. Can you remove your - 1? |
👍 ok :) |
Travis failure on PHP 5.5 is due to avolatile test. |
| /* | ||
| *Old cache returns onlythe catalogue, without resourcesHash | ||
| *Gracefully handle the case thatthecachedcatalogue is in an "old" format, without a resourcesHash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
that -> when
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@cordoval fixed, thanks
Thank you@mpdude. |
… 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#5136In 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.
Thank you for your patience with this over the last 2 years@fabpot :-) |
@mpdude Thanks for your tenacity. Looking forward to your next PR :) |
… (aitboudad)This PR was merged into the 2.7 branch.Discussion----------[Config][cache factory] check type of callback argument.| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Fixed tickets |#14178| Tests pass? | yes| License | MIT/cc@mpdudeCommits-------11f798c [Config][cache factory] check type of callback argument.
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
ConfigCacheFactoryInterfaceis 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
symfony/config~3.0.0incomposer.jsonfor 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).