[HttpKernel] Add DI extension configs as ressources when possible #6148
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
btw @fabpot what about having a base class for |
|
@vicb your suggestion makes sense. Can you also explain the goal of this PR? |
|
The goal of this PR is to avoid having to sfcc when you modify a DI extension configuration. I think @rdohms got trapped. |
|
I thought about it several times but never took time to implement it. It is annoying to have to clear the cache when you modify a default value in the Configuration class. So +1 |
fabpot
added a commit
that referenced
this pull request
Dec 6, 2012
This PR was merged into the master branch. Commits ------- 7f16c1f [HttpKernel] Add DI extension configs as ressources when possible Discussion ---------- [HttpKernel] Add DI extension configs as ressources when possible /cc @rdohms @richardmiller --------------------------------------------------------------------------- by vicb at 2012-11-30T11:57:48Z btw @fabpot what about having a base class for `Extension` in the DI ? Would make it easier to re-use it when using standalone components, Di and (the suggested) Config as the greatest part of the class is not HttpKernel specific. --------------------------------------------------------------------------- by fabpot at 2012-12-06T08:47:28Z @vicb your suggestion makes sense. Can you also explain the goal of this PR? --------------------------------------------------------------------------- by vicb at 2012-12-06T09:01:58Z The goal of this PR is to avoid having to sfcc when you modify a DI extension configuration. I think @rdohms got trapped. --------------------------------------------------------------------------- by vicb at 2012-12-06T09:08:08Z see https://twitter.com/rdohms/status/274059267428978688 --------------------------------------------------------------------------- by stof at 2012-12-06T09:20:54Z I thought about it several times but never took time to implement it. It is annoying to have to clear the cache when you modify a default value in the Configuration class. So +1
fabpot
added a commit
that referenced
this pull request
Dec 6, 2012
This PR was squashed before being merged into the master branch (closes #6207). Commits ------- 57e9d28 [DI] Add a base class for extension Discussion ---------- [DI] Add a base class for extension depends on #6148 @fabpot should we change `addClassesToCompile` & the likes (thinking of traits). --------------------------------------------------------------------------- by fabpot at 2012-12-06T13:05:05Z Can you rebase? --------------------------------------------------------------------------- by fabpot at 2012-12-06T13:06:43Z hmmm, now that I see the result, I'm not sure it is worth it as the Extension class in the DI component depends on the Config one. --------------------------------------------------------------------------- by vicb at 2012-12-06T13:23:29Z No pb, I can remove it, should I remove the `ContainerBuilder` altogether ? --------------------------------------------------------------------------- by fabpot at 2012-12-06T13:37:18Z I would keep everything that is strictly in the DI namespace in the DI extension class and everything else in the HttpKernel class as it is now. --------------------------------------------------------------------------- by vicb at 2012-12-06T13:38:59Z But this change is **great** if you need the DI without the full stack. What about my other comment ? --------------------------------------------------------------------------- by fabpot at 2012-12-06T13:55:30Z Which other comment? This one? "should I remove the ContainerBuilder altogether?" In which case, I don't understand what it means. What about adding 2 classes in the DI component: the base one and another one with the dependency on the config component? Is it overkill? --------------------------------------------------------------------------- by vicb at 2012-12-06T14:06:43Z > "should I remove the ContainerBuilder altogether?" I mean that the **widely used** (ie loaders) `ContainerBuilder` also depends on Config - that was kind of a joke ! I was refering to my first comment here > should we change addClassesToCompile & the likes (thinking of traits). Overkill I don't know but useless for sure: the `ExtensionInterface` depends on `ContainerBuilder` which depends on `Config`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
/cc @rdohms @richardmiller