Skip to content

Conversation

@fabpot
Copy link
Member

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

@ghost
Copy link

ghost commented Mar 20, 2013

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?

@elnur
Copy link
Contributor

elnur commented Mar 20, 2013

I like this one. Can't wait to see this merged.

@stof
Copy link
Member

stof commented Mar 20, 2013

@jrobeson compiling the container is part of building it (it is the step running the compiler passes)

@ghost
Copy link

ghost commented Mar 20, 2013

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.

@stof
Copy link
Member

stof commented Mar 20, 2013

@jrobeson the container is not fully build until you compile it. Half of the work is done by the compile() call.

@fabpot
Copy link
Member Author

fabpot commented Mar 20, 2013

I've moved the compile method as it makes things even more flexible.

@ghost
Copy link

ghost commented Mar 20, 2013

@stof : i know .. which is even better reason to move it outside that big method.

@fabpot: thanks!

fabpot added a commit that referenced this pull request Mar 20, 2013
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 fabpot merged commit 7c3179a into symfony:master Mar 20, 2013
@elnur
Copy link
Contributor

elnur commented Mar 20, 2013

@fabpot, thanks! BTW, any reason we can't have this change in any stable branches?

@fabpot
Copy link
Member Author

fabpot commented Mar 20, 2013

That's because we only fix bugs in stable branches.

@elnur
Copy link
Contributor

elnur commented Mar 20, 2013

Got it. Now I just need to figure out how to upgrade to master.

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.

3 participants