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

Conversation

mnapoli
Copy link
Member

@mnapoli mnapoli commented Nov 4, 2017

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 DefinitionHelper classes) because we can use Definition classes directly in most cases, that will mean less code to execute at runtime and less code to maintain

The main change is this PR is very structural:

  • before: definitions were returned by "sources" (definition files, autowiring, etc.), when they were resolved the "resolver" would handle nested definitions
  • problem: the "resolver" is not able to handle all cases correctly, and its job is also now duplicated in the compiler (that creates the compiled and optimized container)
  • solution: definition "sources" now process (called normalize in the code) definitions and their nested definitions so that they are all clean before going to the resolver or compiler

(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:

  • performance tests
  • changelog
  • documentation

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
@mnapoli mnapoli added this to the 6.0 milestone Nov 4, 2017
@@ -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).

/**
* @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.

@@ -21,6 +21,16 @@
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.)

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.

@mnapoli
Copy link
Member Author

mnapoli commented Nov 4, 2017

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
@mnapoli mnapoli merged commit 4e1fd0e into master Nov 10, 2017
@mnapoli mnapoli deleted the handle-nested-definitions branch November 10, 2017 21:51
@sagikazarmark
Copy link
Contributor

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nested autowire does not work Always interpret closures as factories
2 participants