Skip to content

Commit

Permalink
Renamed Bundle classes to be named like the bundle itself for more cl…
Browse files Browse the repository at this point in the history 
…arity
  • Loading branch information
@fabpot
Bernhard Schussek authored and fabpot committed Jun 23, 2010
1 parent 4a74213 commit c486e1b
Show file tree
Hide file tree
Showing 12 changed files with 27 additions and 25 deletions.
4 changes: 2 additions & 2 deletions ...mfony/Framework/DoctrineBundle/Bundle.php → ...amework/DoctrineBundle/DoctrineBundle.php
View file
Expand Up @@ -2,7 +2,7 @@

namespace Symfony\Framework\DoctrineBundle;

use Symfony\Foundation\Bundle\Bundle as BaseBundle;
use Symfony\Foundation\Bundle\Bundle;
use Symfony\Components\DependencyInjection\ContainerInterface;
use Symfony\Components\DependencyInjection\Loader\Loader;
use Symfony\Components\DependencyInjection\Loader\XmlFileLoader;
Expand All @@ -25,7 +25,7 @@
* @author Fabien Potencier <fabien.potencier@symfony-project.com>
* @author Jonathan H. Wage <jonwage@gmail.com>
*/
class Bundle extends BaseBundle
class DoctrineBundle extends Bundle
{
/**
* Customizes the Container instance.
Expand Down
4 changes: 2 additions & 2 deletions ...ework/DoctrineMigrationsBundle/Bundle.php → ...ationsBundle/DoctrineMigrationsBundle.php
View file
Expand Up @@ -2,7 +2,7 @@

namespace Symfony\Framework\DoctrineMigrationsBundle;

use Symfony\Foundation\Bundle\Bundle as BaseBundle;
use Symfony\Foundation\Bundle\Bundle;

/*
* This file is part of the Symfony framework.
Expand All @@ -21,6 +21,6 @@
* @author Fabien Potencier <fabien.potencier@symfony-project.com>
* @author Jonathan H. Wage <jonwage@gmail.com>
*/
class Bundle extends BaseBundle
class DoctrineMigrationsBundle extends Bundle
{
}
4 changes: 2 additions & 2 deletions ...Symfony/Framework/PropelBundle/Bundle.php → ...y/Framework/PropelBundle/PropelBundle.php
View file
Expand Up @@ -2,13 +2,13 @@

namespace Symfony\Framework\PropelBundle;

use Symfony\Foundation\Bundle\Bundle as BaseBundle;
use Symfony\Foundation\Bundle\Bundle;
use Symfony\Components\DependencyInjection\ContainerInterface;
use Symfony\Components\DependencyInjection\Loader\Loader;
use Symfony\Components\DependencyInjection\Loader\XmlFileLoader;
use Symfony\Framework\PropelBundle\DependencyInjection\PropelExtension;

class Bundle extends BaseBundle
class PropelBundle extends Bundle
{
public function buildContainer(ContainerInterface $container)
{
Expand Down
4 changes: 2 additions & 2 deletions ...ny/Framework/SwiftmailerBundle/Bundle.php → ...k/SwiftmailerBundle/SwiftmailerBundle.php
View file
Expand Up @@ -2,7 +2,7 @@

namespace Symfony\Framework\SwiftmailerBundle;

use Symfony\Foundation\Bundle\Bundle as BaseBundle;
use Symfony\Foundation\Bundle\Bundle;
use Symfony\Components\DependencyInjection\ContainerInterface;
use Symfony\Components\DependencyInjection\Loader\Loader;
use Symfony\Framework\SwiftmailerBundle\DependencyInjection\SwiftmailerExtension;
Expand All @@ -23,7 +23,7 @@
* @subpackage Framework_SwiftmailerBundle
* @author Fabien Potencier <fabien.potencier@symfony-project.com>
*/
class Bundle extends BaseBundle
class SwiftmailerBundle extends Bundle
{
/**
* Customizes the Container instance.
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Framework/TwigBundle/Bundle.php → ...mfony/Framework/TwigBundle/TwigBundle.php
View file
Expand Up @@ -2,7 +2,7 @@

namespace Symfony\Framework\TwigBundle;

use Symfony\Foundation\Bundle\Bundle as BaseBundle;
use Symfony\Foundation\Bundle\Bundle;
use Symfony\Components\DependencyInjection\ContainerInterface;
use Symfony\Components\DependencyInjection\Loader\Loader;
use Symfony\Components\DependencyInjection\Loader\XmlFileLoader;
Expand All @@ -25,7 +25,7 @@
* @subpackage Framework_TwigBundle
* @author Fabien Potencier <fabien.potencier@symfony-project.com>
*/
class Bundle extends BaseBundle
class TwigBundle extends Bundle
{
/**
* Customizes the Container instance.
Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Framework/WebBundle/Command/InitBundleCommand.php
View file
Expand Up @@ -77,5 +77,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
'namespace' => $namespace,
'bundle' => $bundle,
));

rename($targetDir.'/Bundle.php', $targetDir.'/'.$bundle.'.php');
}
}
2 changes: 1 addition & 1 deletion src/Symfony/Framework/WebBundle/Controller/ControllerManager.php
View file
Expand Up @@ -112,7 +112,7 @@ public function findController($controller)
$logs[] = sprintf('Failed finding controller "%s:%s" from namespace "%s" (%s)', $bundle, $controller, $namespace, $try);
}
} else {
if (!in_array($namespace.'\\'.$bundle.'\\Bundle', array_map(function ($bundle) { return get_class($bundle); }, $this->container->getKernelService()->getBundles()))) {
if (!in_array($namespace.'\\'.$bundle.'\\'.$bundle, array_map(function ($bundle) { return get_class($bundle); }, $this->container->getKernelService()->getBundles()))) {
throw new \LogicException(sprintf('To use the "%s" controller, you first need to enable the Bundle "%s" in your Kernel class.', $try, $namespace.'\\'.$bundle));
}

Expand Down
8 changes: 4 additions & 4 deletions src/Symfony/Framework/WebBundle/Resources/skeleton/application/xml/Kernel.php
View file
Expand Up @@ -17,12 +17,12 @@ public function registerBundles()
{
$bundles = array(
new Symfony\Foundation\Bundle\KernelBundle(),
new Symfony\Framework\WebBundle\Bundle(),
new Symfony\Framework\WebBundle\WebBundle(),

// enable third-party bundles
new Symfony\Framework\ZendBundle\Bundle(),
new Symfony\Framework\DoctrineBundle\Bundle(),
new Symfony\Framework\SwiftmailerBundle\Bundle(),
new Symfony\Framework\ZendBundle\ZendBundle(),
new Symfony\Framework\DoctrineBundle\DoctrineBundle(),
new Symfony\Framework\SwiftmailerBundle\SwiftmailerBundle(),

// register your bundles here
);
Expand Down
8 changes: 4 additions & 4 deletions src/Symfony/Framework/WebBundle/Resources/skeleton/application/yaml/Kernel.php
View file
Expand Up @@ -17,12 +17,12 @@ public function registerBundles()
{
$bundles = array(
new Symfony\Foundation\Bundle\KernelBundle(),
new Symfony\Framework\WebBundle\Bundle(),
new Symfony\Framework\WebBundle\WebBundle(),

// enable third-party bundles
new Symfony\Framework\ZendBundle\Bundle(),
new Symfony\Framework\DoctrineBundle\Bundle(),
new Symfony\Framework\SwiftmailerBundle\Bundle(),
new Symfony\Framework\ZendBundle\ZendBundle(),
new Symfony\Framework\DoctrineBundle\DoctrineBundle(),
new Symfony\Framework\SwiftmailerBundle\SwiftmailerBundle(),

// register your bundles here
);
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Framework/WebBundle/Resources/skeleton/bundle/Bundle.php
View file
Expand Up @@ -2,9 +2,9 @@

namespace {{ namespace }}\{{ bundle }};

use Symfony\Foundation\Bundle\Bundle as BaseBundle;
use Symfony\Foundation\Bundle\Bundle;
use Symfony\Components\DependencyInjection\ContainerInterface;

class Bundle extends BaseBundle
class {{ bundle }} extends Bundle
{
}
4 changes: 2 additions & 2 deletions src/Symfony/Framework/WebBundle/Bundle.php → ...Symfony/Framework/WebBundle/WebBundle.php
View file
Expand Up @@ -2,7 +2,7 @@

namespace Symfony\Framework\WebBundle;

use Symfony\Foundation\Bundle\Bundle as BaseBundle;
use Symfony\Foundation\Bundle\Bundle;
use Symfony\Components\DependencyInjection\ContainerInterface;
use Symfony\Components\DependencyInjection\Loader\Loader;
use Symfony\Components\DependencyInjection\Loader\XmlFileLoader;
Expand All @@ -25,7 +25,7 @@
* @subpackage Framework_WebBundle
* @author Fabien Potencier <fabien.potencier@symfony-project.com>
*/
class Bundle extends BaseBundle
class WebBundle extends Bundle
{
/**
* Customizes the Container instance.
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Framework/ZendBundle/Bundle.php → ...mfony/Framework/ZendBundle/ZendBundle.php
View file
Expand Up @@ -2,7 +2,7 @@

namespace Symfony\Framework\ZendBundle;

use Symfony\Foundation\Bundle\Bundle as BaseBundle;
use Symfony\Foundation\Bundle\Bundle;
use Symfony\Components\DependencyInjection\ContainerInterface;
use Symfony\Components\DependencyInjection\Reference;
use Symfony\Components\DependencyInjection\Loader\Loader;
Expand All @@ -24,7 +24,7 @@
* @subpackage Framework_ZendBundle
* @author Fabien Potencier <fabien.potencier@symfony-project.com>
*/
class Bundle extends BaseBundle
class ZendBundle extends Bundle
{
/**
* Customizes the Container instance.
Expand Down

17 comments on commit c486e1b

@henrikbjorn
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont get this commit, the bundle is already in a namespaced path called SomethingBundle why would you want to make it even more verbose ?

@FabienPennequin
Copy link

Choose a reason for hiding this comment

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

Completely agree with henrikbjorn. Namespace should be enough.

@marijn
Copy link

@marijn marijn commented on c486e1b Jun 23, 2010

Choose a reason for hiding this comment

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

I agree with henrikbjorn. Whenever the namespace is not enough, one can always do the following:

<?php

  use Symfony\Framework\ZendBundle\Bundle as ZendBundle;

@fabpot
Copy link
Member

@fabpot fabpot commented on c486e1b Jun 23, 2010

Choose a reason for hiding this comment

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

I was probably a bit too fast on this one. I have reverted the change for now to allow more discussion.

@jmikola
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the cases where one would actually need to use a Bundle class (per margijn's example)?

I imagine two cases where renaming the custom bundle classes could be beneficial:

  • Avoid having dozens of Bundle.php files within a project directory tree. This would be a boon for code editors that let you quick-open files (throughout your project) by their base name.
  • Avoid using namespace aliases (do they have a bad stigma?)

Assuming use as should be avoided, I wonder if this would be better or worse:

class Bundle extends \Symfony\Foundation\Bundle\Bundle

I can't forsee a reason why one would need to access anything withing the base Bundle class (which a use would have allowed for easily).

@henrikbjorn
Copy link
Contributor

Choose a reason for hiding this comment

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

@jmikola

  • The code should not contain naming or other conventions just because its easier for some ide.
  • Why should they be avoided? if they shouldnt be used we where back at Project_Something_Something_Else

@jmikola
Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't arguing one way or the other, just postulating some reasons why it might have been done :)

@CodingFabian
Copy link
Contributor

Choose a reason for hiding this comment

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

I did discuss this with Bernhard earlier and his commit is what I thought would be best.
The point is that when looking into code, or using an IDE, you use the Name of a class/object.
The name does normally not include the namespace. So you will have lots of duplicates.
Second, you need meaning!
Something called Bundle is meaningless. What bundle, what for? Only relying on the namespace to explain this doesn't help much.
Also consider putting two "whatever" inside the same namespace. Like two controller. You might come to a point where you want this.

So I am fully in favour of adding some meaning to the name of a class. Its easier to read, easier to understand and easier to look up.
However this does not mean to come back to the ugly namespace hacks done in the past.

@beberlei
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with CodingFabian and Bernhard on this issue. Although we have namespaces now this doesnt mean we can go back and give our classes meaningless producing clashes in naming.

Concrete Implementations should have concrete names, try to understand "Bundle extends AbstractBundle".

"Its in the namespace" is not a valid argument in my opinion, a class has meaning in the namespace it is in. Using a meaning class-name inside a package makes it harder to understand the code that uses this class.

@henrikbjorn
Copy link
Contributor

Choose a reason for hiding this comment

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

Then i dont really see the meaning of namespaces... Why dont we just go back to I_Want_To_Name_Stuff_Like_This ?

@webmozart
Copy link
Contributor

Choose a reason for hiding this comment

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

Namespaces are a way to make internal code easy to understand. If a class is only used within a component, there's no problem in giving it a short name that has enough meaning in the scope of this component.

Not for public API. If a class is designed to be used by other developers, give it a name that has enough meaning, independent of the namespace.

A good comparison might be: If it makes sense to talk about classes with the same name in a conversation, they should probably have different names.

"Add that code to the Bundle class"
"Which one?"

@webmozart
Copy link
Contributor

Choose a reason for hiding this comment

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

Another point is usability. Imagine having two different Bundle.php files open at the same time in your editor (don't forget that applications will potentially have dozens of custom Bundles!). It's very easy to edit the wrong Bundle.php if you are not extremely careful.

@avalanche123
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with beberlei and others for the above stated reasons.
Symfony\Components\Templating\Renderer\Renderer
Symfony\Components\Templating\Storage\Storage
Symfony\Components\Validator\Validator
Symfony\Components\Process\Process
Symfony\Components\HttpKernel\Profiler\Profiler
etc...
are perfectly valid class names and I don't have problem with duplication in their paths.
Bundle class is WebBundle, it therefore lives under WebBundle namespace along with a lot of other WebBundle related stuff

@CraigMason
Copy link

Choose a reason for hiding this comment

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

"The point is that when looking into code, or using an IDE, you use the Name of a class/object."

I've been using the following, which works perfectly:

/**
 * @param Package\Subpackage\Class $param
 * @returns Package\Subpackage\Something\Else
 */
public function foo(Package\Subpackage\Class $param) { .. }

The rules I generally use are,

  1. If you're referencing a class within the same namespace, just use the Class Name
  2. If you're referencing anything else, fully qualify it \With\The\Name\Space
  3. If that's too long, or the context is closely related, create a non-ambiguous alias (use) using as few elements of the fully qualified namespace as possible

A class name does have meaning, but once you're outside of the context of said namespace, it should be fully qualified.

Also, is there a reason why this is used:

Symfony\Framework\DoctrineBundle\Bundle
Symfony\Framework\SwiftmailerBundle\Bundle

Rather than:

Symfony\Framework\Bundle\Doctrine\Bundle
Symfony\Framework\Bundle\Swiftmailer\Bundle

@henrikbjorn
Copy link
Contributor

Choose a reason for hiding this comment

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

I totally understand the Symfony\Framework\SomeBundle\Bundle naming which makes a lot of sense.

naming it Symfony\Framework\SomeBundle\SomeBundle just looks ugly and does not bring any value but makes us need to type even more characters everytime we are referencing a bundle in Doctrine ORM or Doctrine ODM. And just the bundle namespaces alone makes a line that i a thousand characters wide.

I say if your editors, ide or whatever you are using cant handle it well then maybe you should start looking after a new edtior, besides the top of every file will say what bundle you are editing becuse the namespace declaration will be there.

@bschussek if this "Add that code to the Bundle class" "Which one?" comes up i assume the question would have been to a specific bundle and so there wont be any confussion. :)

@jmikola
Copy link
Contributor

Choose a reason for hiding this comment

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

henrikbjorn: makes us need to type even more characters everytime we are referencing a bundle in Doctrine ORM or Doctrine ODM

Do you mean when referencing entity/document classes? The Bundle's initializing class, Symfony\Framework\SomeBundle\SomeBundle, should only need to show up if you're enabling the Bundle in your kernel. Other places, like routing.yml includes and class references, should be unaffected.

@henrikbjorn
Copy link
Contributor

Choose a reason for hiding this comment

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

i mean then WebExtension should be named FoundationBundleWebExtension because there can be more than one so its clear what bundles WebExtension someone is talking about.

Please sign in to comment.