Skip to content
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

[2.2][HttpKernel] Extract the bundles building code #7396

Closed
wants to merge 1 commit into from

Conversation

elnur
Copy link
Contributor

@elnur elnur commented Mar 15, 2013

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes

At least I need that method extracted to be able to add my own code to it

@fabpot
Copy link
Member

fabpot commented Mar 18, 2013

Can you explain your use case a bit more?

@elnur
Copy link
Contributor Author

elnur commented Mar 18, 2013

I don't use bundles for app specific code and I need to add a security listener factory to the security extension. Usually this is done in the build method of the Bundle class, but since I don't have them, I need to hook the code somewhere else.

Doing this before the Kernel::buildContainer call is too early because the security extension hasn't been loaded yet, and after the call it's too late because the container has been compiled already. The buildContainer method is too monolithic and needs to be broken down. This PR is a modest step in that direction.

@fabpot
Copy link
Member

fabpot commented Mar 20, 2013

Why not using the registerContainerConfiguration() method then?

@fabpot
Copy link
Member

fabpot commented Mar 20, 2013

see #7437 for an alterative patch.

@elnur
Copy link
Contributor Author

elnur commented Mar 20, 2013

How would I get the currently used container builder in the registerContainerConfiguration() method? I need it to get to the security extension.

@fabpot
Copy link
Member

fabpot commented Mar 20, 2013

I figured that out after posting my comment. That's why I've made a PR which I think is better than this one. What do you think?

@elnur
Copy link
Contributor Author

elnur commented Mar 20, 2013

Looks good and would allow to resolve my use case.

@fabpot
Copy link
Member

fabpot commented Mar 20, 2013

Closing in favor of #7437

@fabpot fabpot closed this Mar 20, 2013
@elnur elnur deleted the build-bundles-extraction branch March 20, 2013 14:30
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!
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.

None yet

2 participants