Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] Implement lazy collection type using generators#20907
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
60fef16 to19a3df9Compared85d9a8 tobf39a70Comparetgalopin commentedDec 14, 2016
IMO this is ready for review. The Travis failure is not llinked to this PR. |
tgalopin commentedDec 14, 2016
I fixed some issues found by fabbot.io. |
a19f2ca to6aa3d65Compare| if ($definition->isAutowired()) { | ||
| $doc =<<<EOF | ||
| $doc = <<<'EOF' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@fabpot is it intended, even in php cs fixer, or is it a bug ? I don't see the value of quoting the heredoc
nicolas-grekas commentedDec 14, 2016
We miss an equivalent to#19902, which means we miss a way to walk through the values in IteratorArgument in compiler passes. |
93da7d3 to8985dd8Compare| } | ||
| /** | ||
| * Returns true if the edge is lazy, meaning it's a dependency not requiring direct instanciation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Typo: instanciation should be instantiation
jvasseur commentedDec 15, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 onehttps://github.com/nikic/iter/blob/master/src/iter.rewindable.php#L85-L136 to create rewindable generators. |
nicolas-grekas commentedDec 15, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 RewindableGeneratorimplements \IteratorAggregate{private$generator;publicfunction__construct(callable$generator) {$this->generator =$generator; }publicfunctiongetIterator() {$g =$this->generator;return$g(); }} |
| namespaceSymfony\Component\DependencyInjection\Argument; | ||
| /** | ||
| * Represents a complex argument containg nested values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
typocontaing =>containing
| publicfunctiongetValues(); | ||
| /** | ||
| * @param array $values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
useless
| * 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
bool (same below) + alignement
| $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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not sure about the cs change, we have a preference for long lines (and eases review)
| foreach ($lazyContext->lazyValuesas$k =>$v) { | ||
| ++$i; | ||
| if ($i ===0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should be Yoda style.
| continue; | ||
| } | ||
| if ($i ===1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same here (Yoda).
| continue; | ||
| } | ||
| if ($i ===2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same here (Yoda).
| continue; | ||
| } | ||
| if ($i ===3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same here (Yoda).
| continue; | ||
| } | ||
| if ($i ===4) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same here (Yoda).
dunglas commentedDec 20, 2016
👍 great! |
a2c7e06 to243fdefComparec173714 to51e2d70Compare51e2d70 to7de2571Comparefabpot commentedJan 5, 2017
Great job so far! Some remarks:
|
| $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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This could be moved to a private method for better clarity,indentCode.
nicolas-grekas commentedJan 5, 2017
Yaml done, eg: |
51250f8 tofbbadf7Compare| useSymfony\Component\DependencyInjection\Argument\IteratorArgument; | ||
| useSymfony\Component\Yaml\DumperasYmlDumper; | ||
| useSymfony\Component\DependencyInjection\Alias; | ||
| useSymfony\Component\DependencyInjection\Argument\IteratorArgument; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
To be reverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
reverted
fbbadf7 to7b078f4Compare7b078f4 to1dbf52aComparefabpot commentedJan 5, 2017
👍 |
fabpot commentedJan 6, 2017
Thank you@tgalopin. |
…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 passesCommits-------1dbf52a [DI] Add "=iterator" arguments to Yaml loader5313943 [DependencyInjection] Implement lazy collection type using generators
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`:```phpinterface 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:```phpfinal 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
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`:```phpinterface 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:```phpfinal 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
Uh oh!
There was an error while loading.Please reload this page.
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