Skip to content

[HttpKernel] use ConfigCacheFactory to create ConfigCache instances #14636

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 1 commit into from

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented May 14, 2015

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR
#14178 introduced a new ConfigCacheFactory to create ConfigCache instances. The Router and the Translator class had been updated. However, the Kernel class still created the ConfigCache on its own.

@xabbuh xabbuh force-pushed the config-cache-factory branch from b57891f to 8d5f992 Compare May 14, 2015 14:54
@fabpot
Copy link
Member

fabpot commented May 14, 2015

IIRC, it was consciously not changed in Kernel.

*
* @api
*/
public function __construct($environment, $debug)
public function __construct($environment, $debug, ConfigCacheFactoryInterface $configCacheFactory = null)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we will accept this PR as I don't see the use case but in any case, this should be moved to a method instead of a constructor argument. I would even create a protected method like the getCacheDir() method, so configuration happen in the AppKernel class instead of whenever you need to create a Kernel instance (think tests for instance).

@xabbuh
Copy link
Member Author

xabbuh commented May 14, 2015

@fabpot Do you remember for what reason?

@xabbuh xabbuh force-pushed the config-cache-factory branch from 8d5f992 to f67f261 Compare May 14, 2015 15:21
@fabpot
Copy link
Member

fabpot commented May 14, 2015

@xabbuh I'm not sure but @mpdude might remember.

@mpdude
Copy link
Contributor

mpdude commented May 14, 2015

ConfigCacheFactory can be used for service based cache validation (doc PR still pending). That does not make sense for the container if you need the contained services to validate it... Or would it?

@xabbuh
Copy link
Member Author

xabbuh commented May 14, 2015

I found this in the PR description of #14178:

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

I don't think that this is true. For example, one might want to customise this steps by providing a custom ConfigCacheFactoryInterface implementation in their AppKernel class.

@mpdude What do you think?

@mpdude
Copy link
Contributor

mpdude commented May 14, 2015

Well, technically it should work in a straightforward manner.

It's probably tedious to set up in the AppKernel when you don't have services or the DIC.

All real life use cases I've seen so far involved loading routes or translations from a database and I have no idea if that might make sense for the Kernel as well.

So, is this just a consistency thing or do you have a real use case where it would help? If so, do you want to change the way the container is cached or the way that cache is validated?

@fabpot
Copy link
Member

fabpot commented May 14, 2015

I would err on the side of not "fixing" this in the Kernel... except if someone does have a real world use case.

@xabbuh
Copy link
Member Author

xabbuh commented May 14, 2015

For now, this is more a consistency thing, which @aitboudad and I talked about, but I don't have a concrete use case in mind and AFAIK neither does he. So, of you think it's not wish to do this change just for the same if consistency, let's see if someone comes up with a real use case or close it otherwise.

@fabpot
Copy link
Member

fabpot commented May 14, 2015

I'm in favor of closing this. There is no point in providing an extension point if we don't have a use case.

@aitboudad
Copy link
Contributor

@fabpot what do you think about using ConfigCacheFactory here without providing an extension point, which is consistent and better than using ConfigCache directly ?

@aitboudad
Copy link
Contributor

Honestly I don't see any use case even for translations or routes, the current implementation is mainly related to locale files and it's not possible to loading from a database for example.

@mpdude
Copy link
Contributor

mpdude commented May 14, 2015

@aitboudad See #14178 and the related PRs.

The ConfigCacheFactory does not directly provide the loading from (for example) a database. You'd have to write your own loaded for that.

The thing is that once your data has been pulled from the DB and is put into ConfigCache, you have no way of re-validating that cache and to check if the cached content is still in sync with the database. This is because all the cache validation happens inside the ConfigCache class which was previously created inside the Translator (for example). No way of getting any other validation logic in there, especially one that would need access to a DB. Being able to inject the ConfigCacheFactory changed that.

@aitboudad
Copy link
Contributor

@mpdude thanks for the clarification :)
If I understand, the need here is to check the cache is still sync or not, so why not just using custom ResourceInterface.
If we talk about Translator (for example) the consistant way is to use a custom loader (loading from database) and when data is loaded we can add then add Resource to the Catalogue to check data freshness from DB.

Anyway I think I missed something so I prefer to stay until this part is documented ^^

@mpdude
Copy link
Contributor

mpdude commented May 15, 2015

Yes, I know, writing the docs is on my list. A custom ResourceInterface implementation does not work, because the Resource is simply unserialized from the cache and isFresh() is called. No way to get a database connection in there.

Can we close this PR for the time being?

@fabpot
Copy link
Member

fabpot commented May 16, 2015

Closing now.

@fabpot fabpot closed this May 16, 2015
@xabbuh xabbuh deleted the config-cache-factory branch May 16, 2015 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants