Problem/Motivation

Follow-up to #2640994: Fix label token replacement for views entity reference arguments to also fix argument summaries for entity references. #2912332: Allow entity reference views arguments to display the entity label in titles and summaries proposed doing both, was marked as duplicate with #2640994, but #2640994 doesn't deal with summaries. They currently use the entity ID as the link text, not the (correctly translated) label as everyone expects.

Steps to reproduce

  1. Install Drupal 10.2.5 (for example, just about any modern Drupal will do)
  2. Add an entity reference field to an entity type (e.g. add a 'field_tags' reference to a 'Tags' taxonomy vocabulary)
  3. Create some tag terms
  4. Create some articles that point to tags
  5. Create a View of articles with an argument on the 'field_tags' entity reference
  6. Configure the argument that if the value is not provided to generate a summary
  7. Visit the summary
  8. Behold term IDs, not tag labels

Views was created with plumbing to handle this case (the summaryName() method in argument handlers), but we're not taking advantage of that functionality for entity reference fields.

Proposed resolution

Once #2640994: Fix label token replacement for views entity reference arguments lands, add something like this to core/modules/views/src/Plugin/views/argument/EntityReferenceArgument.php:

  /**
   * {@inheritdoc}
   */
  public function summaryName($data) {
    $id = $data->{$this->name_alias};
    $entity = $id ? $this->entityTypeManager->getStorage($this->definition['target_entity_type_id'])->load($id) : NULL;
    if ($entity) {
      return $this->entityRepository->getTranslationFromContext($entity)->label();
    }
    if (($id === NULL || $id === '') && isset($this->definition['empty field name'])) {
      return $this->definition['empty field name'];
    }
    return $id;
  }

Remaining tasks

  1. Land #2640994: Fix label token replacement for views entity reference arguments
  2. See if we've got existing test coverage of summaries we can extend for this Amazingly, it doesn't seem so.
  3. If not, convert the steps to reproduce into a test (ideally Kernel) Done.
  4. Take the code from the summary and put it in an MR
  5. Reviews / refinements
  6. RTBC
  7. Commit & rejoice

User interface changes

No changes to the Views UI, just that Views start working as expected.

Before

Views summary shows entity IDs

After

Views summary shows entity titles

API changes

Not really a change, but core/modules/views/src/Plugin/views/argument/EntityReferenceArgument overrides summaryName() from its parent (ArgumentPluginBase)

Data model changes

Release notes snippet

CommentFileSizeAuthor
#15 Screenshot 2024-05-31 at 2.26.44 PM.png18.95 KBsmustgrave
#15 Screenshot 2024-05-31 at 2.26.11 PM.png14.66 KBsmustgrave
#2 3442227-2.after_.png45.62 KBdww
#2 3442227-2.before.png28.97 KBdww

Issue fork drupal-3442227

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

dww created an issue. See original summary.

dww’s picture

dww
we/he/they
commented
Issue summary: View changes
StatusFileSize
new 3442227-2.before.png28.97 KB
new 3442227-2.after_.png45.62 KB
dww’s picture

dww
we/he/they
commented
Issue summary: View changes

dww opened merge request !7678

dww’s picture

dww
we/he/they
commented
Title: [PP-1] Use labels in Views argument summaries for entity references » Use labels in Views argument summaries for entity references
Issue summary: View changes
Status: Active » Needs review

This is technically still blocked on #2640994: Fix label token replacement for views entity reference arguments. 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.

dww’s picture

dww
we/he/they
commented

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Postponed

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

dww’s picture

dww
we/he/they
commented

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.

smustgrave’s picture

smustgrave commented

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

dww’s picture

dww
we/he/they
commented

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

dww’s picture

dww
we/he/they
commented
Issue summary: View changes

Needs tests

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 #2640994: Fix label token replacement for views entity reference arguments 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...

dww’s picture

dww
we/he/they
commented

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

dww’s picture

dww
we/he/they
commented
Status: Postponed » Needs work

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.

dww’s picture

dww
we/he/they
commented
Issue summary: View changes
Status: Needs work » Needs review
  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

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative
StatusFileSize
new Screenshot 2024-05-31 at 2.26.11 PM.png14.66 KB
new Screenshot 2024-05-31 at 2.26.44 PM.png18.95 KB

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

before

After

after

LGTM!

  • catch committed 170919be on 10.3.x 
    Issue #3442227 by dww, smustgrave: Use labels in Views argument...

  • catch committed f72d926d on 10.4.x 
    Issue #3442227 by dww, smustgrave: Use labels in Views argument...

  • catch committed d729516c on 11.0.x 
    Issue #3442227 by dww, smustgrave: Use labels in Views argument...

  • catch committed a62995de on 11.x 
    Issue #3442227 by dww, smustgrave: Use labels in Views argument...

catch credited daffie.

catch credited plach.

catch credited Sinan Erdem.

catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

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