This tool is currently proof-of-concept. Your feedback and evaluation is valuable in helping to improve it and ensure its reports are meaninful.
Please click here to complete a short survey to tell us what you think. It should take less than 5 minutes and help further this research project!
Symfony\Component\Routing\Loader\Configurator\CollectionConfigurator
Detected issues
Issue | Method | Line number |
---|---|---|
Using `new` in constructor | __construct | 35 |
Using `new` in constructor | __construct | 36 |
Code
Click highlighted lines for details
<?php
/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/
namespace Symfony\Component\Routing\Loader\Configurator;
use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;
/**
* @author Nicolas Grekas <p@tchwork.com>
*/
class CollectionConfigurator
{
use Traits\AddTrait;
use Traits\HostTrait;
use Traits\RouteTrait;
private $parent;
private $parentConfigurator;
private $parentPrefixes;
private $host;
public function __construct(RouteCollection $parent, string $name, self $parentConfigurator = null, array $parentPrefixes = null)
{
$this->parent = $parent;
$this->name = $name;
Usage of the new keyword in a constructor
Summary of issues
- Tight Coupling
- Hidden dependencies
- Broken Encapsulatioin
Background
If a dependency is constructed inside the object that requires it rather than passed in as a reference then flexibility is lost[1][2]
public class Car {
private Engine engine;
public Car() {
this.engine = new Engine();
}
}Here, the
Car
constructor creates theEngine
instance. This is inflexible as it forces allCar
objects to use the exact sameEngine
type. Instead, it would encourage reuse if the program supported different engine types (e.g.DieselEngine
,PetrolEngine
orHybridEngine
).The same is true when an instance variable is created when the class is defined:
public class Car {
private Engine engine = new Engine();
}By using the
new
keyword to instantiate a dependency, the specific implementation of that dependency is hardcoded and cannot be substituted.Instead, the dependency should be constructed outside the class and injected in:
public class Car {
private Engine engine;
public Car(Engine engine) {
this.engine = engine;
}
}Using dependency injection it is possible to pass in any engine type:
//Instead of
Car myCar = new Car();
//It's now possible to construct different types of car:
Car petrolCar = new Car(new PetrolEngine);
Car electricCar = new Car(new ElectricEngine);A secondary advantage to Dependency Injection with regards to flexibility and encapsulation is that the class which has the dependency (
Car
, in this example) it not aware of the dependencies of theEngine
class.For example, if the
Engine
class required aGearbox
instance as a constructor argument, theCar
class would need to instantiate and pass in the relevantGearbox
instance. And provide any dependencies of theGearbox
class when instantiating it.If the constructor arguments of any of the classes which need to be instantiated are modified during development, any class which creates an instance of the class must also be modified. A change to the constructor for
Engine
would require modifying theCar
class. Instead, if the fully constructedEngine
instanceBy loosely coupling the
Engine
class to theCar
class, the author of theCar
class does not need to know anything about the implementation ofEngine
class or have knowledge of what dependencies it has.
public Car() {
this.engine = new Engine(new Gearbox());
}Further reading
Additional resources:
References
- Hevery, M. (2008) How to Think About the “new” Operator with Respect to Unit Testing [online]. Available from: http://misko.hevery.com/2008/07/08/how-to-think-about-the-new-operator/
- Hevery, M. (2008) Code Reviewers Guide [online]. Available from: http://misko.hevery.com/code-reviewers-guide/
Please note: This feature is currently proof-of-concept, the instructions may not always be completely accurate.
\Symfony\Component\Routing\RouteCollection
is instantiated inside the constructor ofSymfony\Component\Routing\Loader\Configurator\CollectionConfigurator
1) Remove the new expression and replace it with a variable:
$this->collection = new RouteCollection();
becomes:
$this->collection = $routeCollection;
2) Add a constructor argument for the new variable: Replace:
public function __construct(RouteCollection $parent, string $name, self $parentConfigurator = null, array $parentPrefixes = null)
with:
public function __construct(\Symfony\Component\Routing\RouteCollection $routeCollection, RouteCollection $parent, string $name, self $parentConfigurator = null, array $parentPrefixes = null)
3) Find any occurance of
new Symfony\Component\Routing\Loader\Configurator\CollectionConfigurator
and provide the new dependency.
\Symfony\Component\Routing\Loader\Configurator\CollectionConfigurator
is being instantiated in Loader/Configurator/RoutingConfigurator.php:0Replace:
return new CollectionConfigurator($this->collection, $name);
With:
return new CollectionConfigurator(new \Symfony\Component\Routing\RouteCollection(), $this->collection, $name);
Please note: This feature is currently proof-of-concept, this patch may not work, please don't blindly apply it.
diff --git a/Loader/Configurator/CollectionConfigurator.php b/Loader/Configurator/CollectionConfigurator.php index 09274cc..1b4ef51 100644 --- a/Loader/Configurator/CollectionConfigurator.php +++ b/Loader/Configurator/CollectionConfigurator.php @@ -28,11 +28,11 @@ class CollectionConfigurator private $parentPrefixes; private $host; - public function __construct(RouteCollection $parent, string $name, self $parentConfigurator = null, array $parentPrefixes = null) + public function __construct(\Symfony\Component\Routing\RouteCollection $routeCollection, RouteCollection $parent, string $name, self $parentConfigurator = null, array $parentPrefixes = null) { $this->parent = $parent; $this->name = $name; - $this->collection = new RouteCollection(); + $this->collection = $routeCollection; $this->route = new Route(''); $this->parentConfigurator = $parentConfigurator; // for GC control $this->parentPrefixes = $parentPrefixes; @@ -122,4 +122,4 @@ class CollectionConfigurator { return (clone $this->route)->setPath($path); } -} +} \ No newline at end of file diff --git a/Loader/Configurator/RoutingConfigurator.php b/Loader/Configurator/RoutingConfigurator.php index 4687bf6..f3bccb8 100644 --- a/Loader/Configurator/RoutingConfigurator.php +++ b/Loader/Configurator/RoutingConfigurator.php @@ -57,7 +57,7 @@ class RoutingConfigurator final public function collection(string $name = ''): CollectionConfigurator { - return new CollectionConfigurator($this->collection, $name); + return new CollectionConfigurator(new \Symfony\Component\Routing\RouteCollection(), $this->collection, $name); } /**
Usage of the new keyword in a constructor
Summary of issues
- Tight Coupling
- Hidden dependencies
- Broken Encapsulatioin
Background
If a dependency is constructed inside the object that requires it rather than passed in as a reference then flexibility is lost[1][2]
public class Car {
private Engine engine;
public Car() {
this.engine = new Engine();
}
}Here, the
Car
constructor creates theEngine
instance. This is inflexible as it forces allCar
objects to use the exact sameEngine
type. Instead, it would encourage reuse if the program supported different engine types (e.g.DieselEngine
,PetrolEngine
orHybridEngine
).The same is true when an instance variable is created when the class is defined:
public class Car {
private Engine engine = new Engine();
}By using the
new
keyword to instantiate a dependency, the specific implementation of that dependency is hardcoded and cannot be substituted.Instead, the dependency should be constructed outside the class and injected in:
public class Car {
private Engine engine;
public Car(Engine engine) {
this.engine = engine;
}
}Using dependency injection it is possible to pass in any engine type:
//Instead of
Car myCar = new Car();
//It's now possible to construct different types of car:
Car petrolCar = new Car(new PetrolEngine);
Car electricCar = new Car(new ElectricEngine);A secondary advantage to Dependency Injection with regards to flexibility and encapsulation is that the class which has the dependency (
Car
, in this example) it not aware of the dependencies of theEngine
class.For example, if the
Engine
class required aGearbox
instance as a constructor argument, theCar
class would need to instantiate and pass in the relevantGearbox
instance. And provide any dependencies of theGearbox
class when instantiating it.If the constructor arguments of any of the classes which need to be instantiated are modified during development, any class which creates an instance of the class must also be modified. A change to the constructor for
Engine
would require modifying theCar
class. Instead, if the fully constructedEngine
instanceBy loosely coupling the
Engine
class to theCar
class, the author of theCar
class does not need to know anything about the implementation ofEngine
class or have knowledge of what dependencies it has.
public Car() {
this.engine = new Engine(new Gearbox());
}Further reading
Additional resources:
References
- Hevery, M. (2008) How to Think About the “new” Operator with Respect to Unit Testing [online]. Available from: http://misko.hevery.com/2008/07/08/how-to-think-about-the-new-operator/
- Hevery, M. (2008) Code Reviewers Guide [online]. Available from: http://misko.hevery.com/code-reviewers-guide/
Please note: This feature is currently proof-of-concept, the instructions may not always be completely accurate.
\Symfony\Component\Routing\Route
is instantiated inside the constructor ofSymfony\Component\Routing\Loader\Configurator\CollectionConfigurator
1) Remove the new expression and replace it with a variable:
$this->route = new Route('');
becomes:
$this->route = $route;
2) Add a constructor argument for the new variable: Replace:
public function __construct(RouteCollection $parent, string $name, self $parentConfigurator = null, array $parentPrefixes = null)
with:
public function __construct(\Symfony\Component\Routing\Route $route, RouteCollection $parent, string $name, self $parentConfigurator = null, array $parentPrefixes = null)
3) Find any occurance of
new Symfony\Component\Routing\Loader\Configurator\CollectionConfigurator
and provide the new dependency.
\Symfony\Component\Routing\Loader\Configurator\CollectionConfigurator
is being instantiated in Loader/Configurator/RoutingConfigurator.php:0Replace:
return new CollectionConfigurator(new \Symfony\Component\Routing\RouteCollection(), $this->collection, $name);
With:
return new CollectionConfigurator(new \Symfony\Component\Routing\Route(''), new \Symfony\Component\Routing\RouteCollection(), $this->collection, $name);
Please note: This feature is currently proof-of-concept, this patch may not work, please don't blindly apply it.
diff --git a/Loader/Configurator/CollectionConfigurator.php b/Loader/Configurator/CollectionConfigurator.php index 09274cc..f53cb00 100644 --- a/Loader/Configurator/CollectionConfigurator.php +++ b/Loader/Configurator/CollectionConfigurator.php @@ -28,12 +28,12 @@ class CollectionConfigurator private $parentPrefixes; private $host; - public function __construct(RouteCollection $parent, string $name, self $parentConfigurator = null, array $parentPrefixes = null) + public function __construct(\Symfony\Component\Routing\Route $route, RouteCollection $parent, string $name, self $parentConfigurator = null, array $parentPrefixes = null) { $this->parent = $parent; $this->name = $name; - $this->collection = new RouteCollection(); - $this->route = new Route(''); + $this->collection = $routeCollection; + $this->route = $route; $this->parentConfigurator = $parentConfigurator; // for GC control $this->parentPrefixes = $parentPrefixes; } @@ -122,4 +122,4 @@ class CollectionConfigurator { return (clone $this->route)->setPath($path); } -} +} \ No newline at end of file diff --git a/Loader/Configurator/RoutingConfigurator.php b/Loader/Configurator/RoutingConfigurator.php index 4687bf6..8ebc32e 100644 --- a/Loader/Configurator/RoutingConfigurator.php +++ b/Loader/Configurator/RoutingConfigurator.php @@ -57,7 +57,7 @@ class RoutingConfigurator final public function collection(string $name = ''): CollectionConfigurator { - return new CollectionConfigurator($this->collection, $name); + return new CollectionConfigurator(new \Symfony\Component\Routing\Route(''), new \Symfony\Component\Routing\RouteCollection(), $this->collection, $name); } /**$this->parentConfigurator = $parentConfigurator; // for GC control
$this->parentPrefixes = $parentPrefixes;
}
/**
* @return array
*/
public function __sleep()
{
throw new \BadMethodCallException('Cannot serialize '.__CLASS__);
}
public function __wakeup()
{
throw new \BadMethodCallException('Cannot unserialize '.__CLASS__);
}
public function __destruct()
{
if (null === $this->prefixes) {
$this->collection->addPrefix($this->route->getPath());
}
if (null !== $this->host) {
$this->addHost($this->collection, $this->host);
}
$this->parent->addCollection($this->collection);
}
/**
* Creates a sub-collection.
*/
final public function collection(string $name = ''): self
{
return new self($this->collection, $this->name.$name, $this, $this->prefixes);
}
/**
* Sets the prefix to add to the path of all child routes.
*
* @param string|array $prefix the prefix, or the localized prefixes
*
* @return $this
*/
final public function prefix($prefix): self
{
if (\is_array($prefix)) {
if (null === $this->parentPrefixes) {
// no-op
} elseif ($missing = array_diff_key($this->parentPrefixes, $prefix)) {
throw new \LogicException(sprintf('Collection "%s" is missing prefixes for locale(s) "%s".', $this->name, implode('", "', array_keys($missing))));
} else {
foreach ($prefix as $locale => $localePrefix) {
if (!isset($this->parentPrefixes[$locale])) {
throw new \LogicException(sprintf('Collection "%s" with locale "%s" is missing a corresponding prefix in its parent collection.', $this->name, $locale));
}
$prefix[$locale] = $this->parentPrefixes[$locale].$localePrefix;
}
}
$this->prefixes = $prefix;
$this->route->setPath('/');
} else {
$this->prefixes = null;
$this->route->setPath($prefix);
}
return $this;
}
/**
* Sets the host to use for all child routes.
*
* @param string|array $host the host, or the localized hosts
*
* @return $this
*/
final public function host($host): self
{
$this->host = $host;
return $this;
}
private function createRoute(string $path): Route
{
return (clone $this->route)->setPath($path);
}
}