Skip to content

[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

Closed
wants to merge 1 commit into from

Conversation

andrerom
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #13505
License MIT
Doc PR symfony/symfony-docs#5610

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:

  • make sure it is loaded, unless dependency is marked as optional
  • order it before "current" bundle

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

  • CS fixes
  • Testing, incl recursion testing and fixes in implementation for this
  • Update Doc when discussion with core team have been made on selecting approach for dealing with optional dependencies (the approach suggested here is to handle it in doc, see BundleDependenciesInterface)

For earlier discussions see #13945 (was targeted at 2.7)

@andrerom andrerom force-pushed the bundle_dependencies branch from 07f86ea to 52d7cba Compare June 16, 2015 20:54
@andrerom andrerom force-pushed the bundle_dependencies branch from 52d7cba to 815bc1f Compare June 16, 2015 21:14
@andrerom andrerom changed the title [2.8][HttpKernel] Added support for registering Bundle dependenciesBundle dependencies [2.8][HttpKernel] Added support for registering Bundle dependencies Jun 16, 2015
@andrerom andrerom force-pushed the bundle_dependencies branch from 815bc1f to b4013ae Compare June 16, 2015 21:57
@Pierstoval
Copy link
Contributor

Hi, I still love this concept and feature, and want it to be implemented 👍

I'd just rename BundleDependenciesInterface into BundleDependencyInterface, because it's bundle-specific (implements BundleDependencyInterface sounds better to me).

@andrerom
Copy link
Contributor Author

I'd just rename BundleDependenciesInterface into BundleDependencyInterface

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?

@aramalipoor
Copy link

👍

1 similar comment
@htollefsen
Copy link

👍

$dependency = $rootBundles[$dependencyFQN];
} else {
if (!class_exists($dependencyFQN)) {
throw new DependencyMismatchException("Could not find '{$dependencyFQN}'", array_keys($stack));
Copy link
Contributor

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

Copy link
Contributor

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 😕

@timglabisch
Copy link

i am not sure. in an ideal world a bundle just should have dependencies to an interface + service name.

@Pierstoval
Copy link
Contributor

Why would a bundle have dependencies to services?

@andrerom
Copy link
Contributor Author

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

@fabpot
Copy link
Member

fabpot commented Jun 19, 2015

FYI, I will take some time early next week to review this one carefully. Thanks for the work so far.

@Pierstoval
Copy link
Contributor

@fabpot 👍 👍 👍 👍


public function registeredDependencies()
{
return parent::registeredDependencies();
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

@andrerom
Copy link
Contributor Author

andrerom commented Jul 9, 2015

@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

@wouterj
Copy link
Member

wouterj commented Jul 29, 2015

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?

@andrerom
Copy link
Contributor Author

@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

@Pierstoval
Copy link
Contributor

Actually, is this really important to "natively" support optional bundles? Can't you let the users use a simple class_exists check to use the bundle if the bundle class exists?

@andrerom
Copy link
Contributor Author

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.

@andrerom andrerom force-pushed the bundle_dependencies branch 2 times, most recently from 59aae70 to 5ef4446 Compare August 9, 2015 18:19
andrerom added a commit to andrerom/symfony-docs that referenced this pull request Aug 9, 2015
}
throw new DependencyMismatchException(sprintf('Could not find "%s"', $dependencyFQN), array_keys($stack));
}
$dependency = new $dependencyFQN();
Copy link
Contributor

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')

@veewee
Copy link
Contributor

veewee commented Nov 6, 2015

Great feature!

@ahmadrabie
Copy link

👍

@Pierstoval
Copy link
Contributor

Wil we see this feature one day in 3.x branch? 😄

@andrerom
Copy link
Contributor Author

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?

@Pierstoval
Copy link
Contributor

Mostly my message was for the team because we need feedback from them to know whether the feature is good and well implemented.

@aminin
Copy link

aminin commented Dec 25, 2015

While @fabpot was reviewing this PR, @mmoreram created a package symfony-bundle-dependencies with similar functionality.

@ghost
Copy link

ghost commented Dec 25, 2015

@aminin I think there should be officiall support for this. Bundle dependencies sound like a big reason why bundles itself exist.

@Pierstoval
Copy link
Contributor

@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 :)

@ghost
Copy link

ghost commented Dec 25, 2015

I often thought about a "plugin management" based on the bundle system of Symfony 😄
Dependencies of bundles would be nice for that.

@mmoreram
Copy link
Contributor

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.

@Tobion
Copy link
Contributor

Tobion commented Feb 14, 2016

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.
Those arguments have also been raised in https://blog.fervo.se/blog/2016/02/07/bundle-deps/ and I agree with it.

One question: I've seen its possible to specify optional dependencies. But what does it do?
Let's say you need bundle X and only for feature Y bundle X has an optional depencency on Z. I mean Z is not added to the Kernel as it's optional. So you have to add it yourself anyway I guess. So what does it bring to specify optional dependencies?

@Tobion
Copy link
Contributor

Tobion commented Feb 14, 2016

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.

@fabpot
Copy link
Member

fabpot commented Feb 14, 2016

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.

@fabpot fabpot closed this Feb 14, 2016
@wouterj
Copy link
Member

wouterj commented Feb 14, 2016

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.

@lsmith77
Copy link
Contributor

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

@javiereguiluz
Copy link
Member

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

@andrerom andrerom deleted the bundle_dependencies branch February 15, 2016 07:52
@andrerom
Copy link
Contributor Author

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.

@magnusnordlander
Copy link
Contributor

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

@andrerom
Copy link
Contributor Author

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.

@wouterj
Copy link
Member

wouterj commented Feb 15, 2016

@magnusnordlander the solution to that is the PrependedExtensionInterface, which allows configuring third party extensions from inside bundles.

@aschempp
Copy link
Contributor

@magnusnordlander the solution to that is the PrependedExtensionInterface, which allows configuring third party extensions from inside bundles.

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.

@curry684
Copy link
Contributor

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.

@magnusnordlander
Copy link
Contributor

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

@Pierstoval
Copy link
Contributor

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

@magnusnordlander
Copy link
Contributor

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

@cedvan
Copy link

cedvan commented Oct 27, 2016

Hi, so this feature will be implement ?

@mmoreram
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.