[Kernel] [Bundle] Set up bundle dependencies #13506
Conversation
Pierstoval
commented
Jan 24, 2015
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | yes |
| Deprecations? | no |
| Tests pass? | yes (not modified) |
| Fixed tickets | #13505 |
| License | MIT |
| Doc PR | none |
|
This is BC break as you add new method to the interface. |
|
Didn't thought it was so much BC as the new method is only an implementation of what was inside the I updated the comment :) |
|
I see a big BC break opportunity here (creating a fragile case btw): if I register 2 bundles explicitly in my kernel and the first one decides to declare a dependency on the second one, I will get an error when reaching the explicit registration of the second bundle |
|
Do you think that it'd be a big issue if I removed the second argument of the |
|
I think the whole bundle list should be resolved first, this way you can filter out things and throw errors if desired. If X requires Z and Z is first in the list, it could be resolved to put Z behind X. |
|
It seems like the behavior in the last commit I pushed :) a184992 First you register the whole "root" bundle list, and after that, you register the whole "dependency" list. In fact, the dependencies are registered the same way than the root bundles, but I just don't throw any exception if you try to declare a dependency that is already registered. And after that, bundle initialization comes to its native behavior :) Normally, IIRC, the bundles order should have no impact, whatever dependency you are using. |
| { | ||
| $name = $bundle->getName(); | ||
|
|
||
| if (isset($this->bundles[$name]) && false === $dependency) { |
There was a problem hiding this comment.
Why not avoid the exception if bundles' FQCN are the same!?
if(isset($this->bundles[$name]) && get_class($this->bundles[$name]) !== get_class($bundle))Then you can remove $dependency and safely throw an exception only if there two different bundles but with the same name!
There was a problem hiding this comment.
Using two bundles with the same name does not only impact this feature, but what about templating ? When using AppBundle:Default:index.html.twig can be a real problem if you have these two FQCN:
\AppBundle
\VendorName\CustomerApplication\Bundle\AppBundle
These FQCN are entirely different, but the bundle name remains the same, so this is still a problem. I only set up this check the same way it was before. Just look at this diff :)
EDIT: That's exactly the reason why the SensioGeneratorBundle generate:bundle command proposes an automatic bundle name based on your FQCN, for example when you specify the namespace Acme\Demo it automatically proposes AcmeDemoBundle, to avoid using something like DemoBundle which might be too "common".
There was a problem hiding this comment.
You're right but my proposal does not allow two different bundles with the same name to be registered!
It something like this:
- If bundles have the same name and the same FQCN, just easily continue. (So if different bundles have the same exact dependency no exception is thrown)
- If bundles have the same name and the different FQCN, throw an exception.
- If bundles have the different name and the different FQCN, smile and continue. :)
There was a problem hiding this comment.
Yea but as I said above, the native behavior from Symfony does not allow using two bundles with the same name, even if they have different FQCN. It is useless to implement this feature in this PR, because it would need to refactor the bundle registration system in order to "change" a bundle name if another one exists with the same name but with another FQCN. And this is not the point of my implementation.
There was a problem hiding this comment.
What if we skip including that bundle at all?! i.e. Return out of addBundle (with no exception)?
There was a problem hiding this comment.
I don't understand what you want :/
You mean that if I have two bundles with the same name, the latter won't be included at all ? If it's this behavior you're talking about, it's wrong IMO. The "duplicated" bundles must be warned by the developer, as a new bundle is almost all the time added in dev environment, so the developer can surely view the exception message and fix its bug. Plus, the bundles order would have a significant role, and it is not an expected behavior at all.
There was a problem hiding this comment.
Hmmmm yeah you're right. I had some kind of plugin-based system in my mind in which we prefer less run-time errors.
But you're right, when we're talking about development of bundles and their dependencies it's wrong to fail silently, the developer must know about the warning.
|
Squashed commits, ok, this is no great update, but is there anyone interested in this PR ? If no one is, maybe we should close it then.. |
|
I'm very interested in Bundle Dependencies 👍 |
|
For those following this PR there is also a slightly different take on this now: #13945 Not really trying to compete or anything, both @Pierstoval and I just want this feature in, and I didn't know about this variant of it. We are far from alone btw, this is a clear need in Symfony CMF, eZ, Elcodi, Oro, .... |
|
Your PR is more advanced than mine, I'll just close this one and follow your work on the bundle dependencies :) Hope it's gonna be merged soon, at least for 3.0 👍 |