Rework how nested definitions are handled (fix #501 & #487)#540
Rework how nested definitions are handled (fix #501 & #487)#540
Conversation
Definitions are now completely prepared by the definition source thanks to the normalizer.
Use the definition class directly to simplify and optimize everything.
Use the definition class directly to simplify and optimize everything.
Use the definition class directly to simplify and optimize everything.
Use the definition class directly to simplify and optimize everything.
This class no longer implements the Definition interface.
# Conflicts: # src/Definition/Definition.php # src/Definition/HasSubDefinition.php # src/Definition/ObjectDefinition/PropertyInjection.php # tests/UnitTest/Definition/ArrayDefinitionExtensionTest.php # tests/UnitTest/Definition/DecoratorDefinitionTest.php # tests/UnitTest/Definition/Helper/ArrayDefinitionExtensionHelperTest.php # tests/UnitTest/FunctionsTest.php
| { | ||
| if ($value instanceof DefinitionHelper) { | ||
| $value = $value->getDefinition(''); | ||
| } |
There was a problem hiding this comment.
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).
| /** | ||
| * @param string $name Entry name | ||
| */ | ||
| public function __construct(string $name, array $values) |
There was a problem hiding this comment.
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.
|
|
||
| /** | ||
| * Set the name of the entry in the container. | ||
| */ |
There was a problem hiding this comment.
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. | ||
| */ |
There was a problem hiding this comment.
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.)
| if ($value instanceof EntryReference) { | ||
| $args[] = sprintf('$%s = get(%s)', $parameter->getName(), $value->getName()); | ||
| if ($value instanceof Definition) { | ||
| $valueStr = (string) $value; |
There was a problem hiding this comment.
All these little changes also mean that nested definitions are better supported when debugging and in exception messages.
|
Performance tests: I see no difference, both when the container is compiled or not. So no performance impact and a simpler codebase: ✅ |
# Conflicts: # src/Definition/ObjectDefinition/PropertyInjection.php
|
Awesome! |
2 participants
Sorry for the big PR if you intend to review, the work behind that kinda grow organically from a small change :) Reworking those commits would be very complicated.
This PR solves the problem of nested definitions, i.e.:
fixes Nested autowire does not work #501: support nested autowire (and includes Tests reproducing #501 (nested autowire) #503 which were the tests), some examples:
[ Foo::class => create() ->constructor(autowire(Bar::class)), 'array' => [ autowire(...), autowire(...), ], ]fixes Always interpret closures as factories #487: support nested anonymous functions (aka factories), some examples:
[ Foo::class => create() ->constructor(function () { return ...; }), 'array' => [ function () { return ...; }, function () { return ...; }, ], 'env' => \DI\env('FOO', function () { return ...; }), ]removes dead code
simplifies the code
removes some useless classes (some
DefinitionHelperclasses) because we can useDefinitionclasses directly in most cases, that will mean less code to execute at runtime and less code to maintainThe main change is this PR is very structural:
(to normalize definitions they are now like "tree nodes", i.e. the normalizer visits each definition and its subdefinitions to prepare it for the resolver)
That means a bit more job in definition sources, less in resolvers and compiler. The code is simpler and handles all edge cases the same way \o/
TODO: