-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[HttpKernel] made the Kernel class more flexible (closes #7396) #7437
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
fabpot
commented
Mar 20, 2013
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #7396 |
| License | MIT |
| Doc PR | n/a |
|
I still (personally) think that the compile() method is still too close to the building of the container. Is there a reasonable way to move the compile() method away from the rest of the logic used to build the container? |
|
I like this one. Can't wait to see this merged. |
|
@jrobeson compiling the container is part of building it (it is the step running the compiler passes) |
|
i read the code stof .. that is what it does :) . I just thought it'd be easier to modify the container from the kernel of the compile() call was placed after the buildContainer() call at https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Kernel.php#L556 This is something i had to work around when migrating to symfony. It's not a big deal to me anymore, so no worries. |
|
@jrobeson the container is not fully build until you compile it. Half of the work is done by the compile() call. |
|
I've moved the compile method as it makes things even more flexible. |
This PR was merged into the master branch. Commits ------- 7c3179a [HttpKernel] moved the Container compilation for more flexibility 757194c [HttpKernel] made the Kernel class more flexible (closes #7396) Discussion ---------- [HttpKernel] made the Kernel class more flexible (closes #7396) | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #7396 | License | MIT | Doc PR | n/a --------------------------------------------------------------------------- by jrobeson at 2013-03-20T14:04:53Z I still (personally) think that the compile() method is still too close to the building of the container. Is there a reasonable way to move the compile() method away from the rest of the logic used to build the container? --------------------------------------------------------------------------- by elnur at 2013-03-20T14:34:23Z I like this one. Can't wait to see this merged. --------------------------------------------------------------------------- by stof at 2013-03-20T14:50:22Z @jrobeson compiling the container is part of building it (it is the step running the compiler passes) --------------------------------------------------------------------------- by jrobeson at 2013-03-20T15:32:26Z i read the code stof .. that is what it does :) . I just thought it'd be easier to modify the container from the kernel of the compile() call was placed after the buildContainer() call at https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Kernel.php#L556 This is something i had to work around when migrating to symfony. It's not a big deal to me anymore, so no worries. --------------------------------------------------------------------------- by stof at 2013-03-20T15:36:12Z @jrobeson the container is not fully build until you compile it. Half of the work is done by the compile() call. --------------------------------------------------------------------------- by fabpot at 2013-03-20T15:38:24Z I've moved the compile method as it makes things even more flexible. --------------------------------------------------------------------------- by jrobeson at 2013-03-20T15:41:45Z @stof : i know .. which is even better reason to move it outside that big method. @fabpot: thanks!
|
@fabpot, thanks! BTW, any reason we can't have this change in any stable branches? |
|
That's because we only fix bugs in stable branches. |
|
Got it. Now I just need to figure out how to upgrade to |