[2.7][HttpKernel] Added support for registering Bundle dependencies #13945

andrerom · 2015-03-16T21:43:01Z

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#5103

As there are ongoing discussions in both in eZ and Symfony CMF on this topic, 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.

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
Doc

Possible followups

Could potentially add support for using Ns\MyBundle::CLASS also in registerBundles
Consider caching the resulting bundle order if needed by additional features
Open request from Symfony-CMF is to be be able to specify optional dependencies, basically dependencies that should be silently ignored if they don't exists, however as this is during boot that would lead to un cached file system calls (PHP does not cache stat calls to non existing files) unless result is cached as mentioned in prior point
There is some code duplication between ordering logic in parent handling and this, could be removed if getParent over time changes to return FQN as well
Caching and string representation in registerBundles() can potentially open the door for lazy loading bundles

lsmith77 · 2015-03-17T08:04:42Z

One thing to note, with the ability to somewhat control load order it can also mean that people will have less needs to write compiler passes. imho would bring down the barrier for these kinds of things, but I also kind of like that inter-Bundle service configuration is clearly laid out in compiler passes.

As for the need of silent ignoring of optional dependencies, indeed this will add overhead but I would imagine that in many cases it will not really be an issue.

This leads me to an alternative/complementary idea: Simply a command that outputs the code to paste into the Kernel Bundle register method.

andrerom · 2015-03-17T10:11:33Z

This leads me to an alternative/complementary idea: Simply a command that outputs the code to paste into the Kernel Bundle register method.

could somehow work, but that would essentially be like a cache the user needs to manually maintain then. And won't be able to execute the command if bundle dependencies goes away after an composer update :/

lolautruche · 2015-03-17T10:46:43Z

src/Symfony/Component/HttpKernel/Kernel.php

