Use labels in Views argument summaries for entity references

This is technically still blocked on 🐛 Fix label token replacement for views entity reference arguments Fixed . I pushed a single commit with all the changes from that issue into this issue fork, then another commit to actually fix the bug here. It'll be trivial to rebase this and remove the 1st commit once #2640994 lands. But I wanted to get this moving in the hopes we can fix both in time for 10.3.0.

Note to reviewers: Until #2640994 lands, the "Changes" tab in the MR is a ton of stuff you don't need to look at. This is the only change being proposed here:

https://git.drupalcode.org/project/drupal/-/merge_requests/7678/diffs?co...

Thanks,
-Derek

Only comment is is it possible to add typehint and return?

Thanks for taking a look!

Re types: I don’t believe so, since this is implementing a parent method that doesn’t have types and that’d be an api change. I believe. 😂 I’m honestly not sure what our policy is on that.

Not 100% but if it doesn’t break anything adding I’ve seen typehints added. But if it does break then it gets left.

Not sure the plan in the future to go back and update these

I tried this locally:

-  public function summaryName($data) {
+  public function summaryName(array $data): string|\Stringable|NULL {

Then this:

./vendor/bin/phpunit --debug -c core/phpunit.xml core/modules/views/tests/src/Functional/Plugin/EntityArgumentTest.php

Which resulted in this:

There was 1 failure:

1) Drupal\Tests\views\Functional\Plugin\EntityArgumentTest::testArgumentTitle
Test was run in child process and ended unexpectedly

Reverting the type changes to summaryName() and that test is passing fine. So either I botched it somehow, or this just won't work until we add types to Views plugins (or at least to Argument plugins). /shrug

I searched (a lot), and didn't find any test coverage of argument summaries. So I wrote a Kernel test for this on a flight earlier today. The MR still has a ton of changes since this needs 🐛 Fix label token replacement for views entity reference arguments Fixed to land. But I squashed that into a single commit in this issue fork, so it'll be easy to rebase that out of here once that issue lands. If anyone wants to review, the actual changes are just 3 commits:

https://git.drupalcode.org/project/drupal/-/merge_requests/7678/diffs?co...
https://git.drupalcode.org/project/drupal/-/merge_requests/7678/diffs?co...
https://git.drupalcode.org/project/drupal/-/merge_requests/7678/diffs?co...

p.s. I tried triggering the test-only GitLab CI job, but it's all confused by the #2640994 changes. Probably test-only via GitLab won't tell us much until that issue lands. However, locally, if I revert the fix and just run the new test, I see:

There was 1 failure:

1) Drupal\Tests\views\Kernel\Handler\ArgumentSummaryTest::testArgumentSummary
Failed asserting that '1 (4) 2 (2)' contains "phk5vh2z (4)".

Which is exactly what we'd expect. The summary is only showing term IDs and the node counts, not the term labels...

Woo hoo, the blocker is in. This needs a rebase, but then should be ready. I’ll do it later tonight when I’m back at a computer, unless someone else is inspired to do it before then.

1. Rebased so the issue fork branch is clean and back to only the test and the fix.
2. Updated the MR description now that that's done.
3. Updated the summary here with a more common use case (nodes referencing taxonomy terms) and updated remaining tasks.
4. Main pipeline is all green.
5. Test-only failed exactly as intended:

There was 1 failure:
1) Drupal\Tests\views\Kernel\Handler\ArgumentSummaryTest::testArgumentSummary
Failed asserting that '1 (4) 2 (2)' [ASCII](length: 11) contains "rejeaplf (4)" [ASCII](length: 12).
/builds/issue/drupal-3442227/core/modules/views/tests/src/Kernel/Handler/ArgumentSummaryTest.php:146
FAILURES!
Tests: 1, Assertions: 14, Failures: 1.

Ready for review (and hopefully quick RTBC). Would be lovely to also get this in before 10.3.0.

Thanks!
-Derek

Thanks for doing all the review steps @dww haha

Pushed some nitpicky typehints. Wonder if we need a follow up as summaryName() has no return documented, which seems out of scope of this issue definitely.

Confirmed the issue following the steps

Before

After

LGTM!

catch → credited daffie → .

catch → credited plach → .

catch → credited Sinan Erdem → .

This looks good. Adding some credit over from #2912332: Allow entity reference views arguments to display the entity label in titles and summaries → .

Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

catch → closed merge request !7678

Automatically closed - issue fixed for 2 weeks with no activity.