'#empty' => $this->t('There is no @label yet.', ['@label' => $this->entityType->getLabel()]),

In practice: "There is no Product yet."
We've had this since the early days, and in the meantime entity types got plural labels, allowing us to say "There are no products yet." instead.

So, let's do that.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz created an issue. See original summary.

mohit1604’s picture

Assigned: Unassigned » mohit1604
mohit1604’s picture

Assigned: mohit1604 » Unassigned
Status: Active » Needs review
FileSize
760 bytes

@bojanz , Thanks for the issue , uploading required patch , please review it :)

Status: Needs review » Needs work

The last submitted patch, 3: 2942588-3-D8.patch, failed testing. View results

tstoeckler’s picture

Hey @Mohit Malik: Thanks for creating the patch. In addition to changing the message you should also change the actual variable to use $entity->getPluralLabel().

And apparently there are a number of tests for this string, so you have to update those as well to match the new string.

mohit1604’s picture

Assigned: Unassigned » mohit1604
mohit1604’s picture

Assigned: mohit1604 » Unassigned
Status: Needs work » Needs review
FileSize
1.49 KB

Uploading required patch , please review it :)

Status: Needs review » Needs work

The last submitted patch, 7: 2942588-7-D8.patch, failed testing. View results

tstoeckler’s picture

Thanks @Mohit Malik. The patch looks better now. As you can see a number of automated tests fail, though, because they expect the old strings. So you have to update the tests, as well. If you look at https://www.drupal.org/pift-ci-job/882911 you can see a list of all the failures. For example ActionListTest::testEmptyActionList() asserts that there is a string "There is no Action yet." on the page. But with the patch the string is now "There are no actions yet." so you should update those.

mohit1604’s picture

Assigned: Unassigned » mohit1604
Status: Needs work » Needs review
mohit1604’s picture

Status: Needs review » Needs work
mohit1604’s picture

Assigned: mohit1604 » Unassigned
Status: Needs work » Needs review
FileSize
7.52 KB

@tstoeckler, Thanks for reviewing the patch. Uploading patch with changes mentioned in #9, please review it :)

Status: Needs review » Needs work

The last submitted patch, 12: 2942588-12-D8.patch, failed testing. View results

joshi.rohit100’s picture

@mohit-malik this should be like for example - There are no action entities yet.

Check Drupal\Core\Entity\EntityType::getPluralLabel()

mohit1604’s picture

Assigned: Unassigned » mohit1604
mohit1604’s picture

Assigned: mohit1604 » Unassigned
Status: Needs work » Needs review
FileSize
5.48 KB
7.52 KB

Status: Needs review » Needs work

The last submitted patch, 16: 2942588-16-D8.patch, failed testing. View results

mohit1604’s picture

Assigned: Unassigned » mohit1604
joshi.rohit100’s picture

still missing the #14

mohit1604’s picture

Assigned: mohit1604 » Unassigned
Status: Needs work » Needs review
FileSize
8.25 KB

Did changes as per #14, please review it :)

Status: Needs review » Needs work

The last submitted patch, 20: 2942588-20-D8.patch, failed testing. View results

mohit1604’s picture

Assigned: Unassigned » mohit1604
mohit1604’s picture

Assigned: mohit1604 » Unassigned
Status: Needs work » Needs review
FileSize
967 bytes
7.59 KB

Fixed test, hope it shows green ;)

hchonov’s picture

+++ b/core/modules/action/tests/src/Functional/ActionListTest.php
@@ -31,7 +31,7 @@ public function testEmptyActionList() {
+    $this->assertRaw('There are no action entities yet.');

+++ b/core/modules/block_content/tests/src/Functional/BlockContentListTest.php
@@ -103,7 +103,7 @@ public function testListing() {
+    $this->assertText(t('There are no custom block entities yet.'));

+++ b/core/modules/config/tests/src/Functional/ConfigEntityListTest.php
@@ -243,7 +243,7 @@ public function testListUI() {
+    $this->assertText('There are no test configuration entities yet.');

+++ b/core/modules/responsive_image/tests/src/Functional/ResponsiveImageAdminUITest.php
@@ -35,7 +35,7 @@ protected function setUp() {
+    $this->assertText('There are no responsive image style entities yet.');

@@ -54,7 +54,7 @@ public function testResponsiveImageAdmin() {
+    $this->assertNoText('There are no responsive image style entities yet.');

@@ -137,7 +137,7 @@ public function testResponsiveImageAdmin() {
+    $this->assertText('There are no responsive image style entities yet.');

+++ b/core/modules/workflows/tests/src/Functional/WorkflowUiNoTypeTest.php
@@ -49,7 +49,7 @@ public function testWorkflowUiWithNoType() {
+    $this->assertSession()->pageTextContains('There are no workflow entities yet.');

+++ b/core/modules/workflows/tests/src/Functional/WorkflowUiTest.php
@@ -109,7 +109,7 @@ public function testWorkflowCreation() {
+    $this->assertSession()->pageTextContains('There are no workflow entities yet.');

@@ -253,7 +253,7 @@ public function testWorkflowCreation() {
+    $this->assertSession()->pageTextContains('There are no workflow entities yet.');

Instead of using the default we should define the plural label on all those entity types.

bojanz’s picture

Title: EntityListBuilder::render() should use the entity type plural label in the empty text » [PP-1] EntityListBuilder::render() should use the entity type plural label in the empty text
Status: Needs review » Postponed

Revived #2702683: Add plural labels to entity types, let's get that in first.

chr.fritsch’s picture

Title: [PP-1] EntityListBuilder::render() should use the entity type plural label in the empty text » EntityListBuilder::render() should use the entity type plural label in the empty text
Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, 23: 2942588-23-D8.patch, failed testing. View results

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
7.53 KB
4.93 KB

Fixing tests

Status: Needs review » Needs work

The last submitted patch, 28: interdiff-2942588-23-28.patch, failed testing. View results

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community

Yay, thanks for the updated patch!

alexpott’s picture

Issue tags: +String change in 8.6.0

Granting review credit to everyone.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 27c8064 and pushed to 8.6.x. Thanks!

  • alexpott committed 27c8064 on 8.6.x
    Issue #2942588 by mohit1604, chr.fritsch, tstoeckler, bojanz, joshi....

Status: Fixed » Closed (fixed)

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

daniel.nitsche’s picture

This looks like an improvement, so great work there thanks. However, should this not conform to the user interface standards? Specifically:
https://www.drupal.org/docs/develop/user-interface-standards/table#empty

Empty text
- Use Text pattern "No [things] available. Add [a thing]".
- The "Add [a thing]" link is the same action as the Local action link on the same page.

Happy to open a separate issue, I just thought it was worth mentioning here because core (URL alias screen, content types screen, custom block library screen etc.) seems to follow the standards, while contrib (Pathauto, Webform, Paragraphs) use EntityListBuilder::render(), so there is an inconsistency here.

Happy to open a separate issue if needed.