Skip to content

[Kernel] [Bundle] Set up bundle dependencies #13506

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.

Sign up for GitHub

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
Closed

[Kernel] [Bundle] Set up bundle dependencies #13506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/Symfony/Component/HttpKernel/Bundle/Bundle.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,16 @@ public function shutdown()
{
}

/**
* Get the current bundle dependencies
*
* @return BundleInterface[]
*/
public function getBundleDependencies()
{
return array();
}

/**
* Builds the bundle.
*
Expand Down
9 changes: 9 additions & 0 deletions src/Symfony/Component/HttpKernel/Bundle/BundleInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@ public function boot();
*/
public function shutdown();

/**
* Get the current bundle dependencies
*
* @return BundleInterface[]
*
* @api
*/
public function getBundleDependencies();

/**
* Builds the bundle.
*
Expand Down
62 changes: 41 additions & 21 deletions src/Symfony/Component/HttpKernel/Kernel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ abstract class Kernel implements KernelInterface, TerminableInterface
protected $name;
protected $startTime;
protected $loadClassCache;
protected $topMostBundles;
protected $directChildrenBundles;

const VERSION = '3.0.0-DEV';
const VERSION_ID = '30000';
Expand Down Expand Up @@ -216,7 +218,7 @@ public function getBundle($name, $first = true)
}

/**
* {@inheritDoc}
* {@inheritdoc}
*
* @throws \RuntimeException if a custom resource is hidden by a resource in a derived bundle
*/
Expand Down Expand Up @@ -420,26 +422,18 @@ protected function initializeBundles()
{
// init bundles
$this->bundles = array();
$topMostBundles = array();
$directChildren = array();
$this->topMostBundles = array();
$this->directChildrenBundles = array();

// Register root bundles
foreach ($this->registerBundles() as $bundle) {
$name = $bundle->getName();
if (isset($this->bundles[$name])) {
throw new \LogicException(sprintf('Trying to register two bundles with the same name "%s"', $name));
}
$this->bundles[$name] = $bundle;
$this->addBundle($bundle);
}

if ($parentName = $bundle->getParent()) {
if (isset($directChildren[$parentName])) {
throw new \LogicException(sprintf('Bundle "%s" is directly extended by two bundles "%s" and "%s".', $parentName, $name, $directChildren[$parentName]));
}
if ($parentName == $name) {
throw new \LogicException(sprintf('Bundle "%s" can not extend itself.', $name));
}
$directChildren[$parentName] = $name;
} else {
$topMostBundles[$name] = $bundle;
// Register dependencies
foreach ($this->bundles as $bundle) {
foreach ($bundle->getBundleDependencies() as $bundleDependency) {
$this->addBundle($bundleDependency, true);
}
}

Expand All @@ -452,12 +446,12 @@ protected function initializeBundles()

// inheritance
$this->bundleMap = array();
foreach ($topMostBundles as $name => $bundle) {
foreach ($this->topMostBundles as $name => $bundle) {
$bundleMap = array($bundle);
$hierarchy = array($name);

while (isset($directChildren[$name])) {
$name = $directChildren[$name];
while (isset($this->directChildrenBundles[$name])) {
$name = $this->directChildrenBundles[$name];
array_unshift($bundleMap, $this->bundles[$name]);
$hierarchy[] = $name;
}
Expand All @@ -469,6 +463,32 @@ protected function initializeBundles()
}
}

/**
* @param BundleInterface $bundle
* @param bool $dependency
*/
protected function addBundle($bundle, $dependency = false)
{
$name = $bundle->getName();

if (isset($this->bundles[$name]) && false === $dependency) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@Pierstoval

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we skip including that bundle at all?! i.e. Return out of addBundle (with no exception)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

throw new \LogicException(sprintf('Trying to register two bundles with the same name "%s"', $name));
}
$this->bundles[$name] = $bundle;

if ($parentName = $bundle->getParent()) {
if (isset($this->directChildrenBundles[$parentName])) {
throw new \LogicException(sprintf('Bundle "%s" is directly extended by two bundles "%s" and "%s".', $parentName, $name, $this->directChildrenBundles[$parentName]));
}
if ($parentName == $name) {
throw new \LogicException(sprintf('Bundle "%s" can not extend itself.', $name));
}
$this->directChildrenBundles[$parentName] = $name;
} else {
$this->topMostBundles[$name] = $bundle;
}
}

/**
* Gets the container class.
*
Expand Down