-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[2.8][HttpKernel] Added support for registering Bundle dependencies #15011
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
07f86ea
to
52d7cba
Compare
52d7cba
to
815bc1f
Compare
815bc1f
to
b4013ae
Compare
Hi, I still love this concept and feature, and want it to be implemented 👍 I'd just rename |
could make sense @Pierstoval but it is plural in the sense that the bundle contains dependencies, and not just one dependency. But what did you have in mind here? |
👍 |
1 similar comment
👍 |
$dependency = $rootBundles[$dependencyFQN]; | ||
} else { | ||
if (!class_exists($dependencyFQN)) { | ||
throw new DependencyMismatchException("Could not find '{$dependencyFQN}'", array_keys($stack)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we were to support optional dependencies than this would be the place to add the relevant logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this behavior, as explained in the class, is encouraged by using the class_exists
check manually in the bundle dependencies registration 😕
i am not sure. in an ideal world a bundle just should have dependencies to an interface + service name. |
Why would a bundle have dependencies to services? |
@timglabisch you are referring to bundles like FlySystemBundle and the like, that only depends on "libs", and in such an example I agree they should. But what about the systems on top combining these bundles into an software application (distribution)? A common problem for any distribution like eZ, Sylius, CMF, Sulu, Oro, (....), Symfony Standard (before they decided to not support upgrades anymore, a luxury the rest of us don't have), is that any changes to root project (the root repository and not the vendors), which is needed for composer.json, kernel, config, code ... Makes it hard to deal with upgrades, as it will often conflict with changes the project has done for customizations. This is just one of several features needed to Symfony (config and extensibility) and Composer (composer.json extends/inheritance) to make life easier for software distributions, and their users. But other bundle creators also seems eager to have such a feature, so the use case also extend to bundle writers extending or otherwise using functionality from other bundles, see #13945 and related PRs/issues. @lsmith77 Does this sum it up nicely from your perspective? Anything to add? Side note: Now it should be noted that things like Puli from @webmozart might solve all of these problems, however it is very early in it's life and might not be ready for powering these parts of Symfony3 yet, so this is potentially a temporary solution. |
FYI, I will take some time early next week to review this one carefully. Thanks for the work so far. |
@fabpot 👍 👍 👍 👍 |
|
||
public function registeredDependencies() | ||
{ | ||
return parent::registeredDependencies(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any need to redefine the method just to call the parent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, because the parent one is protected
so redefining it here as public
allows to get dependencies outside the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't figured that. Thank you @Pierstoval.
b4013ae
to
dabe9fe
Compare
@fabpot In the meantime I was pitched https://github.com/matthiasnoback/symfony-bundle-plugins by @lolautruche. It makes it clear how optional dependencies should be handled, as in instead of hardcoding all possible bundles that are optional in the root Bundle (the DemoBundle:: alwaysRegisteredPlugins() in the example of the symfony-bundle-plugins doc), all user to specify as new "plugins" might be created independently of the one they extend. /cc @lsmith77 |
So, what's holding this PR up? I think it is a really great feature for 2.8/3.0 (especially for 3rd party bundle maintainers, like the Sonata project and the CMF), so can we please wrap this PR up? |
@wouterj I was considering it done but I would like to adjust the handling for optional dependencies to match how symfony-bundle-plugins is doing it, or remove it for now. TL;DR; lets the optional /bundles/plugins extend the root instead of root bundle having to specify all possible optional bundles/plugins. If you/someone have time I would like to discuss possible approaches /cc @lsmith77 |
Actually, is this really important to "natively" support optional bundles? Can't you let the users use a simple |
That is how it is done now, in this PR currently (dabe9fe), but this implies root having to have knowledge of which bundles extends it. |
59aae70
to
5ef4446
Compare
} | ||
throw new DependencyMismatchException(sprintf('Could not find "%s"', $dependencyFQN), array_keys($stack)); | ||
} | ||
$dependency = new $dependencyFQN(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some bundles might have constructor parameters. For example: SonataUserBundle('FOSUserBundle')
Great feature! |
👍 |
Wil we see this feature one day in 3.x branch? 😄 |
Was that question for me or core? Cause in both cases I think neither knows how this should be done, so only way this moves forward is if someone that does or figures that out, contributes. Maybe someone is at least up for starting a spec process in an issue? |
Mostly my message was for the team because we need feedback from them to know whether the feature is good and well implemented. |
While @fabpot was reviewing this PR, @mmoreram created a package symfony-bundle-dependencies with similar functionality. |
@aminin I think there should be officiall support for this. Bundle dependencies sound like a big reason why bundles itself exist. |
@JHGitty Actually, as the bundle's concept is for them to be standalone, adding bundle dependencies is not really a good reason about bundle's concept, it's more about bundle's flexibility and extensibility, but anyway I still think this feature is good and should be implemented :) |
I often thought about a "plugin management" based on the bundle system of Symfony 😄 |
While some PRs are being reviewed during months, people and projects have to evolve and grow, and we cannot wait for this approval (If it's approved...). The project that @aminin talks about is being used in several projects and is working properly. If you really need this feature, feel free to use it. |
I'm not convinced we should add bundle dependencies to symfony. I never needed bundle dependencies in all these years. In theory a bundle should just be responsible to integrate a library into symfony (with config etc). So normally the dependency should only be between libraries. And for that we have composer. Bundle inheritance is rather uncommon and does not justify automated bundle dependencies IMO. One question: I've seen its possible to specify optional dependencies. But what does it do? |
I mean dependency management is really complex thing. And I would not add another layer besides composer. And if it is mainly about loading the bundles in the kernel, then I'd rather suggest to create a solution using Puli. Also since it seems to be possible to provide the current solution outside of the core (https://github.com/mmoreram/symfony-bundle-dependencies), I'm 👎 to add it in symfony directly. |
When I created the first prototype of Symfony bundles, I explicitly leaved dep management out as I wanted something more generic and that became Composer. So, I think I agree with @Tobion. Closing it now. |
As far as I can see, bundle dependencies are something completely different than the things Composer is doing. Composer is about loading packages, while bundle dependencies are about improving the user experience by automatically registering packages. There is not much complex stuff needed to add bundle class instances to the bundles array (=bundle deps), while resolving exact versions to install (=composer deps) are complex. Only putting infrastructure stuff in bundles (aka the Naked Bundle) is a real great thing. But (as for all OO concepts), it depends on the case whether it is needed or not. A lot of libraries don't need this layer of abstraction. For instance, I'm sure that the company I work for will not switch frameworks or database backends. It doesn't make sense to include this abstraction, it only makes ones live harder. Besides this, many popular community bundles don't follow this naked bundle concept. For example, during installation, SonataAdminBundle requires you to register 5 different bundles and even EasyAdminBundle requires 3 bundles to be registered. If you have open source bundles that need to be very generic (e.g. support different db backends), you can either include everything in one bundle (making versioning very hard) or split everything into multiple "adapter" bundles (making the installation procedure very verbose). With the introduction of bundle dependencies, each backend can have its own bundle. This helps versioning (and thus Composer) and doesn't harm the installation procedure. |
well not having it in core means that app authors will need to put it else where in order to share the code and ensure that apps (like the CMF and sylius) can be combined. this is possible but not ideal imho. not putting it into core could be a signal that maybe we need a new namespace for app devs |
@wouterj even for advanced bundles such as SonataAdmin, the dependencies part is trivial. Just copy+paste these lines in 1 file: new Sonata\CoreBundle\SonataCoreBundle(),
new Sonata\BlockBundle\SonataBlockBundle(),
new Knp\Bundle\MenuBundle\KnpMenuBundle(),
new Sonata\DoctrineORMAdminBundle\SonataDoctrineORMAdminBundle(),
new Sonata\AdminBundle\SonataAdminBundle(), And then copy-paste these lines in another file: sonata_block:
default_contexts: [cms]
blocks:
sonata.admin.block.admin_list:
contexts: [admin] Everything can be completed in few seconds and it's something that is done once in the application lifetime. |
Same comment here, bundle inheritance has several use cases that are not covered in the argumentations for closing this. As @lsmith77 covers, handling symfony dependencies for applications is one of the most prominent that both @magnusnordlander and @Tobion overlooks in the argumentation on lib vs bundle. Which is understandable since they only work with projects and reusable single purpose bundles themselves, applications however is a different beast. The examples for applications within symfony are many, and sonata is just one of them, a simple one. @javiereguiluz / all, if you look to Sylius, CMF, ORO or others you'll find a pattern that you can also see in eZ Platform: 33 bundles, several of which tend to change from one release to the next, making installation on a symfony standard install & especially upgrading (because of conflicts) for users harder then it needs to be. We can easily fix this ourselves by having a kernel our users must extend at all times, and also force users to use our distribution. But the bundle inheritance discussion is at it's root about fixing this within symfony itself, to allow interpolarity between symfony applications, and to move towards allowing applications to not have to force own distribution on users. To allow any symfony application installation to always just be two changes away: composer require and a single bundle in kernel. Apps is just one example that bundle dependencies exists and are very real, so pita to use the wrong arguments to dismiss the challenges they represent. |
@andrerom While you're entirely correct that I haven't considered the application case (and I would still like some time to think about that :) ), I would like to add one thing that I did consider (but did not write about) which complicates the issue of bundle dependencies significantly in my mind. Configuration. A lot of (dare I say most?) bundles and I can only assume the same for applications based on bundles, require configuration. If I depend on SecurityBundle and that's not already enabled, just adding the bundle to the kernel is not going to do my users much good. The user still needs to configure the bundle. A bundle they are now not really aware of even having. Without a solution for that, installing a Symfony application into your project is probably not just two changes away. |
You are absolutely correct, the bundles themselves would have to have safe defaults, and we would most likely need adopt Incenteev/ParameterHandler to allow it to let bundles provide dist files for required user settings, or find a solution in core for this. |
@magnusnordlander the solution to that is the |
I don't think that's a good idea. It suggests that Bundle A would configure Bundle B. However, Bundle B should provide a safe default config instead. |
The configuration issue is a big one, and I do tend to agree with the Symfony team that that places it outside of the scope of bundles to automatically instantiate other bundles. It's a kind of magic that makes programmers not see anymore what's happening at all, and that's usually bad magic. I think there is a middle road however to satisfy both camps. Instead of automatically configuring bundles not explicitly referenced the core could also throw an error if a required dependency is not loaded, resolved recursively. So in the case of something like Sonata, with minor modifications it could just declare its 5 dependencies, and those their own 5, and the compiler could just err out with a list of which of the 30 bundles is not instantiated. I think that fixes the main usecase. It's not Composer's job to check that package dependencies are actually used or instantiated. It would be possible with a plugin on post-install to list installed bundles which were not instantiated, but that would again require some magic assumptions about which kernel is going to be loaded from where at runtime. Symfony itself can do it a lot safer during compilation. |
@wouterj That seems fragile with regards to conflicts. Imagine installing both ez Platform and Sylius, with both of them attempting to configure the security bundle. It's very likely that the bundle user will need to manually configure the bundle him/herself in these situations. |
The point here is that bundles provide default configuration, not global one. It would need to specify bundles' priorities or to take account of the bundle's instanciation order. |
@Pierstoval My current line of thinking is that such a mechanism probably is useful in fewer situations than one would think, and would serve to confuse the user in more situation than it's worth (e.g. Default bundle config from the docs no longer applies). Don't take me wrong, I'm not necessarily against bundle dependencies, I just think that we need to have good and well thought out reasons to go above and beyond a "declare your dependencies and we'll throw an exception if they're not fulfilled" scheme. |
Hi, so this feature will be implement ? |
As there are ongoing discussions in both in eZ and Symfony CMF on this topic (and in Sylius), agreed with @lsmith77 to open PR to discuss this potential light weight native approach. Alternative if native feature is not wanted is OroPlatform, a custom, still somewhat project specific and heavier approach involving bundles.yml files and caching needs.
What?
Adds possibility for a Bundle to specify other bundles it depends on. This allows root/meta projects to not have to specify dependencies of dependencies, and no need for user to perform manual upgrade work in Kernel file when they change.
This could potentially allow "distribution bundles" for larger solutions like Sylius, CMF and eZ to be more easily installed in same Symfony installation, assuming there is no package or other conflicts.
A dependency implies two things for the Kernel:
The last point is because we assume a dependency is something we might extend so we always load it before "self".
How
A optional interface is added that bundles can implement, while the method could have been added
to the main interface like getParent(), this approach allows the code to check using instanceof,
keeps full BC, and allows a clear contract on use of this feature.
The approach further explicitly avoids giving bundle writers any control over priority of load ordering other
then "before self", as otherwise it would 1. potential lead to conflicts, and 2. complicate the implementation.
For further info on the new function and why it returns FQN class names, see BundleDependenciesInterface.
Todo
For earlier discussions see #13945 (was targeted at 2.7)