Skip to content
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

Rework how nested definitions are handled (fix #501 & #487) #540

Merged
merged 20 commits into from Nov 10, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
2b390bd
Cover #501 with tests to reproduce issues with nested autowire() defi…
mnapoli Jun 5, 2017
32fb5b3
Emphasize a note in the performances documentation
mnapoli Nov 4, 2017
d93e035
Allow to pass a `$previous` exception in the InvalidDefinition exception
mnapoli Nov 4, 2017
759207d
Traverse definitions to handle nested definitions properly
mnapoli Nov 4, 2017
e771b2e
Extract definition normalization into a dedicated class
mnapoli Nov 4, 2017
fd7bde0
Make definition names settable to remove `EntryReference`
mnapoli Nov 4, 2017
ca69945
Rename `AliasDefinition` to `Reference`
mnapoli Nov 4, 2017
b6ff39d
Remove special cases for DefinitionHelpers that are not needed anymore
mnapoli Nov 4, 2017
c0c467b
Remove the `EnvironmentVariableDefinitionHelper` class
mnapoli Nov 4, 2017
0616139
Remove the `StringDefinitionHelper` class
mnapoli Nov 4, 2017
e3d002a
Remove the `ValueDefinitionHelper` class
mnapoli Nov 4, 2017
1940ca1
Remove the `ArrayDefinitionExtensionHelper` class
mnapoli Nov 4, 2017
baf7de1
Remove useless test now that definitions are already processed
mnapoli Nov 4, 2017
02231ae
Remove useless method
mnapoli Nov 4, 2017
b4ede14
Merge branch 'master' into handle-nested-definitions
mnapoli Nov 4, 2017
79712dc
Cover #487 with a test case
mnapoli Nov 4, 2017
7cc2ca2
Coding style fixes
mnapoli Nov 4, 2017
f902438
#540 changelog
mnapoli Nov 4, 2017
f568561
#540 documentation
mnapoli Nov 4, 2017
73ba9f7
Merge branch 'master' into handle-nested-definitions
mnapoli Nov 10, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion change-log.md
Expand Up @@ -11,7 +11,10 @@ Improvements:
- `DI\autowire()` autowires an object and allows to override specific constructor and method parameters
- [#494](https://github.com/PHP-DI/PHP-DI/pull/494) The container can now be compiled for optimum performances in production
- The container can now be built without parameters: `new Container()`
- [#490](https://github.com/PHP-DI/PHP-DI/issues/490) Support definitions in nested arrays (by [@yuloh](https://github.com/yuloh))
- Definitions can be nested:
- [#490](https://github.com/PHP-DI/PHP-DI/issues/490) Definitions can be nested in arrays (by [@yuloh](https://github.com/yuloh))
- [#501](https://github.com/PHP-DI/PHP-DI/issues/501) & [#540](https://github.com/PHP-DI/PHP-DI/issues/540) Autowire definitions can be nested in other definitions
- [#487](https://github.com/PHP-DI/PHP-DI/issues/487) & [#540](https://github.com/PHP-DI/PHP-DI/issues/540) Closures are now handled as factories when they are nested in other definitions
- [#242](https://github.com/PHP-DI/PHP-DI/issues/242) Error in case a definition is not indexed by a string
- [#505](https://github.com/PHP-DI/PHP-DI/pull/505) Debug container entries

Expand Down
2 changes: 1 addition & 1 deletion doc/how-it-works.md
Expand Up @@ -30,7 +30,7 @@ A definition defines what an entry is:

- **a simple value** (string, number, object instance…): `ValueDefinition`
- **a factory/callable returning the value**: `FactoryDefinition`
- **a definition of an entry alias**: `AliasDefinition`
- **a definition of an entry alias**: `Reference`
- **a definition of a class**: `ObjectDefinition`
- **a definition of an environment variable**: `EnvironmentVariableDefinition`

Expand Down
2 changes: 1 addition & 1 deletion doc/performances.md
Expand Up @@ -56,7 +56,7 @@ As a side note, do not confuse "development environment" with your automated tes

### Optimizing for compilation

As you can read in the "*How it works*" section, PHP-DI will take all the definitions it can find and compile them. That means that definitions like autowired classes that are not listed in the configuration cannot be compiled since PHP-DI doesn't know about them.
As you can read in the "*How it works*" section, PHP-DI will take all the definitions it can find and compile them. That means that definitions like **autowired classes that are not listed in the configuration cannot be compiled** since PHP-DI doesn't know about them.

If you want to optimize performances to a maximum in exchange for more verbosity, you can let PHP-DI know about all the autowired classes by listing them in definition files:

Expand Down
1 change: 1 addition & 0 deletions doc/php-definitions.md
Expand Up @@ -210,6 +210,7 @@ Please note:
- `factory([FooFactory::class, 'build'])`: if `build()` is a **static** method then the object will not be created: `FooFactory::build()` will be called statically (as one would expect)
- you can set any container entry name in the array, e.g. `DI\factory(['foo_bar_baz', 'build'])` (or alternatively: `DI\factory('foo_bar_baz::build')`), allowing you to configure `foo_bar_baz` and its dependencies like any other object
- as a factory can be any PHP callable, you can use invokable objects, too: `DI\factory(InvokableFooFactory::class)` (or alternatively: `DI\factory('invokable_foo_factory')`, if it's defined in the container)
- all closures will be considered by PHP-DI as *factories*, even if they are nested into other definitions like `create()`, `env()`, etc.

#### Retrieving the name of the requested entry

Expand Down
9 changes: 2 additions & 7 deletions src/Compiler.php
Expand Up @@ -5,15 +5,14 @@
namespace DI;

use DI\Compiler\ObjectCreationCompiler;
use DI\Definition\AliasDefinition;
use DI\Definition\ArrayDefinition;
use DI\Definition\DecoratorDefinition;
use DI\Definition\Definition;
use DI\Definition\EnvironmentVariableDefinition;
use DI\Definition\Exception\InvalidDefinition;
use DI\Definition\FactoryDefinition;
use DI\Definition\Helper\DefinitionHelper;
use DI\Definition\ObjectDefinition;
use DI\Definition\Reference;
use DI\Definition\Source\DefinitionSource;
use DI\Definition\StringDefinition;
use DI\Definition\ValueDefinition;
Expand Down Expand Up @@ -126,7 +125,7 @@ private function compileDefinition(string $entryName, Definition $definition) :
$value = $definition->getValue();
$code = 'return ' . $this->compileValue($value) . ';';
break;
case $definition instanceof AliasDefinition:
case $definition instanceof Reference:
$targetEntryName = $definition->getTargetEntryName();
$code = 'return $this->delegateContainer->get(' . $this->compileValue($targetEntryName) . ');';
break;
Expand Down Expand Up @@ -218,10 +217,6 @@ private function compileDefinition(string $entryName, Definition $definition) :

public function compileValue($value) : string
{
if ($value instanceof DefinitionHelper) {
$value = $value->getDefinition('');
}
Copy link
Member Author

Choose a reason for hiding this comment

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

cf. the description of the PR but there are no DefinitionHelper anymore once a definition is returned by the DefinitionSource (the definition is 100% prepared by the source, meaning less work for the compiler and definition resolvers).


// Check that the value can be compiled
$errorMessage = $this->isCompilable($value);
if ($errorMessage !== true) {
Expand Down
24 changes: 14 additions & 10 deletions src/Definition/ArrayDefinition.php
Expand Up @@ -4,8 +4,6 @@

namespace DI\Definition;

use DI\Definition\Helper\DefinitionHelper;

/**
* Definition of an array containing values or references.
*
Expand All @@ -18,19 +16,15 @@ class ArrayDefinition implements Definition
* Entry name.
* @var string
*/
private $name;
private $name = '';

/**
* @var array
*/
private $values;

/**
* @param string $name Entry name
*/
public function __construct(string $name, array $values)
Copy link
Member Author

Choose a reason for hiding this comment

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

There will be the same pattern on several definition classes: by making the name optional that allows me to get rid of several DefinitionHelper classes. The name is simply injected later by the DefinitionSource.

It's a tiny bit less clean, but it removes a lot of code and classes, it simplifies everything and also it makes a little sense because nested definitions don't have names, so… the name is not really mandatory after all.

public function __construct(array $values)
{
$this->name = $name;
$this->values = $values;
}

Expand All @@ -39,11 +33,21 @@ public function getName() : string
return $this->name;
}

public function setName(string $name)
{
$this->name = $name;
}

public function getValues() : array
{
return $this->values;
}

public function replaceNestedDefinitions(callable $replacer)
{
$this->values = array_map($replacer, $this->values);
}

public function __toString()
{
$str = '[' . PHP_EOL;
Expand All @@ -55,8 +59,8 @@ public function __toString()

$str .= ' ' . $key . ' => ';

if ($value instanceof DefinitionHelper) {
$str .= str_replace(PHP_EOL, PHP_EOL . ' ', $value->getDefinition(''));
if ($value instanceof Definition) {
$str .= str_replace(PHP_EOL, PHP_EOL . ' ', $value);
} else {
$str .= var_export($value, true);
}
Expand Down
5 changes: 5 additions & 0 deletions src/Definition/DecoratorDefinition.php
Expand Up @@ -30,6 +30,11 @@ public function getDecoratedDefinition()
return $this->decorated;
}

public function replaceNestedDefinitions(callable $replacer)
{
// no nested definitions
}

public function __toString()
{
return 'Decorate(' . $this->getName() . ')';
Expand Down
10 changes: 10 additions & 0 deletions src/Definition/Definition.php
Expand Up @@ -20,6 +20,16 @@ interface Definition extends RequestedEntry
*/
public function getName() : string;

/**
* Set the name of the entry in the container.
*/
public function setName(string $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.

This is the new method that allows to get rid of several DefinitionHelper classes (see the comment above).


/**
* Apply a callable that replaces the definitions nested in this definition.
*/
public function replaceNestedDefinitions(callable $replacer);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the method that allows the new DefinitionNormalizer to traverse all nested definitions to prepare them (e.g. turn closures into "factory" definitions, process autowire() definition helpers, etc.)


/**
* Definitions can be cast to string for debugging information.
*/
Expand Down
14 changes: 8 additions & 6 deletions src/Definition/Dumper/ObjectDefinitionDumper.php
Expand Up @@ -4,7 +4,7 @@

namespace DI\Definition\Dumper;

use DI\Definition\EntryReference;
use DI\Definition\Definition;
use DI\Definition\ObjectDefinition;
use DI\Definition\ObjectDefinition\MethodInjection;
use ReflectionException;
Expand Down Expand Up @@ -72,8 +72,8 @@ private function dumpProperties(ObjectDefinition $definition) : string

foreach ($definition->getPropertyInjections() as $propertyInjection) {
$value = $propertyInjection->getValue();
if ($value instanceof EntryReference) {
$valueStr = sprintf('get(%s)', $value->getName());
if ($value instanceof Definition) {
$valueStr = (string) $value;
} else {
$valueStr = var_export($value, true);
}
Expand Down Expand Up @@ -109,11 +109,13 @@ private function dumpMethodParameters(string $className, MethodInjection $method
if (array_key_exists($index, $definitionParameters)) {
$value = $definitionParameters[$index];

if ($value instanceof EntryReference) {
$args[] = sprintf('$%s = get(%s)', $parameter->getName(), $value->getName());
if ($value instanceof Definition) {
$valueStr = (string) $value;
Copy link
Member Author

Choose a reason for hiding this comment

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

All these little changes also mean that nested definitions are better supported when debugging and in exception messages.

} else {
$args[] = sprintf('$%s = %s', $parameter->getName(), var_export($value, true));
$valueStr = var_export($value, true);
}
$args[] = sprintf('$%s = %s', $parameter->getName(), $valueStr);

continue;
}

Expand Down
41 changes: 0 additions & 41 deletions src/Definition/EntryReference.php

This file was deleted.

22 changes: 14 additions & 8 deletions src/Definition/EnvironmentVariableDefinition.php
Expand Up @@ -4,8 +4,6 @@

namespace DI\Definition;

use DI\Definition\Helper\DefinitionHelper;

/**
* Defines a reference to an environment variable, with fallback to a default
* value if the environment variable is not defined.
Expand All @@ -18,7 +16,7 @@ class EnvironmentVariableDefinition implements Definition
* Entry name.
* @var string
*/
private $name;
private $name = '';

/**
* The name of the environment variable.
Expand All @@ -43,14 +41,12 @@ class EnvironmentVariableDefinition implements Definition
private $defaultValue;

/**
* @param string $name Entry name
* @param string $variableName The name of the environment variable
* @param bool $isOptional Whether or not the environment variable definition is optional
* @param mixed $defaultValue The default value to use if the environment variable is optional and not provided
*/
public function __construct(string $name, string $variableName, bool $isOptional = false, $defaultValue = null)
public function __construct(string $variableName, bool $isOptional = false, $defaultValue = null)
{
$this->name = $name;
$this->variableName = $variableName;
$this->isOptional = $isOptional;
$this->defaultValue = $defaultValue;
Expand All @@ -61,6 +57,11 @@ public function getName() : string
return $this->name;
}

public function setName(string $name)
{
$this->name = $name;
}

/**
* @return string The name of the environment variable
*/
Expand All @@ -85,14 +86,19 @@ public function getDefaultValue()
return $this->defaultValue;
}

public function replaceNestedDefinitions(callable $replacer)
{
$this->defaultValue = $replacer($this->defaultValue);
}

public function __toString()
{
$str = ' variable = ' . $this->variableName . PHP_EOL
. ' optional = ' . ($this->isOptional ? 'yes' : 'no');

if ($this->isOptional) {
if ($this->defaultValue instanceof DefinitionHelper) {
$nestedDefinition = (string) $this->defaultValue->getDefinition('');
if ($this->defaultValue instanceof Definition) {
$nestedDefinition = (string) $this->defaultValue;
$defaultValueStr = str_replace(PHP_EOL, PHP_EOL . ' ', $nestedDefinition);
} else {
$defaultValueStr = var_export($this->defaultValue, true);
Expand Down
4 changes: 2 additions & 2 deletions src/Definition/Exception/InvalidDefinition.php
Expand Up @@ -13,12 +13,12 @@
*/
class InvalidDefinition extends \Exception
{
public static function create(Definition $definition, string $message) : self
public static function create(Definition $definition, string $message, \Exception $previous = null) : self
{
return new self(sprintf(
'%s' . PHP_EOL . 'Full definition:' . PHP_EOL . '%s',
$message,
(string) $definition
));
), 0, $previous);
}
}
12 changes: 11 additions & 1 deletion src/Definition/FactoryDefinition.php
Expand Up @@ -25,7 +25,7 @@ class FactoryDefinition implements Definition

/**
* Factory parameters.
* @var array
* @var mixed[]
*/
private $parameters = [];

Expand All @@ -46,6 +46,11 @@ public function getName() : string
return $this->name;
}

public function setName(string $name)
{
$this->name = $name;
}

/**
* @return callable Callable that returns the value associated to the entry name.
*/
Expand All @@ -62,6 +67,11 @@ public function getParameters() : array
return $this->parameters;
}

public function replaceNestedDefinitions(callable $replacer)
{
$this->parameters = array_map($replacer, $this->parameters);
}

public function __toString()
{
return 'Factory';
Expand Down