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

ogizanagi · 2017-01-29T19:21:50Z

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 (with symfony/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):

class 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.

ogizanagi · 2017-01-29T19:23:26Z

src/Symfony/Component/DependencyInjection/Argument/RewindableGenerator.php

+     * @param callable $generator
+     * @param int|null $count If provided, used as the returned value for count. iterator_to_array will be used otherwise.
+     */
+    public function __construct(callable $generator, $count = null)


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

chalasr · 2017-01-29T19:30:56Z

src/Symfony/Component/DependencyInjection/Argument/RewindableGenerator.php

+    public function count()
+    {
+        if (null === $this->count) {
+            return count(iterator_to_array($this->getIterator()));


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

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

chalasr · 2017-01-29T19:36:05Z

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

ogizanagi · 2017-01-29T19:41:33Z

@chalasr: calling iterator_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, calling iterator_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.

nicolas-grekas · 2017-01-29T20:04:27Z

src/Symfony/Component/DependencyInjection/Argument/RewindableGenerator.php

+     * @param callable $generator
+     * @param int|null $count     If provided, used as the returned value for count. iterator_to_array will be used otherwise.
+     */
+    public function __construct(callable $generator, $count = null)


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

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

nicolas-grekas · 2017-01-29T20:08:13Z

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

nicolas-grekas · 2017-01-29T20:08:46Z

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

ogizanagi · 2017-01-29T20:10:21Z

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

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

nicolas-grekas · 2017-01-29T20:25:44Z

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 · 2017-01-30T08:20:50Z

Ready to review

chalasr · 2017-01-30T09:07:39Z

LGTM 👍

nicolas-grekas · 2017-01-30T09:22:11Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

@@ -966,6 +966,18 @@ public function resolveServices($value)
            }
        } elseif ($value instanceof IteratorArgument) {
            $parameterBag = $this->getParameterBag();
+
+            $count = 0;
+            foreach ($value->getValues() as $k => $v) {


can be wrapped in a closure as second arg below

Indeed 👍

nicolas-grekas · 2017-01-30T09:24:17Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

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


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

$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.

See cfdc528

nicolas-grekas · 2017-01-30T09:28:27Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

                foreach (explode("\n", $v) as $v) {
                    if ($v) {
                        $code[] = '    '.$v;
                    }
                }
+
+                ($c = $this->getServiceConditionals($value)) ? $operands[] = "(int) ($c)" : ++$operands[0];


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

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

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' ]

nicolas-grekas · 2017-01-30T13:16:36Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

+                        }
+                    }
+
+                    ++$count;


missing return statement?

Grumpf. Good catch. 😅

I need to add a test case.

Tested here: https://github.com/symfony/symfony/pull/21455/files#diff-24e0fa9f775c02b92eae9281a338deb6R425

nicolas-grekas · 2017-01-30T13:18:35Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

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


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

jvasseur · 2017-01-30T13:28:14Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

@@ -1387,17 +1403,26 @@ private function dumpValue($value, $interpolate = true)

            return sprintf('array(%s)', implode(', ', $code));
        } elseif ($value instanceof IteratorArgument) {
+            $countCode = array();
+            $countCode[] = 'function() {';


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

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

then you can fix it in this PR also :)

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?

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

chalasr · 2017-01-30T18:33:38Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

-        $code = implode("\n", array_map(function ($line) { return $line ? '    '.$line : $line; }, explode("\n", $code)));
-
-        return sprintf("        if (%s) {\n%s        }\n", implode(' && ', $conditions), $code);
+        return sprintf('%s', implode(' && ', $conditions));


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

Ah ah. Sure 😆
Thanks

nicolas-grekas · 2017-01-31T07:39:38Z

👍
Status: reviewed

xabbuh · 2017-01-30T19:51:51Z

src/Symfony/Component/DependencyInjection/Argument/RewindableGenerator.php

+
+    public function count()
+    {
+        if (is_callable($count = $this->count)) {


Do we need the local $count variable here?

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

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

xabbuh

👍

xabbuh · 2017-01-31T09:37:03Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

            foreach ($value->getValues() as $k => $v) {
+                ($c = $this->getServiceConditionals($v)) ? $operands[] = "(int)($c)" : ++$operands[0];


Shouldn't there be a space after the int type cast?

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

stof · 2017-01-31T09:49:33Z

👍

nicolas-grekas · 2017-02-02T14:11:56Z

Thank you @ogizanagi.

@param

…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 (with symfony/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): ```php class 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

carsonbot added Status: Needs Review DependencyInjection Feature labels Jan 29, 2017

ogizanagi commented Jan 29, 2017

View reviewed changes

ogizanagi mentioned this pull request Jan 29, 2017

[Security] Lazy load guard authenticators and authentication providers #21450

Merged

chalasr reviewed Jan 29, 2017

View reviewed changes

nicolas-grekas added this to the 3.3 milestone Jan 29, 2017

nicolas-grekas reviewed Jan 29, 2017

View reviewed changes

nicolas-grekas reviewed Jan 30, 2017

View reviewed changes

jvasseur reviewed Jan 30, 2017

View reviewed changes

chalasr reviewed Jan 30, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 31, 2017

xabbuh reviewed Jan 31, 2017

View reviewed changes

xabbuh approved these changes Jan 31, 2017

View reviewed changes

xabbuh reviewed Jan 31, 2017

View reviewed changes

[DI] Allow to count on lazy collection arguments

f23e460

nicolas-grekas merged commit f23e460 into symfony:master Feb 2, 2017

ogizanagi deleted the feature/di/countable_generator branch February 2, 2017 14:15

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

stof mentioned this pull request May 19, 2017

[DependencyInjection] RewindableGenerator returns an invalid traversable when IteratorArgument is empty #22780

Closed

+                                      }
+                                  }
+                                  ++$count;

		foreach ($value->getValues() as $k => $v) {
		($c = $this->getServiceConditionals($v)) ? $operands[] = "(int)($c)" : ++$operands[0];

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

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

Conversation

ogizanagi commented Jan 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi Jan 29, 2017 • edited Loading

Choose a reason for hiding this comment

chalasr commented Jan 29, 2017

ogizanagi commented Jan 29, 2017

nicolas-grekas Jan 29, 2017 • edited Loading

Choose a reason for hiding this comment

ogizanagi Jan 29, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Jan 29, 2017

nicolas-grekas commented Jan 29, 2017

ogizanagi commented Jan 29, 2017 • edited Loading

nicolas-grekas commented Jan 29, 2017

ogizanagi commented Jan 30, 2017

chalasr commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jan 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Jan 31, 2017

nicolas-grekas commented Feb 2, 2017

ogizanagi commented Jan 29, 2017 •

edited

Loading

ogizanagi Jan 29, 2017 •

edited

Loading

nicolas-grekas Jan 29, 2017 •

edited

Loading

ogizanagi Jan 29, 2017 •

edited

Loading

ogizanagi commented Jan 29, 2017 •

edited

Loading

nicolas-grekas Jan 30, 2017 •

edited

Loading

ogizanagi Jan 30, 2017 •

edited

Loading