Skip to content

[DI] Reference instead of inline for array-params #23143

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

Merged
merged 1 commit into from
Jun 14, 2017
Merged

[DI] Reference instead of inline for array-params #23143

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
2 changes: 1 addition & 1 deletion src/Symfony/Component/DependencyInjection/Compiler/PassConfig.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public function __construct()
new ResolveDefinitionTemplatesPass(),
new ServiceLocatorTagPass(),
new DecoratorServicePass(),
new ResolveParameterPlaceHoldersPass(),
new ResolveParameterPlaceHoldersPass(false),
new ResolveFactoryClassPass(),
new FactoryReturnTypePass($resolveClassPass),
new CheckDefinitionValidityPass(),
Expand Down
10 changes: 9 additions & 1 deletion src/Symfony/Component/DependencyInjection/Compiler/ResolveParameterPlaceHoldersPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@
class ResolveParameterPlaceHoldersPass extends AbstractRecursivePass
{
private $bag;
private $resolveArrays;

public function __construct($resolveArrays = true)
{
$this->resolveArrays = $resolveArrays;
}

/**
* {@inheritdoc}
Expand Down Expand Up @@ -55,7 +61,9 @@ public function process(ContainerBuilder $container)
protected function processValue($value, $isRoot = false)
{
if (is_string($value)) {
return $this->bag->resolveValue($value);
$v = $this->bag->resolveValue($value);

return $this->resolveArrays || !$v || !is_array($v) ? $v : $value;
}
if ($value instanceof Definition) {
$changes = $value->getChanges();
Expand Down
25 changes: 19 additions & 6 deletions src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1061,10 +1061,12 @@ private function addDefaultParametersMethod()
*/
public function getParameter($name)
{
$name = strtolower($name);
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
$name = strtolower($name);

if (!(isset($this->parameters[$name]) || array_key_exists($name, $this->parameters) || isset($this->loadedDynamicParameters[$name]))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
}
}
if (isset($this->loadedDynamicParameters[$name])) {
return $this->loadedDynamicParameters[$name] ? $this->dynamicParameters[$name] : $this->getDynamicParameter($name);
Expand All @@ -1080,7 +1082,7 @@ public function hasParameter($name)
{
$name = strtolower($name);

return isset($this->parameters[$name]) || array_key_exists($name, $this->parameters) || isset($this->loadedDynamicParameters[$name]);
return isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters);
}

/**
Expand Down Expand Up @@ -1580,11 +1582,22 @@ private function dumpLiteralClass($class)
*/
private function dumpParameter($name)
{
$name = strtolower($name);

if ($this->container->isCompiled() && $this->container->hasParameter($name)) {
return $this->dumpValue($this->container->getParameter($name), false);
$value = $this->container->getParameter($name);
$dumpedValue = $this->dumpValue($value, false);

if (!$value || !is_array($value)) {
return $dumpedValue;
}

if (!preg_match("/\\\$this->(?:getEnv\('\w++'\)|targetDirs\[\d++\])/", $dumpedValue)) {
return sprintf("\$this->parameters['%s']", $name);
}
}

return sprintf("\$this->getParameter('%s')", strtolower($name));
return sprintf("\$this->getParameter('%s')", $name);
}

/**
Expand Down
14 changes: 14 additions & 0 deletions src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,4 +584,18 @@ public function testPrivateWithIgnoreOnInvalidReference()
$container = new \Symfony_DI_PhpDumper_Test_Private_With_Ignore_On_Invalid_Reference();
$this->assertInstanceOf('BazClass', $container->get('bar')->getBaz());
}

public function testArrayParameters()
{
$container = new ContainerBuilder();
$container->setParameter('array_1', array(123));
$container->setParameter('array_2', array(__DIR__));
$container->register('bar', 'BarClass')
->addMethodCall('setBaz', array('%array_1%', '%array_2%', '%%array_1%%'));
$container->compile();

$dumper = new PhpDumper($container);

$this->assertStringEqualsFile(self::$fixturesPath.'/php/services_array_params.php', str_replace('\\\\Dumper', '/Dumper', $dumper->dump(array('file' => self::$fixturesPath.'/php/services_array_params.php'))));
}
}
10 changes: 6 additions & 4 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services10.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,12 @@ protected function getTestService()
*/
public function getParameter($name)
{
$name = strtolower($name);
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
$name = strtolower($name);
Copy link
Member Author

Choose a reason for hiding this comment

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

µoptim here: conditionally call strtolower. This is a bit more important now that this will use getParameter internally a few more times.

Copy link
Member

Choose a reason for hiding this comment

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

should we deprecate the case insensitivity of parameter names ? This would be consistent with service ids.

Copy link
Member

Choose a reason for hiding this comment

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

+1,000 !!! We should stop changing the user config "magically" behind the scenes ... and that includes service IDs, param IDs, etc. ... unless there are strong technical reasons to do so. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

we may, in another PR :)


if (!(isset($this->parameters[$name]) || array_key_exists($name, $this->parameters) || isset($this->loadedDynamicParameters[$name]))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
}
}
if (isset($this->loadedDynamicParameters[$name])) {
return $this->loadedDynamicParameters[$name] ? $this->dynamicParameters[$name] : $this->getDynamicParameter($name);
Expand All @@ -99,7 +101,7 @@ public function hasParameter($name)
{
$name = strtolower($name);

return isset($this->parameters[$name]) || array_key_exists($name, $this->parameters) || isset($this->loadedDynamicParameters[$name]);
return isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters);
}