+                $dependency = $topMostBundlesMap[$dependencyFQN];
+            } else {
+                if (!class_exists($dependencyFQN)) {
+                    throw new \RuntimeException(sprintf("Could not find Bundle '%s', set as dependency by '%s'", $dependencyFQN, $parent));


I'd go for a specific exception here, for clarity sake.

yes, where looking for better examples, open for suggestions.

DependencyMismatchException extends \RuntimeException? Just to throw something in

Thanks, but something like DependencyNotFoundException would be even more specific right? or did you have something in mind with the name?

Just thought, that maybe you can reuse it for other dependency issues as well. On the other hand it's just a suggestion 😄

What if the bundle is optional dependency?

That is currently not supported, see comments above. But if it was, it should indeed not throw ;)

Found ClassNotFoundException in Debug, seems to be the most fitting here.
@kingcrunch What do you think?

Or not, it has hard requirement on previous exception.
I'll go with DependencyMismatchException extends \RuntimeException 😄

mvrhov · 2015-03-18T15:00:32Z

Hm.. On one side we are removing magic (twig automatic file handling) on the other we are adding it. Is it really that hard to write one file into the AppKernel. After all you also have to provide configuration for that bundle

andrerom · 2015-03-18T16:04:11Z

Good point, but you would only need that if defaults are not ok, right? (side note: didn't fully get what twig has to do with this btw)

Also in this scenario where root1 depends on dependency2, it might be that root1 makes sure to add own events/tags that are used by dependency2. Root1 might even adjust the defaults of dependency2 to better fit the combined use for these two bundles. Meaning defaults can often be ok.

With the goal of simplifying distribution handling and upgrade needs, do you see other features needed around this? For instance on configuration?

mmoreram · 2015-03-20T12:06:36Z

Thanks GOD for this PR...

Doc for : symfony/symfony#13945

andrerom · 2015-03-23T00:14:59Z

Tests and further improvements done, doc in review, so ready for second round of review now.

hhamon · 2015-03-24T23:35:23Z

src/Symfony/Component/HttpKernel/Exception/DependencyMismatchException.php

+
+namespace Symfony\Component\HttpKernel\Exception;
+
+use RuntimeException;


Should be removed in favor of an inline new \RuntimeException() in the code.

Hi, you mean removing DependencyMismatchException?
That was the original case before this review thread:
#13945 (diff)

Or did you imply inline extends \RuntimeException (now the case as of 525bb12)

lsmith77 · 2015-03-25T07:28:56Z

it might be a good idea to also offer some debugging tool to help developers understand the dependency tree. though this is imho rather a nice to have that could come via a separate PR.

lsmith77 · 2015-03-25T07:32:05Z

src/Symfony/Component/HttpKernel/Tests/Fixtures/BundleDependencies/BundleCDependenciesBA.php

+    public function getBundleDependencies()
+    {
+        return array(
+            'Symfony\Component\HttpKernel\Tests\Fixtures\BundleDependencies\BundleBDependenciesA',


if we are to ever add optional support imho it would be better to have an associative array here, where the value would be true for required and false for optional for example. then when doing the class_exists check you would simply not throw an exception for optional dependencies.

Ok, thats a fair point, as we should decide on the format now and not change it later. I was thinking separate method, maybe even on a separate BundleOptionalDependenciesInterface if this would be done in a followup.

What about

class AcmeBundle extends Bundle implements BundleDependenciesInterface { public function getBundleDependencies() { return array( self::REQUIRED => array( // All values must be FQN strings to avoid bundles being loaded several times 'FOS\HttpCacheBundle\FOSHttpCacheBundle', // if on php 5.5 and higher Oneup\FlysystemBundle\OneupFlysystemBundle::CLASS, ), self::OPTIONAL => array( // Optional dependencies can not use ::CLASS constant 'FOS\HttpCacheBundle\FOSHttpCacheBundle', ); } }

This could be added now, and it could also be added later actually with bc for current format.

sure .. but what would be the advantage? or are you just proposing a possible new format with migration path in case we leave it out today and want to add optional deps later?

just seems like a more complicated structure. plus an assoc array implicitly takes care of duplicates. then again if one ends up having so many dependencies that accidental duplicates are a risk, one might be over using the feature :)

or are you just proposing a possible new format with migration path in case we leave it out today and want to add optional deps later?

y

But we can also go for 'FOS\HttpCacheBundle\FOSHttpCacheBundle' => self::REQUIRED, but then we would need to do that now imho.

returning to the specifics:

just seems like a more complicated structure. plus an assoc array implicitly takes care of duplicates. then again if one ends up having so many dependencies that accidental duplicates are a risk, one might be over using the feature :)

Was also thinking less typing, implementation can not do a simple array join anyway with your proposed structure, as you always would want REQUIRED to overwrite OPTIONAL. However I'll try out your structure to see here now.

andrerom · 2015-03-25T08:05:14Z

it might be a good idea to also offer some debugging tool to help developers understand the dependency tree.

So like $this->bundleMap for parent/child relationships then, could potentially have information about both in the same structure.

Doc for : symfony/symfony#13945

andrerom · 2015-03-25T18:31:42Z

So quick poll to everyone interested in this feature:

Q	A
Add optional dependencies now?	yes/no

Context: Would be with no (bundle order) caching for now, so it will be doing a uncached class_exists() -> file_exists() lookup for missing dependencies. Unlike existing files, php does not cache the filesystem (stat/..) calls for these.

…ager (nicolas-grekas) This PR was merged into the 2.8 branch. Discussion ---------- [Security] Add setVoters() on AccessDecisionManager | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | partially symfony#12360 | License | MIT | Doc PR | - Alternative for symfony#14550 Commits ------- 3fd7cea [Security] Add setVoters() on AccessDecisionManager

Issue symfony#11045 For now, the Routing DirectoryLoader requires the type `directory` to be specified so it does not conflict with `AnnotationDirectoryLoader`. However, this could be refactored.

* 2.7: (95 commits) [DependencyInjection] provide better error message when using deprecated configuration options [console][TableCell] get cell width without decoration. Improve the config validation in TwigBundle [VarDumper] Changed tooltip to expand-all keybinding in OS X [Bridge\PhpUnit] Fix composer installed phpunit detection [VarDumper] Fix generic casters calling order [2.7][SecurityBundle] Remove SecurityContext from Compile [WebProfilerBundle][logger] added missing deprecation message. Fix profiler CSS [Security][Acl] enforce string identifiers [FrameworkBundle] make `templating.helper.router` service available again for BC reasons [BrowserKit] Fix bug when uri starts with http. bumped Symfony version to 2.7.1 updated VERSION for 2.7.0 updated CHANGELOG for 2.7.0 bumped Symfony version to 2.6.10 updated VERSION for 2.6.9 updated CHANGELOG for 2.6.9 fixed tests bumped Symfony version to 2.3.31 ... Conflicts: src/Symfony/Bundle/FrameworkBundle/Command/TranslationDebugCommand.php src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/logger.html.twig src/Symfony/Component/HttpKernel/Kernel.php src/Symfony/Component/Translation/Loader/JsonFileLoader.php

…ive directory loading (lavoiesl, nicolas-grekas) This PR was merged into the 2.8 branch. Discussion ---------- [DependencyInjection] [Routing] [Config] Recursive directory loading | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#13246, symfony#11045, symfony#11059 | License | MIT | Doc PR | - Commits ------- 60b1c5b Added CHANGELOG entries, cleanups 73f0ee2 [DI][Routing] recursive directory loading

* 2.7: [Security] Update tests after a merge [Console] Remove an unused argument and fix a small cs issue [Translator] avoid serialize unserializable resources.

…re updated (chbruyand) This PR was squashed before being merged into the 2.8 branch (closes symfony#14781). Discussion ---------- [TwigBundle] Reconfigure twig paths when they are updated | Q | A | ------------- | --- | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#14771, symfony#14768, symfony#14262, symfony#14778 | License | MIT Refresh twig paths upon creation and deletion. As we don't care neither about path's modification time nor path's content, a new Resource has been added in the Config Component. Full discussion in symfony#14778. Commits ------- 3cbff05 [TwigBundle] Reconfigure twig paths when they are updated

This PR was merged into the 2.8 branch. Discussion ---------- added missing " Commits ------- 1baf05a added missing "

This PR was squashed before being merged into the 2.8 branch (closes symfony#14904). Discussion ---------- [toolbar] Merged colored icons in toolbar | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Status icons are doubled without any apparent reason. Token before ![capture_token_before](https://cloud.githubusercontent.com/assets/8344487/8023946/203c6fc8-0d22-11e5-9d8f-2760bfdf0c18.PNG) Token after ![capture_token_after](https://cloud.githubusercontent.com/assets/8344487/8024004/d0f4e9b6-0d23-11e5-9263-dd2a38b14d12.PNG) Security before ![capture_security_before](https://cloud.githubusercontent.com/assets/8344487/8023949/2441d6ee-0d22-11e5-9fed-168d850633d2.PNG) Security after ![capture_security_after](https://cloud.githubusercontent.com/assets/8344487/8023950/2444a1da-0d22-11e5-9c8a-1deb654e7ac4.PNG) Commits ------- 5b67bbd [toolbar] Merged colored icons in toolbar

…from the toolbar (MJBGO) This PR was merged into the 2.8 branch. Discussion ---------- [profiler][request toolbar] Removed route name from the toolbar | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#14744 | License | MIT | Doc PR | The action block takes nearly 1/3 of the bar width which is not good. The controller + action names are useful but the route name less important. Before ![capture_before](https://cloud.githubusercontent.com/assets/8344487/8023606/325acb00-0d13-11e5-86db-ab2a08242227.PNG) After ![capture_after](https://cloud.githubusercontent.com/assets/8344487/8023580/908edc22-0d11-11e5-92bd-37b380cc3433.PNG) The route name is shown by hovering the action block ![capture_tooltip](https://cloud.githubusercontent.com/assets/8344487/8023584/a5ae08f8-0d11-11e5-97a5-ffec6d3e41cc.PNG) Commits ------- 9004e29 Removed route name from the debugbar.

* 2.7: [Console] SymfonyStyle : Fix blocks wordwrapping [Console] SymfonyStyle : Fix blocks output is broken on windows cmd [Validator] remove partial deprecation annotation Updated UPGRADE-2.4.md [Form] Support DateTimeImmutable in transform() Show the FormType and FormTypeExtension in case of deprecated use of setDefaultOptions [FrameworkBundle] Document form.csrf_provider service deprecation [Form] add test to avoid regression of symfony#14891 without this change allways the legacy code get called [Form] Fix call to removed method (BC broken in 2.3) Fix ask and askHidden methods [HttpFoundation] Get response content as resource several times for PHP >= 5.6 Change error message to reflect SecurityContext deprecation. fixed merge Issue symfony#14815 [Console] SymfonyStyle : fix & automate block gaps. [Console] SymfonyStyle : Improve EOL consistency by relying on output instance Improved duplicated code in FileLocator

…er on a Response (jakzal) This PR was merged into the 2.8 branch. Discussion ---------- [HttpFoundation] Postpone setting the date header on a Response | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#14906 | License | MIT | Doc PR | - The only risk of doing this is if someone called `getDate()` and the date header was not present, the date might be slightly different than the one sent with the headers. `getDate()` could also set the date header first time it's requested (do a lazy initialisation). Commits ------- 2ad3b0d [HttpFoundation] Postpone setting the date header on a Response

* 2.7: Fix test name fixed CS Allow new lines in Messages translated with transchoice() (replacement for symfony#14867) [Form] Swap new ChoiceView constructor arguments to ease migrating from the deprecated one [2.3] Fix tests on Windows [Yaml] remove partial deprecation annotation Silence invasive deprecation warnings, opt-in for warnings Documenting how to keep option value BC - see symfony#14377 Conflicts: src/Symfony/Bridge/Doctrine/composer.json src/Symfony/Bridge/Twig/composer.json

…imler) This PR was merged into the 2.8 branch. Discussion ---------- fix Merge branch '2.7' into 2.8 JsonFileLoader This fix a merge commit see symfony@5593bdd. The `stream_is_local` and `file_exists` checks are all ready done in the parent `FileLoader` class. | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | none | License | MIT | Doc PR | none Commits ------- 692deff fix Merge branch '2.7' into 2.8 JsonFileLoader

This PR was merged into the 2.8 branch. Discussion ---------- [Profiler][Translation] added filter. | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Fixed tickets | ~ | Tests pass? | yes | License | MIT ![selection_017](https://cloud.githubusercontent.com/assets/1753742/8159914/c2c8ad68-135a-11e5-9ae3-f2e055ea3230.jpg) Commits ------- 65f9291 [Profiler][Translation] added filter.

For further info on the new function and why it returns class names see BundleDependenciesInterface.

…cies

andrerom · 2015-06-16T20:55:40Z

Closed and reopened against 2.8 as #15011 (now also with env and debug arguments)

andrerom force-pushed the bundle_dependencies branch 2 times, most recently from cc118cb to 99663bf Compare March 17, 2015 00:27

lsmith77 mentioned this pull request Mar 17, 2015

Separate Bundle for Sonata Admin classes symfony-cmf/symfony-cmf#216

Closed

lolautruche reviewed Mar 17, 2015
View reviewed changes

andrerom force-pushed the bundle_dependencies branch from 99663bf to 14c4fa5 Compare March 21, 2015 17:08

aitboudad added the HttpKernel label Mar 21, 2015

andrerom force-pushed the bundle_dependencies branch from 14c4fa5 to 242a30e Compare March 22, 2015 22:45

andrerom changed the title ~~[WIP][HttpKernel] Added support for registering Bundle dependencies~~ [HttpKernel] Added support for registering Bundle dependencies Mar 22, 2015

andrerom force-pushed the bundle_dependencies branch 2 times, most recently from c371505 to d647bad Compare March 22, 2015 22:53

andrerom added a commit to andrerom/symfony-docs that referenced this pull request Mar 22, 2015

[HttpKernel] Added support for registering Bundle dependencies

77f6d3c

Doc for : symfony/symfony#13945

andrerom mentioned this pull request Mar 22, 2015

[HttpKernel] Added support for registering Bundle dependencies symfony/symfony-docs#5103

Closed

1 task

andrerom force-pushed the bundle_dependencies branch from d647bad to 01e7d8d Compare March 22, 2015 23:30

andrerom changed the title ~~[HttpKernel] Added support for registering Bundle dependencies~~ [2.7][HttpKernel] Added support for registering Bundle dependencies Mar 22, 2015

hhamon reviewed Mar 24, 2015
View reviewed changes

andrerom force-pushed the bundle_dependencies branch 2 times, most recently from c001a31 to 525bb12 Compare March 25, 2015 00:48

lsmith77 reviewed Mar 25, 2015
View reviewed changes

andrerom added a commit to andrerom/symfony-docs that referenced this pull request Mar 25, 2015

[HttpKernel] Added support for registering Bundle dependencies

4532ec8

Doc for : symfony/symfony#13945

fabpot and others added 26 commits June 1, 2015 17:31

[DI][Routing] recursive directory loading

73f0ee2

Issue symfony#11045 For now, the Routing DirectoryLoader requires the type `directory` to be specified so it does not conflict with `AnnotationDirectoryLoader`. However, this could be refactored.

Added CHANGELOG entries, cleanups

60b1c5b

Merge branch '2.7' into 2.8

99ec717

* 2.7: [Security] Update tests after a merge [Console] Remove an unused argument and fix a small cs issue [Translator] avoid serialize unserializable resources.

[TwigBundle] Reconfigure twig paths when they are updated

3cbff05

Removed route name from the debugbar.

9004e29

added missing "

1baf05a

minor symfony#14909 added missing " (OskarStark)

90f771c

This PR was merged into the 2.8 branch. Discussion ---------- added missing " Commits ------- 1baf05a added missing "

[toolbar] Merged colored icons in toolbar

5b67bbd

[HttpFoundation] Postpone setting the date header on a Response

2ad3b0d

fix Merge branch '2.7' into 2.8 JsonFileLoader

692deff

[Profiler][Translation] added filter.

65f9291

[HttpKernel] Added support for registering Bundle dependencies

952be5e

For further info on the new function and why it returns class names see BundleDependenciesInterface.

[HttpKernel] Added support for registering *optional* Bundle dependen…

c7a7509

…cies

[HttpKernel] Simplify Bundle dependencies by moving *optional* to doc

f5b3578

[HttpKernel] Added env & debug to Bundle dependencies method

07f86ea

andrerom force-pushed the bundle_dependencies branch from ae35ae0 to 07f86ea Compare June 16, 2015 20:46

andrerom closed this Jun 16, 2015

andrerom mentioned this pull request Jun 16, 2015

[2.8][HttpKernel] Added support for registering Bundle dependencies #15011

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2.7][HttpKernel] Added support for registering Bundle dependencies #13945

[2.7][HttpKernel] Added support for registering Bundle dependencies #13945

andrerom commented Mar 16, 2015

lsmith77 commented Mar 17, 2015

andrerom commented Mar 17, 2015

lolautruche Mar 17, 2015

andrerom Mar 17, 2015

kingcrunch Mar 17, 2015

andrerom Mar 17, 2015

kingcrunch Mar 17, 2015

hason Mar 17, 2015

andrerom Mar 17, 2015

andrerom Mar 21, 2015

andrerom Mar 21, 2015

mvrhov commented Mar 18, 2015

andrerom commented Mar 18, 2015

mmoreram commented Mar 20, 2015

andrerom commented Mar 23, 2015

hhamon Mar 24, 2015

andrerom Mar 25, 2015

hhamon Mar 25, 2015

lsmith77 commented Mar 25, 2015

lsmith77 Mar 25, 2015

andrerom Mar 25, 2015

andrerom Mar 25, 2015

lsmith77 Mar 25, 2015

andrerom Mar 25, 2015

andrerom Mar 27, 2015

andrerom commented Mar 25, 2015

andrerom commented Mar 25, 2015

andrerom commented Jun 16, 2015


		namespace Symfony\Component\HttpKernel\Exception;

		use RuntimeException;

[2.7][HttpKernel] Added support for registering Bundle dependencies #13945

[2.7][HttpKernel] Added support for registering Bundle dependencies #13945

Conversation

andrerom commented Mar 16, 2015

What?

How

Todo

Possible followups

lsmith77 commented Mar 17, 2015

andrerom commented Mar 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvrhov commented Mar 18, 2015

andrerom commented Mar 18, 2015

mmoreram commented Mar 20, 2015

andrerom commented Mar 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsmith77 commented Mar 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom commented Mar 25, 2015

andrerom commented Mar 25, 2015

andrerom commented Jun 16, 2015