[DependencyInjection] Implement lazy collection type using generators #20907
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
60fef16 to
19a3df9
Compare
d85d9a8 to
bf39a70
Compare
|
IMO this is ready for review. The Travis failure is not llinked to this PR. |
bf39a70 to
ad4c2da
Compare
|
I fixed some issues found by fabbot.io. |
a19f2ca to
6aa3d65
Compare
fabpot
reviewed
Dec 14, 2016
| @@ -632,7 +633,7 @@ private function addService($id, Definition $definition) | |||
| } | |||
|
|
|||
| if ($definition->isAutowired()) { | |||
| $doc = <<<EOF | |||
| $doc = <<<'EOF' | |||
There was a problem hiding this comment.
Should be reverted. I know that it comes from php-cs-fixer, but we should instead disable this rule in .php.cs.dist
There was a problem hiding this comment.
@fabpot is it intended, even in php cs fixer, or is it a bug ? I don't see the value of quoting the heredoc
|
We miss an equivalent to #19902, which means we miss a way to walk through the values in IteratorArgument in compiler passes. |
93da7d3 to
8985dd8
Compare
pierredup
reviewed
Dec 14, 2016
| @@ -65,4 +68,14 @@ public function getDestNode() | |||
| { | |||
| return $this->destNode; | |||
| } | |||
|
|
|||
| /** | |||
| * Returns true if the edge is lazy, meaning it's a dependency not requiring direct instanciation. | |||
There was a problem hiding this comment.
Typo: instanciation should be instantiation
|
Using generators has one downside, they aren't rewindable meaning services using it will only be able to iterate on them once. If we keep it like this developers will have store the result of the iteration into another variable. This means we will lose in terms of DX and that we will lose the possibility to stop iterating mid-collection (event with stoped propagation, stopping at the first service that accept the input, etc...) losing the laziness of only instantiating the first services. A solution would be to wrap the generator into a class like this one https://github.com/nikic/iter/blob/master/src/iter.rewindable.php#L85-L136 to create rewindable generators. |
|
@jvasseur good point! We just don't need so much code to create a rewindable generator in our case. Here is what I came up with: /**
* @internal
*/
class RewindableGenerator implements \IteratorAggregate
{
private $generator;
public function __construct(callable $generator)
{
$this->generator = $generator;
}
public function getIterator()
{
$g = $this->generator;
return $g();
}
} |
chalasr
reviewed
Dec 15, 2016
| namespace Symfony\Component\DependencyInjection\Argument; | ||
|
|
||
| /** | ||
| * Represents a complex argument containg nested values. |
| public function getValues(); | ||
|
|
||
| /** | ||
| * @param array $values |
| @@ -91,19 +92,25 @@ public function process(ContainerBuilder $container) | |||
| * Processes service definitions for arguments to find relationships for the service graph. | |||
| * | |||
| * @param array $arguments An array of Reference or Definition objects relating to service definitions | |||
| * @param boolean $lazy Whether the references nested in the arguments should be considered lazy or not | |||
There was a problem hiding this comment.
bool (same below) + alignement
| @@ -95,7 +96,14 @@ private function addNodes() | |||
| foreach ($this->nodes as $id => $node) { | |||
| $aliases = $this->getAliases($id); | |||
|
|
|||
| $code .= sprintf(" node_%s [label=\"%s\\n%s\\n\", shape=%s%s];\n", $this->dotize($id), $id.($aliases ? ' ('.implode(', ', $aliases).')' : ''), $node['class'], $this->options['node']['shape'], $this->addAttributes($node['attributes'])); | |||
| $code .= sprintf( | |||
There was a problem hiding this comment.
not sure about the cs change, we have a preference for long lines (and eases review)
dunglas
approved these changes
Dec 20, 2016
| foreach ($lazyContext->lazyValues as $k => $v) { | ||
| ++$i; | ||
|
|
||
| if ($i === 0) { |
| continue; | ||
| } | ||
|
|
||
| if ($i === 1) { |
| continue; | ||
| } | ||
|
|
||
| if ($i === 2) { |
| continue; | ||
| } | ||
|
|
||
| if ($i === 3) { |
| continue; | ||
| } | ||
|
|
||
| if ($i === 4) { |
|
👍 great! |
a2c7e06 to
243fdef
Compare
nicolas-grekas
approved these changes
Dec 26, 2016
c173714 to
51e2d70
Compare
51e2d70 to
7de2571
Compare
|
Great job so far! Some remarks:
|
sstok
approved these changes
Jan 5, 2017
| $code[] = 'new RewindableGenerator(function() {'; | ||
| foreach ($value->getValues() as $k => $v) { | ||
| $v = $this->wrapServiceConditionals($v, sprintf(" yield %s => %s;\n", $this->dumpValue($k, $interpolate), $this->dumpValue($v, $interpolate))); | ||
| foreach (explode("\n", $v) as $v) { |
There was a problem hiding this comment.
This could be moved to a private method for better clarity, indentCode.
|
Yaml done, eg: |
51250f8 to
fbbadf7
Compare
fabpot
reviewed
Jan 5, 2017
| use Symfony\Component\Yaml\Dumper as YmlDumper; | ||
| use Symfony\Component\DependencyInjection\Alias; | ||
| use Symfony\Component\DependencyInjection\Argument\IteratorArgument; |
fbbadf7 to
7b078f4
Compare
7b078f4 to
1dbf52a
Compare
|
👍 |
|
Thank you @tgalopin. |
fabpot
added a commit
that referenced
this pull request
Jan 6, 2017
…sing generators (tgalopin, nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DependencyInjection] Implement lazy collection type using generators | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | - | Fixed tickets | #20875 | License | MIT | Doc PR | - Following the RFC #20875, this PR aims to implement lazy services collections using generators internally. For now, I suggest to support only XML and PHP definitions. Supporting YAML means adding another convention to the language and this is not the aim of this PR. - [x] value object holding the semantic (IteratorArgument) - [x] iterator dumping - [x] lazyness awareness in CheckCircularReferencesPass & GraphvizDumper - [x] rewindable iterators - [x] `*_ON_INVALID_REFERENCE` behavior - [x] ContainerBuilder::createService - [x] ArgumentInterface handling in compiler passes Commits ------- 1dbf52a [DI] Add "=iterator" arguments to Yaml loader 5313943 [DependencyInjection] Implement lazy collection type using generators
symfony-splitter
pushed a commit
to symfony/dependency-injection
that referenced
this pull request
Feb 10, 2017
This PR was squashed before being merged into the 3.3-dev branch (closes #21194). Discussion ---------- [Yaml] Add tags support | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | symfony/symfony#21185 | License | MIT | Doc PR | This PR adds custom tags support to the Yaml component. Symfony tags (`!!binary`, `!str`, etc.) are still managed in the parser to have a lighter diff but we'll be able to convert them later if we want to. The primary addition of this PR is the `TagInterface`: ```php interface TagInterface { public function construct(mixed $value): mixed; } ``` It can be used to register custom tags. An example that could be used to convert [the syntax `=iterator`](symfony/symfony#20907 (comment)) to a tag: ```php final class IteratorTag implements TagInterface { public function construct(mixed $value): mixed { return new IteratorArgument($value); } } $parser = new Parser(['iterator' => new IteratorTag()]); ``` If you think this is too complex, @nicolas-grekas [proposed an alternative](symfony/symfony#21185 (comment)) to my proposal externalizing this support by introducing a new class `TaggedValue`. Commits ------- 4744107 [Yaml] Add tags support
nicolas-grekas
added a commit
that referenced
this pull request
Feb 10, 2017
This PR was squashed before being merged into the 3.3-dev branch (closes #21194). Discussion ---------- [Yaml] Add tags support | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #21185 | License | MIT | Doc PR | This PR adds custom tags support to the Yaml component. Symfony tags (`!!binary`, `!str`, etc.) are still managed in the parser to have a lighter diff but we'll be able to convert them later if we want to. The primary addition of this PR is the `TagInterface`: ```php interface TagInterface { public function construct(mixed $value): mixed; } ``` It can be used to register custom tags. An example that could be used to convert [the syntax `=iterator`](#20907 (comment)) to a tag: ```php final class IteratorTag implements TagInterface { public function construct(mixed $value): mixed { return new IteratorArgument($value); } } $parser = new Parser(['iterator' => new IteratorTag()]); ``` If you think this is too complex, @nicolas-grekas [proposed an alternative](#21185 (comment)) to my proposal externalizing this support by introducing a new class `TaggedValue`. Commits ------- 4744107 [Yaml] Add tags support
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following the RFC #20875, this PR aims to implement lazy services collections using generators internally.
For now, I suggest to support only XML and PHP definitions. Supporting YAML means adding another convention to the language and this is not the aim of this PR.
*_ON_INVALID_REFERENCEbehavior