-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
b57891f
to
8d5f992
Compare
IIRC, it was consciously not changed in Kernel. |
* | ||
* @api | ||
*/ | ||
public function __construct($environment, $debug) | ||
public function __construct($environment, $debug, ConfigCacheFactoryInterface $configCacheFactory = null) |
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.
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).
@fabpot Do you remember for what reason? |
8d5f992
to
f67f261
Compare
|
I found this in the PR description of #14178:
I don't think that this is true. For example, one might want to customise this steps by providing a custom @mpdude What do you think? |
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? |
I would err on the side of not "fixing" this in the Kernel... except if someone does have a real world use case. |
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. |
I'm in favor of closing this. There is no point in providing an extension point if we don't have a use case. |
@fabpot what do you think about using |
Honestly I don't see any use case even for |
@aitboudad See #14178 and the related PRs. The The thing is that once your data has been pulled from the DB and is put into |
@mpdude thanks for the clarification :) Anyway I think I missed something so I prefer to stay until this part is documented ^^ |
Yes, I know, writing the docs is on my list. A custom Can we close this PR for the time being? |
Closing now. |
ConfigCacheFactory
to createConfigCache
instances. TheRouter
and theTranslator
class had been updated. However, theKernel
class still created theConfigCache
on its own.