Let Twig without() filter take both arrays and strings as arguments

Still needs tests, but let's make sure this doesn't break any current tests...

Manual testing locally works great.

Found the existing tests for TwigExtension, so added a trivial case to prove this works. Passes locally. Can't imagine how this breaks anything else. What's the word, bot?

Sweet, thanks!

Updating remaining tasks. ;)

Thanks for the reviews!

Sadly, the new docs are indeed wrong, or at least, misleading. You can't pass an array of arrays. But probably you should be able to. ;)

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -666,6 +666,10 @@ public function withoutFilter($element) {
+    // Allows use a Twig array for the arguments.

Typo.

I'll fix these given the current approach.

This fixes the doc problems from #8.

However, now that I'm starting to use this on a real site, I'm seeing a need for either multiple arrays, or a mix of arrays and strings. Otherwise, you have to jump through some hoops with merge() inside your Twig template. We're merging here, anyway. Might as well make this even easier.

Stay tuned.

Now this allows mixed[] as the arguments, either arrays of keys, or directly string keys. With updated docs and tests.

Maybe there's a slightly more elegant want to do this, but I didn't want to have to iterate through $args twice to make a single $keys array first, then actually unset() stuff. Nitpicks welcome. ;)

I added this as a separate option B to the summary's proposed solution, showing why allowing mixed is even better in real world scenarios.

Also needs review and RTBC again, so updating remaining tasks... ;)

I'll wait for a CR until we agree on #11 vs. this approach.

Leaving both #11 and #12 patches visible in the file table, since those are the two viable approaches to consider.

Cheers,
-Derek

Re: #13:

new RecursiveIteratorIterator(new RecursiveArrayIterator($args)); to simplify your loop.

Hah, MUCH simpler. ;) 🤦‍♂️What's the world coming to? ;)

Re: #14 Nice catch. Fixed here.

Re: #17.2: Sure, why not? Moar Tests(tm) is always good. ;) Especially since it's inside an existing test class and it's effectively free to test more combinations of this.

For comparison, 2 alternate approaches...

This does an initial loop to find all the keys.
Then a second loop to unset them.

No changes to test coverage. Passes locally.

With the "simple" solution from #13. ;)

edit: Note: we need \ in front for PHP to find the right class. These are PHP native classes. Using exactly #13 fails with:

1) Drupal\Tests\system\Kernel\Theme\TwigFilterTest::testTwigWithoutFilter
Error: Class 'Drupal\Core\Template\RecursiveIteratorIterator' not found

#20 plus a code comment to explain WTF. ;)

Note: if we go with #18 and up, the issue title should be changed to:

"Let our Twig without() filter take arrays and strings as arguments"

before commit.

Thanks!
-Derek

Canceled the test run on #20. If we do that, we need the code comment from #21. ;) So let's save the bot cycles churning on #20...

Put the implementation comparison for #18 vs. #19 vs. #21 into the summary for easier review.

Okay, people, let's pick. ;)

Thanks!
-Derek

@Krzysztof Domański re: #25

Agreed on option B. I think it'd be weird to require and handle only a single array, and would probably result in more trouble than it helps. Preemptively solving #22 by re-titling this now. If someone really thinks we need option A, we can reset it.

Re: #25.1: wow, the "cure" is turning into something worse than the "disease". ;) So much for simple. :/ How, exactly, would someone be able to pass an object into this inside a twig template? Seems like unnecessary complication.

Re: #25.2: I don't think so. The whole point is that the $args array from func_get_args() might include nested arrays. That's what a RecursiveArrayIterator handles. If we make this just an ArrayIterator, we'd only walk through the keys in $args itself, not sub arrays. I think. ;)

Re: #25.2 : I just confirmed #27 in local testing:

Doing exactly this:

    $iterator = new \RecursiveIteratorIterator(new \ArrayIterator($args));

Results in this:

1) Drupal\Tests\system\Kernel\Theme\TwigFilterTest::testTwigWithoutFilter
InvalidArgumentException: An instance of RecursiveIterator or IteratorAggregate creating it is required

Doing this:

    $iterator = new \ArrayIterator($args);

Results in this:

There was 1 error:

1) Drupal\Tests\system\Kernel\Theme\TwigFilterTest::testTwigWithoutFilter
Illegal offset type in isset or empty

/Applications/MAMP-common/htdocs/drupal-8_8/core/lib/Drupal/Core/Template/Attribute.php:159
/Applications/MAMP-common/htdocs/drupal-8_8/core/lib/Drupal/Core/Template/TwigExtension.php:675

Line 675 would be this:

      if (isset($filtered_element[$key])) {

So yeah, as expected, we need \RecursiveArrayIterator to get any benefit from #21 over #18.

Cheers,
-Derek

Sure. If we're cool with mix of strings and multiple arrays (#21) here we go:

https://www.drupal.org/node/3102853

I assumed this won't be backported to 8.8.x, so I put it as introduced in 8.9.x branch and 8.9.0-alpha1 release. Please adjust accordingly when committing this and publishing that.

Thanks,
-Derek

I guess it's safe to restore the status, too, since the only objection to RTBC was the lack of a CR...

Re: #33.1: If you prefer, sure. I still think mixed[] is more true, but I don't really feel that strongly.

Re: #33.2. Okay, fixed.

Thanks,
-Derek

@lauriii re: #35: I've never found a good reference on The Right Way(tm) to document ... in general, that's all. :) You're totally right that's what each element of ... can be: a string, or an array of strings. But technically the "..." is itself an array, and its children elements are of mixed type (strings or arrays). That's my confusion. But if we ignore the array, #33.1 is indeed better. I'll plan to do that from now on whenever documenting ... args.

Can we call #34 RTBC?

Thanks!
-Derek