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

mpdude · 2015-04-02T13:37:00Z

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).

fabpot · 2015-04-02T13:39:32Z

src/Symfony/Component/Config/ConfigCache.php

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


GetFilePath or getPath (see the method name below)?

mpdude · 2015-04-02T13:55:35Z

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

aitboudad · 2015-04-02T14:40:15Z

src/Symfony/Component/Config/DefaultConfigCacheFactory.php

+     *
+     * @param bool $debug The debug flag to pass to ConfigCache.
+     */
+    public function __construct($debug)


I suggest to use false as default value for $debug

aitboudad · 2015-04-03T16:34:46Z

src/Symfony/Component/Config/ConfigCacheFactoryInterface.php

+     *
+     * @return ConfigCacheInterface $configCache The cache instance
+     */
+    public function cache($file, $callable);


we need to change file argument name here and to make it more generic.

what about $resource?

I don't know... I did not intend to change the way ConfigCache works with this PR, so...?

aitboudad · 2015-04-03T18:00:14Z

@mpdude I attempt to implement this feature into #13986

mpdude · 2015-04-04T17:48:46Z

src/Symfony/Bundle/FrameworkBundle/composer.json

@@ -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",


@fabpot is it right to add these? Otherwise a too low version of symfony/config is installed.

…ake tests pass. See for example https://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

fabpot · 2015-04-07T10:11:01Z

src/Symfony/Component/HttpKernel/composer.json

@@ -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"


Do we need this line? The requirement should been enough, no?

The require-dev does not work if symfony/config is followed as a transitive dependency during dep=... builds. So conflict avoids making symfony/config a hard requirement, but makes sure we get at least 2.7 if we get it.

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

ok, I thought config was a hard requirement, makes sense then.

fabpot · 2015-04-07T10:21:48Z

👍

fabpot · 2015-04-07T10:22:58Z

@aitboudad I think we all agree that #13986 and this PR and not related. Can you remove your - 1?

aitboudad · 2015-04-07T10:23:54Z

👍 ok :)

mpdude · 2015-04-07T10:30:04Z

Travis failure on PHP 5.5 is due to a volatile test. dep=high fails because changes to symfony/config are not yet visible in dev-master.

cordoval · 2015-04-07T12:38:21Z

src/Symfony/Component/Translation/Translator.php


        /*
-         * Old cache returns only the catalogue, without resourcesHash
+         * Gracefully handle the case that the cached catalogue is in an "old" format, without a resourcesHash


that -> when

@cordoval fixed, thanks

fabpot · 2015-04-08T05:21:42Z

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#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 · 2015-04-08T06:08:25Z

Thank you for your patience with this over the last 2 years @fabpot :-)

fabpot · 2015-04-08T06:30:28Z

@mpdude Thanks for your tenacity. Looking forward to your next PR :)

@mpdude

… (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 @mpdude Commits ------- 11f798c [Config][cache factory] check type of callback argument.

mpdude mentioned this pull request Apr 2, 2015

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

Closed

fabpot reviewed Apr 2, 2015
View reviewed changes

mpdude mentioned this pull request Apr 2, 2015

Write a short cookbook entry how to make use of the ConfigCacheFactory in 2.7 symfony/symfony-docs#5136

Closed

mpdude changed the title ~~Delegate creation of ConfigCache instances to a factory.~~ [Config][Routing][Translation] Delegate creation of ConfigCache instances to a factory. Apr 2, 2015

mpdude changed the title ~~[Config][Routing][Translation] Delegate creation of ConfigCache instances to a factory.~~ [Config] Delegate creation of ConfigCache instances to a factory. Apr 2, 2015

aitboudad reviewed Apr 2, 2015
View reviewed changes

mpdude mentioned this pull request Apr 3, 2015

[2.7][Translation] added message cache. #13986

Closed

2 tasks

aitboudad reviewed Apr 3, 2015
View reviewed changes

aitboudad added the Config label Apr 4, 2015

mpdude reviewed Apr 4, 2015
View reviewed changes

mpdude added 11 commits April 6, 2015 20:18

Explicitly add symfony/config dependency in an desperate attempt to m…

f2c3a24

…ake tests pass. See for example https://travis-ci.org/symfony/symfony/jobs/57160322 - wrong (too low) versions of symfony/config are installed.

Suggestions from GH

35488bd

FabBot compliance

17a55ec

Bump composer dep

07ecf57

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.

As in aa82fb0

a89db7a

Update composer.json

7938349

Don't mess with composer.json where it is not necessary

cb9917f

The HttpKernel for sure depends on the Config component

2016537

That should resolve the other dependency problems as well.

Cleanup composer.json

beb0a92

Make sure at least symfony/config 2.7 is installed when loaded as a t…

afee2bb

…ransitive dependency

symfony/config dev-master cannot be used unless changes from this PR …

4832e1a

…have been merged into it

fabpot reviewed Apr 7, 2015
View reviewed changes

cordoval reviewed Apr 7, 2015
View reviewed changes

Grammar fix in comment

7f90bae

fabpot closed this Apr 8, 2015

aitboudad mentioned this pull request Apr 10, 2015

[Config][cache factory] check type of callback argument. #14303

Merged

mpdude deleted the 7781-on-2.7 branch May 3, 2015 19:37

aitboudad mentioned this pull request May 11, 2015

[HttpKernel] use ConfigCache::getPath() method when it exists #14610

Merged

xabbuh mentioned this pull request May 14, 2015

[HttpKernel] use ConfigCacheFactory to create ConfigCache instances #14636

Closed

wouterj mentioned this pull request Apr 11, 2022

Allow Translator to use CacheInterface instead of ConfigCacheInterface #45991

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mpdude commented Apr 2, 2015

fabpot Apr 2, 2015

mpdude commented Apr 2, 2015

aitboudad Apr 2, 2015

aitboudad Apr 3, 2015

wouterj Apr 5, 2015

mpdude Apr 5, 2015

aitboudad commented Apr 3, 2015

mpdude Apr 4, 2015

fabpot Apr 7, 2015

mpdude Apr 7, 2015

mpdude Apr 7, 2015

fabpot Apr 7, 2015

fabpot commented Apr 7, 2015

fabpot commented Apr 7, 2015

aitboudad commented Apr 7, 2015

mpdude commented Apr 7, 2015

cordoval Apr 7, 2015

mpdude Apr 7, 2015

fabpot commented Apr 8, 2015

mpdude commented Apr 8, 2015

fabpot commented Apr 8, 2015

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

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

Conversation

mpdude commented Apr 2, 2015

Todo

Choose a reason for hiding this comment

mpdude commented Apr 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aitboudad commented Apr 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Apr 7, 2015

fabpot commented Apr 7, 2015

aitboudad commented Apr 7, 2015

mpdude commented Apr 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Apr 8, 2015

mpdude commented Apr 8, 2015

fabpot commented Apr 8, 2015