Skip to content

[DI] Add "by-id" autowiring: a side-effect free variant of it based on the class<>id convention #22060

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.

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

Merged
merged 1 commit into from
Mar 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ private function getContainerDefinitionData(Definition $definition, $omitTags =
'lazy' => $definition->isLazy(),
'shared' => $definition->isShared(),
'abstract' => $definition->isAbstract(),
'autowire' => $definition->isAutowired(),
'autowire' => $definition->isAutowired() ? (Definition::AUTOWIRE_BY_TYPE === $definition->getAutowired() ? 'by-type' : 'by-id') : false,
);

foreach ($definition->getAutowiringTypes(false) as $autowiringType) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ protected function describeContainerDefinition(Definition $definition, array $op
."\n".'- Lazy: '.($definition->isLazy() ? 'yes' : 'no')
."\n".'- Shared: '.($definition->isShared() ? 'yes' : 'no')
."\n".'- Abstract: '.($definition->isAbstract() ? 'yes' : 'no')
."\n".'- Autowired: '.($definition->isAutowired() ? 'yes' : 'no')
."\n".'- Autowired: '.($definition->isAutowired() ? (Definition::AUTOWIRE_BY_TYPE === $definition->getAutowired() ? 'by-type' : 'by-id') : 'no')
;

foreach ($definition->getAutowiringTypes(false) as $autowiringType) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ protected function describeContainerDefinition(Definition $definition, array $op
$tableRows[] = array('Lazy', $definition->isLazy() ? 'yes' : 'no');
$tableRows[] = array('Shared', $definition->isShared() ? 'yes' : 'no');
$tableRows[] = array('Abstract', $definition->isAbstract() ? 'yes' : 'no');
$tableRows[] = array('Autowired', $definition->isAutowired() ? 'yes' : 'no');
$tableRows[] = array('Autowired', $definition->isAutowired() ? (Definition::AUTOWIRE_BY_TYPE === $definition->getAutowired() ? 'by-type' : 'by-id') : 'no');

if ($autowiringTypes = $definition->getAutowiringTypes(false)) {
$tableRows[] = array('Autowiring Types', implode(', ', $autowiringTypes));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ private function getContainerDefinitionDocument(Definition $definition, $id = nu
$serviceXML->setAttribute('lazy', $definition->isLazy() ? 'true' : 'false');
$serviceXML->setAttribute('shared', $definition->isShared() ? 'true' : 'false');
$serviceXML->setAttribute('abstract', $definition->isAbstract() ? 'true' : 'false');
$serviceXML->setAttribute('autowired', $definition->isAutowired() ? 'true' : 'false');
$serviceXML->setAttribute('autowired', $definition->isAutowired() ? (Definition::AUTOWIRE_BY_TYPE === $definition->getAutowired() ? 'by-type' : 'by-id') : 'false');
$serviceXML->setAttribute('file', $definition->getFile());

$calls = $definition->getMethodCalls();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,10 @@ private function getAutowiredReference($type, $autoRegister = true)
return new Reference($type);
}

if (Definition::AUTOWIRE_BY_ID === $this->currentDefinition->getAutowired()) {
return;
}

if (null === $this->types) {
$this->populateAvailableTypes();
}
Expand Down Expand Up @@ -505,10 +509,18 @@ private function createAutowiredDefinition(\ReflectionClass $typeHint)

private function createTypeNotFoundMessage($type, $label)
{
if (!$classOrInterface = class_exists($type, false) ? 'class' : (interface_exists($type, false) ? 'interface' : null)) {
$autowireById = Definition::AUTOWIRE_BY_ID === $this->currentDefinition->getAutowired();
if (!$classOrInterface = class_exists($type, $autowireById) ? 'class' : (interface_exists($type, false) ? 'interface' : null)) {
return sprintf('Cannot autowire service "%s": %s has type "%s" but this class does not exist.', $this->currentId, $label, $type);
}
$message = sprintf('no services were found matching the "%s" %s and it cannot be auto-registered for %s.', $type, $classOrInterface, $label);
if (null === $this->types) {
$this->populateAvailableTypes();
}
if ($autowireById) {
$message = sprintf('%s references %s "%s" but no such service exists.%s', $label, $classOrInterface, $type, $this->createTypeAlternatives($type));
} else {
$message = sprintf('no services were found matching the "%s" %s and it cannot be auto-registered for %s.', $type, $classOrInterface, $label);
}

return sprintf('Cannot autowire service "%s": %s', $this->currentId, $message);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ private function doResolveDefinition(ChildDefinition $definition)
$def->setFile($parentDef->getFile());
$def->setPublic($parentDef->isPublic());
$def->setLazy($parentDef->isLazy());
$def->setAutowired($parentDef->isAutowired());
$def->setAutowired($parentDef->getAutowired());

self::mergeDefinition($def, $definition);

Expand Down Expand Up @@ -147,7 +147,7 @@ public static function mergeDefinition(Definition $def, ChildDefinition $definit
$def->setDeprecated($definition->isDeprecated(), $definition->getDeprecationMessage('%service_id%'));
}
if (isset($changes['autowired'])) {
$def->setAutowired($definition->isAutowired());
$def->setAutowired($definition->getAutowired());
}
if (isset($changes['decorated_service'])) {
$decoratedService = $definition->getDecoratedService();
Expand Down
24 changes: 21 additions & 3 deletions src/Symfony/Component/DependencyInjection/Definition.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
*/
class Definition
{
const AUTOWIRE_BY_TYPE = 1;
const AUTOWIRE_BY_ID = 2;

private $class;
private $file;
private $factory;
Expand All @@ -38,7 +41,7 @@ class Definition
private $abstract = false;
private $lazy = false;
private $decoratedService;
private $autowired = false;
private $autowired = 0;
private $autowiringTypes = array();

protected $arguments;
Expand Down Expand Up @@ -736,20 +739,35 @@ public function setAutowiringTypes(array $types)
* @return bool
*/
public function isAutowired()
{
return (bool) $this->autowired;
}

/**
* Gets the autowiring mode.
*
* @return int
*/
public function getAutowired()
{
return $this->autowired;
}

/**
* Sets autowired.
*
* @param bool $autowired
* @param bool|int $autowired
*
* @return $this
*/
public function setAutowired($autowired)
{
$this->autowired = (bool) $autowired;
$autowired = (int) $autowired;

if ($autowired && self::AUTOWIRE_BY_TYPE !== $autowired && self::AUTOWIRE_BY_ID !== $autowired) {
throw new InvalidArgumentException(sprintf('Invalid argument: Definition::AUTOWIRE_BY_TYPE (%d) or Definition::AUTOWIRE_BY_ID (%d) expected, %d given.', self::AUTOWIRE_BY_TYPE, self::AUTOWIRE_BY_ID, $autowired));
}
$this->autowired = $autowired;

return $this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -734,10 +734,11 @@ private function addService($id, Definition $definition)
}

if ($definition->isAutowired()) {
$autowired = Definition::AUTOWIRE_BY_TYPE === $definition->getAutowired() ? 'types' : 'ids';
$doc .= <<<EOF

*
* This service is autowired.
* This service is autowired by {$autowired}.
EOF;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ private function addService($definition, $id, \DOMElement $parent)
}

if ($definition->isAutowired()) {
$service->setAttribute('autowire', 'true');
$service->setAttribute('autowire', Definition::AUTOWIRE_BY_TYPE === $definition->getAutowired() ? 'by-type' : 'by-id');
}

foreach ($definition->getAutowiringTypes(false) as $autowiringTypeValue) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ private function addService($id, $definition)
}

if ($definition->isAutowired()) {
$code .= " autowire: true\n";
$code .= sprintf(" autowire: %s\n", Definition::AUTOWIRE_BY_TYPE === $definition->getAutowired() ? 'by_type' : 'by_id');
}

$autowiringTypesCode = '';
Expand Down
21 changes: 19 additions & 2 deletions src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ private function getServiceDefaults(\DOMDocument $xml, $file)
}
}
if ($defaultsNode->hasAttribute('autowire')) {
$defaults['autowire'] = XmlUtils::phpize($defaultsNode->getAttribute('autowire'));
$defaults['autowire'] = $this->getAutowired($defaultsNode->getAttribute('autowire'), $file);
}
if ($defaultsNode->hasAttribute('public')) {
$defaults['public'] = XmlUtils::phpize($defaultsNode->getAttribute('public'));
Expand Down Expand Up @@ -238,7 +238,7 @@ private function parseDefinition(\DOMElement $service, $file, array $defaults =
}

if ($value = $service->getAttribute('autowire')) {
$definition->setAutowired(XmlUtils::phpize($value));
$definition->setAutowired($this->getAutowired($value, $file));
} elseif (isset($defaults['autowire'])) {
$definition->setAutowired($defaults['autowire']);
}
Expand Down Expand Up @@ -666,6 +666,23 @@ private function loadFromExtensions(\DOMDocument $xml)
}
}

private function getAutowired($value, $file)
{
if (is_bool($value = XmlUtils::phpize($value))) {
return $value;
}

if ('by-type' === $value) {
return Definition::AUTOWIRE_BY_TYPE;
}

if ('by-id' === $value) {
return Definition::AUTOWIRE_BY_ID;
}

throw new InvalidArgumentException(sprintf('Invalid autowire attribute: "by-type", "by-id", "true" or "false" expected, "%s" given in "%s".', $value, $file));
}

/**
* Converts a \DomElement object to a PHP array.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,14 @@ private function parseDefinition($id, $service, $file, array $defaults)

$autowire = isset($service['autowire']) ? $service['autowire'] : (isset($defaults['autowire']) ? $defaults['autowire'] : null);
if (null !== $autowire) {
if ('by_type' === $autowire) {
$autowire = Definition::AUTOWIRE_BY_TYPE;
} elseif ('by_id' === $autowire) {
$autowire = Definition::AUTOWIRE_BY_ID;
} elseif (!is_bool($autowire)) {
throw new InvalidArgumentException(sprintf('Invalid autowire attribute: "by_type", "by_id", true or false expected, "%s" given in "%s".', is_string($autowire) ? $autowire : gettype($autowire), $file));
}

$definition->setAutowired($autowire);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@
<xsd:element name="tag" type="tag" minOccurs="0" maxOccurs="unbounded" />
</xsd:choice>
<xsd:attribute name="public" type="boolean" />
<xsd:attribute name="autowire" type="boolean" />
<xsd:attribute name="autowire" type="autowire" />
<xsd:attribute name="inherit-tags" type="boolean" />
</xsd:complexType>

Expand Down Expand Up @@ -131,7 +131,7 @@
<xsd:attribute name="decorates" type="xsd:string" />
<xsd:attribute name="decoration-inner-name" type="xsd:string" />
<xsd:attribute name="decoration-priority" type="xsd:integer" />
<xsd:attribute name="autowire" type="boolean" />
<xsd:attribute name="autowire" type="autowire" />
<xsd:attribute name="inherit-tags" type="boolean" />
</xsd:complexType>

Expand All @@ -151,7 +151,7 @@
<xsd:attribute name="public" type="boolean" />
<xsd:attribute name="lazy" type="boolean" />
<xsd:attribute name="abstract" type="boolean" />
<xsd:attribute name="autowire" type="boolean" />
<xsd:attribute name="autowire" type="autowire" />
</xsd:complexType>

<xsd:complexType name="prototype">
Expand All @@ -172,7 +172,7 @@
<xsd:attribute name="lazy" type="boolean" />
<xsd:attribute name="abstract" type="boolean" />
<xsd:attribute name="parent" type="xsd:string" />
<xsd:attribute name="autowire" type="boolean" />
<xsd:attribute name="autowire" type="autowire" />
<xsd:attribute name="inherit-tags" type="boolean" />
</xsd:complexType>

Expand Down Expand Up @@ -279,4 +279,10 @@
<xsd:pattern value="(%.+%|true|false)" />
</xsd:restriction>
</xsd:simpleType>

<xsd:simpleType name="autowire">
<xsd:restriction base="xsd:string">
<xsd:pattern value="(true|false|by-type|by-id)" />
Copy link
Member

Choose a reason for hiding this comment

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

this does not allow parameters anymore, unlike boolean (see just above), so it is a BC break.

Copy link
Member Author

@nicolas-grekas nicolas-grekas Mar 21, 2017

Choose a reason for hiding this comment

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

actually I wanted to trigger this discussion :)
so: in Definition::setAutowired, there is a cast to bool currently.
This means that parameters never worked, did they?
Adding %.+% to the regexp might fix the BC by still allowing one to pass a param,
but it won't fix the fact that this param is ignored.
Or did I miss something?
Btw, this is the case for all definition setters that "cast" - and has never been an issue really (autowiring/abstract/etc. should not be parametric.)

Copy link
Member

Choose a reason for hiding this comment

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

if it never worked, this is fine for me.

</xsd:restriction>
</xsd:simpleType>
</xsd:schema>
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use PHPUnit\Framework\TestCase;
use Symfony\Component\DependencyInjection\Compiler\AutowirePass;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\Exception\RuntimeException;
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\DependencyInjection\Tests\Fixtures\includes\FooVariadic;
Expand Down Expand Up @@ -755,6 +756,53 @@ public function testAlternatives()
$pass = new AutowirePass();
$pass->process($container);
}

public function testById()
{
$container = new ContainerBuilder();

$container->register(A::class, A::class);
$container->register(DInterface::class, F::class);
$container->register('d', D::class)
->setAutowired(Definition::AUTOWIRE_BY_ID);

$pass = new AutowirePass();
$pass->process($container);

$this->assertSame(array('service_container', A::class, DInterface::class, 'd'), array_keys($container->getDefinitions()));
$this->assertEquals(array(new Reference(A::class), new Reference(DInterface::class)), $container->getDefinition('d')->getArguments());
}

public function testByIdDoesNotAutoregister()
{
$container = new ContainerBuilder();

$container->register('f', F::class);
$container->register('e', E::class)
->setAutowired(Definition::AUTOWIRE_BY_ID);

$pass = new AutowirePass();
$pass->process($container);

$this->assertSame(array('service_container', 'f', 'e'), array_keys($container->getDefinitions()));
}

/**
* @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
* @expectedExceptionMessage Cannot autowire service "j": argument $i of method Symfony\Component\DependencyInjection\Tests\Compiler\J::__construct() references class "Symfony\Component\DependencyInjection\Tests\Compiler\I" but no such service exists. This type-hint could be aliased to the existing "i" service; or be updated to "Symfony\Component\DependencyInjection\Tests\Compiler\IInterface".
*/
public function testByIdAlternative()
{
$container = new ContainerBuilder();

$container->setAlias(IInterface::class, 'i');
$container->register('i', I::class);
$container->register('j', J::class)
->setAutowired(Definition::AUTOWIRE_BY_ID);

$pass = new AutowirePass();
$pass->process($container);
}
}

class Foo
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public function isFrozen()
* This service is shared.
* This method always returns the same instance of the service.
*
* This service is autowired.
* This service is autowired by types.
*
* @return \Foo A Foo instance
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ protected function getTestServiceSubscriberService()
* This service is shared.
* This method always returns the same instance of the service.
*
* This service is autowired.
* This service is autowired by types.
*
* @return \TestServiceSubscriber A TestServiceSubscriber instance
*/
Expand All @@ -118,7 +118,7 @@ protected function getFooServiceService()
* If you want to be able to request this service from the container directly,
* make it public, otherwise you might end up with broken code.
*
* This service is autowired.
* This service is autowired by types.
*
* @return \stdClass A stdClass instance
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<container xmlns="http://symfony.com/schema/dic/services" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
<services>
<service id="service_container" class="Symfony\Component\DependencyInjection\ContainerInterface" synthetic="true"/>
<service id="foo" class="Foo" autowire="true"/>
<service id="foo" class="Foo" autowire="by-type"/>
<service id="Psr\Container\ContainerInterface" alias="service_container" public="false"/>
<service id="Symfony\Component\DependencyInjection\ContainerInterface" alias="service_container" public="false"/>
</services>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ services:
synthetic: true
foo:
class: Foo
autowire: true
autowire: by_type
Psr\Container\ContainerInterface:
alias: service_container
public: false
Expand Down