/**
Expand Down
10 changes: 6 additions & 4 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services12.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,12 @@ protected function getTestService()
*/
public function getParameter($name)
{
$name = strtolower($name);
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
$name = strtolower($name);

if (!(isset($this->parameters[$name]) || array_key_exists($name, $this->parameters) || isset($this->loadedDynamicParameters[$name]))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
}
}
if (isset($this->loadedDynamicParameters[$name])) {
return $this->loadedDynamicParameters[$name] ? $this->dynamicParameters[$name] : $this->getDynamicParameter($name);
Expand All @@ -103,7 +105,7 @@ public function hasParameter($name)
{
$name = strtolower($name);

return isset($this->parameters[$name]) || array_key_exists($name, $this->parameters) || isset($this->loadedDynamicParameters[$name]);
return isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters);
}

/**
Expand Down
10 changes: 6 additions & 4 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services26.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,12 @@ protected function getTestService()
*/
public function getParameter($name)
{
$name = strtolower($name);
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
$name = strtolower($name);

if (!(isset($this->parameters[$name]) || array_key_exists($name, $this->parameters) || isset($this->loadedDynamicParameters[$name]))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
}
}
if (isset($this->loadedDynamicParameters[$name])) {
return $this->loadedDynamicParameters[$name] ? $this->dynamicParameters[$name] : $this->getDynamicParameter($name);
Expand All @@ -101,7 +103,7 @@ public function hasParameter($name)
{
$name = strtolower($name);

return isset($this->parameters[$name]) || array_key_exists($name, $this->parameters) || isset($this->loadedDynamicParameters[$name]);
return isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters);
}

/**
Expand Down
10 changes: 6 additions & 4 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services8.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,12 @@ public function isFrozen()
*/
public function getParameter($name)
{
$name = strtolower($name);
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
$name = strtolower($name);

if (!(isset($this->parameters[$name]) || array_key_exists($name, $this->parameters) || isset($this->loadedDynamicParameters[$name]))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
}
}
if (isset($this->loadedDynamicParameters[$name])) {
return $this->loadedDynamicParameters[$name] ? $this->dynamicParameters[$name] : $this->getDynamicParameter($name);
Expand All @@ -83,7 +85,7 @@ public function hasParameter($name)
{
$name = strtolower($name);

return isset($this->parameters[$name]) || array_key_exists($name, $this->parameters) || isset($this->loadedDynamicParameters[$name]);
return isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters);
}

/**
Expand Down
10 changes: 6 additions & 4 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_compiled.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,10 +391,12 @@ protected function getServiceFromStaticMethodService()
*/
public function getParameter($name)
{
$name = strtolower($name);
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
$name = strtolower($name);

if (!(isset($this->parameters[$name]) || array_key_exists($name, $this->parameters) || isset($this->loadedDynamicParameters[$name]))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
if (!(isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters))) {
throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
}
}
if (isset($this->loadedDynamicParameters[$name])) {
return $this->loadedDynamicParameters[$name] ? $this->dynamicParameters[$name] : $this->getDynamicParameter($name);
Expand All @@ -410,7 +412,7 @@ public function hasParameter($name)
{
$name = strtolower($name);

return isset($this->parameters[$name]) || array_key_exists($name, $this->parameters) || isset($this->loadedDynamicParameters[$name]);
return isset($this->parameters[$name]) || isset($this->loadedDynamicParameters[$name]) || array_key_exists($name, $this->parameters);
}

/**
Expand Down
Loading