508 Compliance Issue -Edit links on content page are not unique

Added an else statement to use a different label is $entity->label is empty.

The last submitted patch, 34: 2852898-34.patch, failed testing. View results →

Lets try this

+++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
@@ -130,17 +130,37 @@ public function getOperations(EntityInterface $entity) {
+          $label = $this->t('Edit') . ' ' . $entity->label();
...
+          $label = $this->t('Edit') . ' ' . $entity->bundle() . ' ' . $this->t('ID') . ' ' . $entity->id();
...
+          $label = $this->t('Delete') . ' ' . $entity->label();
...
+          $label = $this->t('Delete') . ' ' . $entity->bundle() . ' ' . $this->t('ID') . ' ' . $entity->id();

From a translation perspective, these are looking quite bad. Using concatenation prevents some languages to provide an accurate translation because not all language has the words in the same order.

I'd suggest to use string placeholders, for example: "Edit @entity_bundle ID @entity_id"

Like this?

The last submitted patch, 38: 2852898-38.patch, failed testing. View results →

+++ b/core/modules/config/tests/src/Functional/ConfigEntityListTest.php
@@ -57,11 +57,17 @@ public function testList() {
+    $edit_url->setOption('attributes', ['aria-label' => 'Edit ' . $entity->label()]);
...
+    $delete_url->setOption('attributes', ['aria-label' => 'Delete ' . $entity->label()]);

@@ -132,16 +138,22 @@ public function testList() {
+    $edit_url->setOption('attributes', ['aria-label' => 'Edit ' . $entity->label()]);
...
+    $delete_url->setOption('attributes', ['aria-label' => 'Delete ' . $entity->label()]);

+++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
@@ -75,7 +75,10 @@ public function testUserAdmin() {
+      'attributes' => ['aria-label' => 'Edit ' . $user_a->label()],

Should be translated here as well?

addressed #41

@borisson_ I don't think they need to be as they are tests that aren't checking translations.

@borisson_ I don't think they need to be as they are tests that aren't checking translations.

Actually you are right, I didn't notice these were in the tests. In that case I think that #40 looks good to go.

The last submitted patch, 40: 2852898-40.patch, failed testing. View results →

Looking good, needs work/review for the question about bundle labels vs machine names

1. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
   @@ -130,17 +130,37 @@ public function getOperations(EntityInterface $entity) {
   +      if (!is_null($edit_url)) {
   +        if (!empty($entity->label())) {
   +          $label = $this->t('Edit @entity_label', ['@entity_label' => $entity->label()]);
   

   Is this something we should be doing in Entity::toUrl?

   Otherwise this would need to be piecemeal in quite a few places

2. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
   @@ -130,17 +130,37 @@ public function getOperations(EntityInterface $entity) {
   +          $label = $this->t('Edit @entity_bundle @entity_id', ['@entity_bundle' => $entity->bundle(), '@entity_id' => $entity->id()]);
   

   This uses the bundle machine name - are we sure we want to do that - should we instead be fetching the bundle label?

3. +++ b/core/modules/config/tests/src/Functional/ConfigEntityListTest.php
   @@ -132,16 +138,22 @@ public function testList() {
        // Test getOperations() method.
   +    $edit_url = $entity->toUrl()->setOption('query', $this->getRedirectDestination()->getAsArray());
   +    $edit_url->setOption('attributes', ['aria-label' => 'Edit ' . $entity->label()]);
   +
   +    $delete_url = $entity->toUrl('delete-form')->setOption('query', $this->getRedirectDestination()->getAsArray());
   +    $delete_url->setOption('attributes', ['aria-label' => 'Delete ' . $entity->label()]);
   +
   

   We don't seem to have test coverage for the two else cases (when the edit url is null, and when the entity label is empty)

Response to #46

1. Not sure I follow what you mean?
2. Can't use the label in that scenario the label doesn't exist.
3. Added a test for checking when label is empty.

Going to test if one of the if statements is truely needed.

So the is_null check was needed to setOption.

But not sure we need a test. $this->ensureDestination($entity->toUrl('delete-form') was part of the render array. I just moved it out to a variable so we can setOption on it.

For the questions in #49

Hiding patches
Updating issue credits

Commented on the MR

Addressed feedback

Sorry to be a pain, but I don't think we need to handle when the Urls are null, ::ensureDestination requires a Url object and calls a fluent setter on the object (thus always returning a Url)

Removing the null check opened 🐛 toUrl returns Null Active

The blocker is in

Rebased! Tests should pass now

• Looks like issues have been addressed.
• Tests exist and pass.
• Code looks good to me.
• Seems to work great
• Properly escapes data

Posted one comment in the MR 😇

Back to NR, the last requested change may not be needed, per https://www.drupal.org/project/drupal/issues/2852898#mr4211-note219576 🐛 508 Compliance Issue -Edit links on content page are not unique Needs review

Reviewing at Drupalcon 2023.

Based on the comments above from @marvil07, moving to RTC as not necessary.

@lauriii - are you happy with this?

Committed to 11.x

Didn't backport to 10.2.x because there are string changes here

larowlan → closed merge request !4211

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