merged branch jmikola/master-DependencyInjectionExceptions (PR #2786)

Commits
-------

da0773c Note DependencyInjection exception changes in the 2.1 changelog
9a090bc [DependencyInjection] Fix class check and failure message in PhpDumper test
2334596 [DependencyInjection] Use component's SPL classes in dumped service container
3c02ea2 [DependencyInjection] Use exception class for API doc generation
47256ea [DependencyInjection] Make exceptions consistent when ContainerBuilder is frozen
b7300d2 [DependencyInjection] Fix up @throws documentation
123f514 [DependencyInjection] Use component-specific SPL exceptions
cf2ca9b [DependencyInjection] Create additional SPL exceptions
ba8322e [DependencyInjection] Format base exception classes consistently

Discussion
----------

[DependencyInjection] Refactor usage of SPL exceptions

```
Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: -
```

This was something discussed on the mailing list recently and a topic at the last IRC meeting. The DependencyInjection component was what I had in mind while discussing this with @lsmith a few weeks ago, as I found that it already had some local SPL exceptions, which weren't being used directly (only as base classes for the compiler exceptions). This PR replaces uses of core SPL exceptions with the component's equivalents. I've listed it as BC-breaking to be safe, but I don't think it should cause any trouble, since the new exceptions are sub-classes of those originally used.

I purposely avoided changing the exceptions in the dumped PHP container. If we agree that's worth doing, it would be a trivial addition to the PR.

---------------------------------------------------------------------------

by jmikola at 2011/12/04 22:38:47 -0800

One question I came across while implementing this PR: the doc blocks in ContainerInterface reference InvalidArgumentException (actually the component's local SPL equivalent), but there is no practical need for that class to be used in the file. How will the API doc generator handle this? Do we need to change the doc block reference to the full class path?

---------------------------------------------------------------------------

by fabpot at 2011/12/05 01:20:31 -0800

Why have you not replaced the exception in the dumped container code? I don't see any drawback.

---------------------------------------------------------------------------

by stof at 2011/12/05 03:39:25 -0800

it would even be better to be consistent in the generated code

---------------------------------------------------------------------------

by jmikola at 2011/12/05 09:48:05 -0800

I'll update the exceptions in the dumped container code as well. Thanks for the feedback.

---------------------------------------------------------------------------

by jmikola at 2011/12/05 10:41:21 -0800

Ok, this should be ready to review and merge. Tests needed some updating, but they still pass.

Author: fabpot

CHANGELOG-2.1.md

@@ -106,6 +106,10 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c
 
  * added support for loading globally-installed PEAR packages
 
+### DependencyInjection
+
+ * component exceptions that inherit base SPL classes are now used exclusively (this includes dumped containers)
+
 ### DomCrawler
 
  * added a way to get parsing errors for Crawler::addHtmlContent() and Crawler::addXmlContent() via libxml functions

src/Symfony/Component/DependencyInjection/Compiler/CheckCircularReferencesPass.php

@@ -50,7 +50,7 @@ public function process(ContainerBuilder $container)
      * Checks for circular references.
      *
      * @param array $edges An array of Nodes
-     * @throws \RuntimeException When a circular reference is found.
+     * @throws ServiceCircularReferenceException When a circular reference is found.
      */
     private function checkOutEdges(array $edges)
     {

src/Symfony/Component/DependencyInjection/Compiler/CheckDefinitionValidityPass.php

@@ -13,6 +13,7 @@
 
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Exception\RuntimeException;
 
 /**
  * This pass validates each definition individually only taking the information
@@ -33,22 +34,22 @@ class CheckDefinitionValidityPass implements CompilerPassInterface
      * Processes the ContainerBuilder to validate the Definition.
      *
      * @param ContainerBuilder $container
-     * @throws \RuntimeException When the Definition is invalid
+     * @throws RuntimeException When the Definition is invalid
      */
     public function process(ContainerBuilder $container)
     {
         foreach ($container->getDefinitions() as $id => $definition) {
             // synthetic service is public
             if ($definition->isSynthetic() && !$definition->isPublic()) {
-                throw new \RuntimeException(sprintf(
+                throw new RuntimeException(sprintf(
                     'A synthetic service ("%s") must be public.',
                     $id
                 ));
             }
 
             // synthetic service has non-prototype scope
             if ($definition->isSynthetic() && ContainerInterface::SCOPE_PROTOTYPE === $definition->getScope()) {
-                throw new \RuntimeException(sprintf(
+                throw new RuntimeException(sprintf(
                     'A synthetic service ("%s") cannot be of scope "prototype".',
                     $id
                 ));
@@ -57,14 +58,14 @@ public function process(ContainerBuilder $container)
             // non-synthetic, non-abstract service has class
             if (!$definition->isAbstract() && !$definition->isSynthetic() && !$definition->getClass()) {
                 if ($definition->getFactoryClass() || $definition->getFactoryService()) {
-                    throw new \RuntimeException(sprintf(
+                    throw new RuntimeException(sprintf(
                         'Please add the class to service "%s" even if it is constructed by a factory '
                        .'since we might need to add method calls based on compile-time checks.',
                        $id
                     ));
                 }
 
-                throw new \RuntimeException(sprintf(
+                throw new RuntimeException(sprintf(
                     'The definition for "%s" has no class. If you intend to inject '
                    .'this service dynamically at runtime, please mark it as synthetic=true. '
                    .'If this is an abstract definition solely used by child definitions, '

src/Symfony/Component/DependencyInjection/Compiler/CheckReferenceValidityPass.php

@@ -11,12 +11,13 @@
 
 namespace Symfony\Component\DependencyInjection\Compiler;
 
-use Symfony\Component\DependencyInjection\Exception\ScopeWideningInjectionException;
-use Symfony\Component\DependencyInjection\Exception\ScopeCrossingInjectionException;
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Exception\RuntimeException;
+use Symfony\Component\DependencyInjection\Exception\ScopeCrossingInjectionException;
+use Symfony\Component\DependencyInjection\Exception\ScopeWideningInjectionException;
 
 /**
  * Checks the validity of references
@@ -85,7 +86,7 @@ public function process(ContainerBuilder $container)
      * Validates an array of References.
      *
      * @param array $arguments An array of Reference objects
-     * @throws \RuntimeException when there is a reference to an abstract definition.
+     * @throws RuntimeException when there is a reference to an abstract definition.
      */
     private function validateReferences(array $arguments)
     {
@@ -96,7 +97,7 @@ private function validateReferences(array $arguments)
                 $targetDefinition = $this->getDefinition((string) $argument);
 
                 if (null !== $targetDefinition && $targetDefinition->isAbstract()) {
-                    throw new \RuntimeException(sprintf(
+                    throw new RuntimeException(sprintf(
                         'The definition "%s" has a reference to an abstract definition "%s". '
                        .'Abstract definitions cannot be the target of references.',
                        $this->currentId,
@@ -114,7 +115,8 @@ private function validateReferences(array $arguments)
      *
      * @param Reference  $reference
      * @param Definition $definition
-     * @throws \RuntimeException when there is an issue with the Reference scope
+     * @throws ScopeWideningInjectionException when the definition references a service of a narrower scope
+     * @throws ScopeCrossingInjectionException when the definition references a service of another scope hierarchy
      */
     private function validateScope(Reference $reference, Definition $definition = null)
     {

src/Symfony/Component/DependencyInjection/Compiler/PassConfig.php

@@ -11,6 +11,8 @@
 
 namespace Symfony\Component\DependencyInjection\Compiler;
 
+use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
+
 /**
  * Compiler Pass Configuration
  *
@@ -95,15 +97,15 @@ public function getPasses()
      *
      * @param CompilerPassInterface $pass A Compiler pass
      * @param string $type The pass type
-     * @throws \InvalidArgumentException when a pass type doesn't exist
+     * @throws InvalidArgumentException when a pass type doesn't exist
      *
      * @api
      */
     public function addPass(CompilerPassInterface $pass, $type = self::TYPE_BEFORE_OPTIMIZATION)
     {
         $property = $type.'Passes';
         if (!isset($this->$property)) {
-            throw new \InvalidArgumentException(sprintf('Invalid type "%s".', $type));
+            throw new InvalidArgumentException(sprintf('Invalid type "%s".', $type));
         }
 
         $passes = &$this->$property;

src/Symfony/Component/DependencyInjection/Compiler/RepeatedPass.php

@@ -12,6 +12,7 @@
 namespace Symfony\Component\DependencyInjection\Compiler;
 
 use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
 
 /**
  * A pass that might be run repeatedly.
@@ -27,12 +28,13 @@ class RepeatedPass implements CompilerPassInterface
      * Constructor.
      *
      * @param array $passes An array of RepeatablePassInterface objects
+     * @throws InvalidArgumentException if a pass is not a RepeatablePassInterface instance
      */
     public function __construct(array $passes)
     {
         foreach ($passes as $pass) {
             if (!$pass instanceof RepeatablePassInterface) {
-                throw new \InvalidArgumentException('$passes must be an array of RepeatablePassInterface.');
+                throw new InvalidArgumentException('$passes must be an array of RepeatablePassInterface.');
             }
 
             $pass->setRepeatedPass($this);

src/Symfony/Component/DependencyInjection/Compiler/ResolveDefinitionTemplatesPass.php

@@ -14,6 +14,7 @@
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\DefinitionDecorator;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Exception\RuntimeException;
 
 /**
  * This replaces all DefinitionDecorator instances with their equivalent fully
@@ -60,7 +61,7 @@ public function process(ContainerBuilder $container)
     private function resolveDefinition($id, DefinitionDecorator $definition)
     {
         if (!$this->container->hasDefinition($parent = $definition->getParent())) {
-            throw new \RuntimeException(sprintf('The parent definition "%s" defined for definition "%s" does not exist.', $parent, $id));
+            throw new RuntimeException(sprintf('The parent definition "%s" defined for definition "%s" does not exist.', $parent, $id));
         }
 
         $parentDef = $this->container->getDefinition($parent);
@@ -116,7 +117,7 @@ private function resolveDefinition($id, DefinitionDecorator $definition)
             }
 
             if (0 !== strpos($k, 'index_')) {
-                throw new \RuntimeException(sprintf('Invalid argument key "%s" found.', $k));
+                throw new RuntimeException(sprintf('Invalid argument key "%s" found.', $k));
             }
 
             $index = (integer) substr($k, strlen('index_'));

src/Symfony/Component/DependencyInjection/Compiler/ResolveInvalidReferencesPass.php

@@ -14,6 +14,7 @@
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Exception\RuntimeException;
 
 /**
  * Emulates the invalid behavior if the reference is not found within the
@@ -46,7 +47,7 @@ public function process(ContainerBuilder $container)
             foreach ($definition->getMethodCalls() as $call) {
                 try {
                     $calls[] = array($call[0], $this->processArguments($call[1], true));
-                } catch (\RuntimeException $ignore) {
+                } catch (RuntimeException $ignore) {
                     // this call is simply removed
                 }
             }
@@ -57,7 +58,7 @@ public function process(ContainerBuilder $container)
                 try {
                     $value = $this->processArguments(array($value), true);
                     $properties[$name] = reset($value);
-                } catch (\RuntimeException $ignore) {
+                } catch (RuntimeException $ignore) {
                     // ignore property
                 }
             }
@@ -89,7 +90,7 @@ private function processArguments(array $arguments, $inMethodCall = false)
                     $arguments[$k] = null;
                 } else if (!$exists && ContainerInterface::IGNORE_ON_INVALID_REFERENCE === $invalidBehavior) {
                     if ($inMethodCall) {
-                        throw new \RuntimeException('Method shouldn\'t be called.');
+                        throw new RuntimeException('Method shouldn\'t be called.');
                     }
 
                     $arguments[$k] = null;

src/Symfony/Component/DependencyInjection/Compiler/ServiceReferenceGraph.php

@@ -11,6 +11,8 @@
 
 namespace Symfony\Component\DependencyInjection\Compiler;
 
+use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
+
 /**
  * This is a directed graph of your services.
  *
@@ -46,12 +48,12 @@ public function hasNode($id)
      *
      * @param string $id The id to retrieve
      * @return ServiceReferenceGraphNode The node matching the supplied identifier
-     * @throws \InvalidArgumentException
+     * @throws InvalidArgumentException if no node matches the supplied identifier
      */
     public function getNode($id)
     {
         if (!isset($this->nodes[$id])) {
-            throw new \InvalidArgumentException(sprintf('There is no node with id "%s".', $id));
+            throw new InvalidArgumentException(sprintf('There is no node with id "%s".', $id));
         }
 
         return $this->nodes[$id];

src/Symfony/Component/DependencyInjection/Container.php

@@ -11,6 +11,8 @@
 
 namespace Symfony\Component\DependencyInjection;
 
+use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
+use Symfony\Component\DependencyInjection\Exception\RuntimeException;
 use Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException;
 use Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException;
 use Symfony\Component\DependencyInjection\ParameterBag\ParameterBagInterface;
@@ -135,7 +137,7 @@ public function getParameterBag()
      *
      * @return mixed  The parameter value
      *
-     * @throws  \InvalidArgumentException if the parameter is not defined
+     * @throws InvalidArgumentException if the parameter is not defined
      *
      * @api
      */
@@ -183,14 +185,14 @@ public function setParameter($name, $value)
     public function set($id, $service, $scope = self::SCOPE_CONTAINER)
     {
         if (self::SCOPE_PROTOTYPE === $scope) {
-            throw new \InvalidArgumentException('You cannot set services of scope "prototype".');
+            throw new InvalidArgumentException('You cannot set services of scope "prototype".');
         }
 
         $id = strtolower($id);
 
         if (self::SCOPE_CONTAINER !== $scope) {
             if (!isset($this->scopedServices[$scope])) {
-                throw new \RuntimeException('You cannot set services of inactive scopes.');
+                throw new RuntimeException('You cannot set services of inactive scopes.');
             }
 
             $this->scopedServices[$scope][$id] = $service;
@@ -226,7 +228,7 @@ public function has($id)
      *
      * @return object The associated service
      *
-     * @throws \InvalidArgumentException if the service is not defined
+     * @throws InvalidArgumentException if the service is not defined
      *
      * @see Reference
      *
@@ -292,11 +294,11 @@ public function getServiceIds()
     public function enterScope($name)
     {
         if (!isset($this->scopes[$name])) {
-            throw new \InvalidArgumentException(sprintf('The scope "%s" does not exist.', $name));
+            throw new InvalidArgumentException(sprintf('The scope "%s" does not exist.', $name));
         }
 
         if (self::SCOPE_CONTAINER !== $this->scopes[$name] && !isset($this->scopedServices[$this->scopes[$name]])) {
-            throw new \RuntimeException(sprintf('The parent scope "%s" must be active when entering this scope.', $this->scopes[$name]));
+            throw new RuntimeException(sprintf('The parent scope "%s" must be active when entering this scope.', $this->scopes[$name]));
         }
 
         // check if a scope of this name is already active, if so we need to
@@ -330,14 +332,14 @@ public function enterScope($name)
      * scope.
      *
      * @param string $name The name of the scope to leave
-     * @throws \InvalidArgumentException if the scope is not active
+     * @throws InvalidArgumentException if the scope is not active
      *
      * @api
      */
     public function leaveScope($name)
     {
         if (!isset($this->scopedServices[$name])) {
-            throw new \InvalidArgumentException(sprintf('The scope "%s" is not active.', $name));
+            throw new InvalidArgumentException(sprintf('The scope "%s" is not active.', $name));
         }
 
         // remove all services of this scope, or any of its child scopes from
@@ -377,13 +379,13 @@ public function addScope(ScopeInterface $scope)
         $parentScope = $scope->getParentName();
 
         if (self::SCOPE_CONTAINER === $name || self::SCOPE_PROTOTYPE === $name) {
-            throw new \InvalidArgumentException(sprintf('The scope "%s" is reserved.', $name));
+            throw new InvalidArgumentException(sprintf('The scope "%s" is reserved.', $name));
         }
         if (isset($this->scopes[$name])) {
-            throw new \InvalidArgumentException(sprintf('A scope with name "%s" already exists.', $name));
+            throw new InvalidArgumentException(sprintf('A scope with name "%s" already exists.', $name));
         }
         if (self::SCOPE_CONTAINER !== $parentScope && !isset($this->scopes[$parentScope])) {
-            throw new \InvalidArgumentException(sprintf('The parent scope "%s" does not exist, or is invalid.', $parentScope));
+            throw new InvalidArgumentException(sprintf('The parent scope "%s" does not exist, or is invalid.', $parentScope));
         }
 
         $this->scopes[$name] = $parentScope;