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

Projects
None yet
2 participants
@mnapoli
Member

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 #501: support nested autowire (and includes #503 which were the tests), some examples:

        [
            Foo::class => create()
                ->constructor(autowire(Bar::class)),
            'array' => [
                autowire(...),
                autowire(...),
            ],
        ]
  • fixes #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

mnapoli added some commits Jun 5, 2017

Remove special cases for DefinitionHelpers that are not needed anymore
Definitions are now completely prepared by the definition source thanks to the normalizer.
Remove the `EnvironmentVariableDefinitionHelper` class
Use the definition class directly to simplify and optimize everything.
Remove the `StringDefinitionHelper` class
Use the definition class directly to simplify and optimize everything.
Remove the `ValueDefinitionHelper` class
Use the definition class directly to simplify and optimize everything.
Remove the `ArrayDefinitionExtensionHelper` class
Use the definition class directly to simplify and optimize everything.
Remove useless method
This class no longer implements the Definition interface.
Merge branch 'master' into handle-nested-definitions
# 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 the enhancement label Nov 4, 2017

@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('');
}

This comment has been minimized.

@mnapoli

mnapoli Nov 4, 2017

Member

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)

This comment has been minimized.

@mnapoli

mnapoli Nov 4, 2017

Member

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);

This comment has been minimized.

@mnapoli

mnapoli Nov 4, 2017

Member

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);

This comment has been minimized.

@mnapoli

mnapoli Nov 4, 2017

Member

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;

This comment has been minimized.

@mnapoli

mnapoli Nov 4, 2017

Member

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

@mnapoli

This comment has been minimized.

Member

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:

mnapoli added some commits Nov 4, 2017

Merge branch 'master' into handle-nested-definitions
# Conflicts:
#	src/Definition/ObjectDefinition/PropertyInjection.php

@mnapoli mnapoli merged commit 4e1fd0e into master Nov 10, 2017

5 checks passed

Scrutinizer 2 new issues, 35 updated code elements
Details
continuous-integration/prettyci The PrettyCI build has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.4%) to 95.386%
Details

@mnapoli mnapoli deleted the handle-nested-definitions branch Nov 10, 2017

@sagikazarmark

This comment has been minimized.

Contributor

sagikazarmark commented Nov 10, 2017

Awesome!

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