Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[DI] Allow to count on lazy collection arguments#21455

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

Merged
nicolas-grekas merged 1 commit intosymfony:masterfromogizanagi:feature/di/countable_generator
Feb 2, 2017
Merged

[DI] Allow to count on lazy collection arguments#21455

nicolas-grekas merged 1 commit intosymfony:masterfromogizanagi:feature/di/countable_generator
Feb 2, 2017

Conversation

@ogizanagi
Copy link
Contributor

@ogizanagiogizanagi commentedJan 29, 2017•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#21450 (comment)
LicenseMIT
Doc PRtodo (withsymfony/symfony-docs#7336)

When using the new iterator feature of the DI component to lazy load collection, we always know the number of arguments in the collection (only the invalidBehavior set toIGNORE_ON_INVALID_REFERENCE may change this number). So we are able to generate and use aRewindableGenerator implementing\Countable by computing this value ahead.

So, in a service acceptingarray|iterable, like in theGuardAuthenticationListener (see#21450):

class GuardAuthenticationListenerimplements ListenerInterface{private$guardAuthenticators;/**       * @param iterable|GuardAuthenticatorInterface[]  $guardAuthenticators   The authenticators, with keys that match what's passed to GuardAuthenticationProvider       * @param LoggerInterface                         $logger                A LoggerInterface instance    */publicfunction__construct($guardAuthenticators,LoggerInterface$logger =null)    {// ...    }publicfunctionhandle(GetResponseEvent$event)    {if (null !==$this->logger) {$context = array()if (is_array($this->guardAuthenticators) ||$this->guardAuthenticatorsinstanceof \Countable) {$context['authenticators'] =count($this->guardAuthenticators);                        }$this->logger->debug('Checking for guard authentication credentials.',$context);        }// ...    }}

we still keep the ability to call count without loosing the lazy load benefits.

* @param callable $generator
* @param int|null $count If provided, used as the returned value for count. iterator_to_array will be used otherwise.
*/
publicfunction__construct(callable$generator,$count =null)
Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

As this class is internal and used only for this purpose, I think I can drop the$count = null behavior, though.

publicfunctioncount()
{
if (null ===$this->count) {
returncount(iterator_to_array($this->getIterator()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

iterator_count($this->getIterator()) instead?

Copy link
ContributorAuthor

@ogizanagiogizanagiJan 29, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍 (But I'm sure we don't keep this behavior anyway ^^' (#21455 (comment)))

@chalasr
Copy link
Member

I would not add the$count member and always rely oniterator_count($g()) instead, do I miss the point?

@ogizanagi
Copy link
ContributorAuthor

@chalasr: callingiterator_count would miss the purpose of the lazy collection: instantiate items on need. If we only need to get the number of items in the collection, we do already know that, and don't want to trigger the collection items instantiation.

For instance, in#21450, callingiterator_count($this->guardAuthenticators) right before iterating over it would defeat the purpose of your PR.iterator_count will execute the whole generator to get the number of items in there.

chalasr reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas added this to the3.3 milestoneJan 29, 2017
* @param callable $generator
* @param int|null $count If provided, used as the returned value for count. iterator_to_array will be used otherwise.
*/
publicfunction__construct(callable$generator,$count =null)
Copy link
Member

@nicolas-grekasnicolas-grekasJan 29, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Aren't we always able to have the count? If yes, I'd make count mandatory and simplify thecount() method.

Copy link
ContributorAuthor

@ogizanagiogizanagiJan 29, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, it confirms#21455 (comment) (I'll remove this)

@nicolas-grekas
Copy link
Member

Wait, we can't! There maybe be conditional "yield", whenIGNORE_ON_INVALID_REFERENCE is used!

@nicolas-grekas
Copy link
Member

Let's close and tweak the error message instead sorry for the bad suggestion.

@ogizanagi
Copy link
ContributorAuthor

ogizanagi commentedJan 29, 2017•
edited
Loading

There maybe be conditional "yield", when IGNORE_ON_INVALID_REFERENCE is used!

@nicolas-grekas : But this case can be properly handled, right? Seehttps://github.com/ogizanagi/symfony/blob/3dfcbe10ac3aab14922b92afb0818d35cfa8ba8c/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9.php#L344

@nicolas-grekas
Copy link
Member

That's wise :)
Then I suggest turning $count into int|closure : int when known before hand, closure when conditionals. And compute the count lazily.

ogizanagi and chalasr reacted with thumbs up emoji

@ogizanagi
Copy link
ContributorAuthor

Ready to review

@chalasr
Copy link
Member

LGTM 👍

$parameterBag =$this->getParameterBag();

$count =0;
foreach ($value->getValues()as$k =>$v) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

can be wrapped in a closure as second arg below

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Indeed 👍


$code =array();
$code[] ='new RewindableGenerator(function() {';
foreach ($value->getValues()as$k =>$v) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

$v is nice, makes the diff easier to review also

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

$v is nice, but a need to exploit$value again for conditionals count.$v is overwritten by:

$v =$this->wrapServiceConditionals($value, sprintf("        yield %s => %s;\n",$this->dumpValue($k,$interpolate),$this->dumpValue($value,

Can use something else for this though.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

}
}

($c =$this->getServiceConditionals($value)) ?$operands[] ="(int) ($c)" : ++$operands[0];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

when there are several conditionals, this will always evaluate to 1, isn't it?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If there are several conditions, we'll get something like:

(int) ($this->has('foo') &&$this->has('other'))

for a single argument, so should be ok, right? (Actually I don't even know why you would have several conditions for a single service. In case of a collection of service all being ignored on invalid reference?)

Copy link
Member

@nicolas-grekasnicolas-grekasJan 30, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

there are two cases:

  • two optional lines in the list of values: these should count from zero to two condionally
  • a single line with two contitionals inside (eg- [ @?foo, @?bar ]: these count for zero or one only of course

ad both can be mixed, this one counting from zero to two also, (one per line):

-[ '@?foo1', '@?bar1' ]-[ '@?foo2', '@?bar2' ]

}
}

++$count;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

missing return statement?

Copy link
ContributorAuthor

@ogizanagiogizanagiJan 30, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Grumpf. Good catch. 😅

I need to add a test case.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

}
}

($c =$this->getServiceConditionals($v)) ?$operands[] ="(int) ($c)" : ++$operands[0];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

can you move this line before calling wrapServiceConditionals and remove the need for the$y variable?

returnsprintf('array(%s)',implode(',',$code));
}elseif ($valueinstanceof IteratorArgument) {
$countCode =array();
$countCode[] ='function() {';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

There should be a space betweenfunction and() to be valid with PSR-2

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I did first. But any function generated in the dumped container by the PhpDumper givesfunction() currently.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

then you can fix it in this PR also :)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'll rather do a PR on 2.7 to fix other occurrences (if there is in this branch, didn't check yet), don't you think?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ok. There isn't any (appart from 3.3). I'll do it here 😆

$code =implode("\n",array_map(function ($line) {return$line ?''.$line :$line; },explode("\n",$code)));

returnsprintf(" if (%s) {\n%s }\n",implode(' &&',$conditions),$code);
returnsprintf('%s',implode(' &&',$conditions));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

return implode(' && ', $conditions);?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ah ah. Sure 😆
Thanks

chalasr reacted with laugh emoji
@nicolas-grekas
Copy link
Member

👍
Status: reviewed


publicfunctioncount()
{
if (is_callable($count =$this->count)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Do we need the local$count variable here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

we need it as calling$this->count() would fail below, but could be declared inside the if indeed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

No thinking about this again I would keep it as is.

Copy link
Member

@xabbuhxabbuh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍

$code[] ='new RewindableGenerator(function() {';
$code[] ='new RewindableGenerator(function() {';
foreach ($value->getValues()as$k =>$v) {
($c =$this->getServiceConditionals($v)) ?$operands[] ="(int)($c)" : ++$operands[0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Shouldn't there be a space after theint type cast?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You're right. It's in the symfony rules. I don't know why I reverted this.

@stof
Copy link
Member

👍

@nicolas-grekas
Copy link
Member

Thank you@ogizanagi.

@nicolas-grekasnicolas-grekas merged commitf23e460 intosymfony:masterFeb 2, 2017
nicolas-grekas added a commit that referenced this pull requestFeb 2, 2017
…anagi)This PR was merged into the 3.3-dev branch.Discussion----------[DI] Allow to count on lazy collection arguments| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#21450 (comment)| License       | MIT| Doc PR        | todo (withsymfony/symfony-docs#7336)When using the new iterator feature of the DI component to lazy load collection, we always know the number of arguments in the collection (only the invalidBehavior set to `IGNORE_ON_INVALID_REFERENCE` may change this number). So we are able to generate and use a `RewindableGenerator` implementing `\Countable` by computing this value ahead.So, in a service accepting `array|iterable`, like in the `GuardAuthenticationListener` (see#21450):```phpclass GuardAuthenticationListener implements ListenerInterface{    private $guardAuthenticators;    /**       *@param iterable|GuardAuthenticatorInterface[]  $guardAuthenticators   The authenticators, with keys that match what's passed to GuardAuthenticationProvider       *@param LoggerInterface                         $logger                A LoggerInterface instance    */    public function __construct($guardAuthenticators, LoggerInterface $logger = null)    {          // ...    }    public function handle(GetResponseEvent $event)    {        if (null !== $this->logger) {            $context = array()            if (is_array($this->guardAuthenticators) || $this->guardAuthenticators instanceof \Countable) {                $context['authenticators'] = count($this->guardAuthenticators);            }            $this->logger->debug('Checking for guard authentication credentials.', $context);        }        // ...    }}```we still keep the ability to call count without loosing the lazy load benefits.Commits-------f23e460 [DI] Allow to count on lazy collection arguments
@ogizanagiogizanagi deleted the feature/di/countable_generator branchFebruary 2, 2017 14:15
@fabpotfabpot mentioned this pull requestMay 1, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@xabbuhxabbuhxabbuh approved these changes

@chalasrchalasrchalasr left review comments

+1 more reviewer

@jvasseurjvasseurjvasseur left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

7 participants

@ogizanagi@chalasr@nicolas-grekas@stof@jvasseur@xabbuh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp