EntityDefinitionUpdateManager: Show field_name when field has no label

Looks good to me.

Thanks for filing this bug report and for fixing it. Bug fixing is very valuable. However in order to commit a bug fix we need an automated to test to prove that we've fixed the bug and ensure that we don't break it again in the future. For more information about writing tests in Drupal 8 see the following links:

1. https://www.drupal.org/docs/8/testing
2. https://api.drupal.org/api/drupal/core%21core.api.php/group/testing/8.7.x

I found a nitpick, that should be fixed before we can get this in. Overall this looks really good and I think when this small change is done we can mark this RTBC.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
@@ -271,6 +271,42 @@ public function testBaseFieldCreateUpdateDeleteWithoutData() {
+        t('The %field_name field needs to be installed.', ['%field_name' => 'new_base_field']),
...
+        t('The %field_name field needs to be updated.', ['%field_name' => 'new_base_field']),
...
+        t('The %field_name field needs to be uninstalled.', ['%field_name' => 'new_base_field']),

Should be $this->t() instead of the deprecated t().

You are right! In theory this doesn't need to be t because it doesn't the translatability aspect of it.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
@@ -271,6 +271,42 @@ public function testBaseFieldCreateUpdateDeleteWithoutData() {
+    $this->addBaseField('string', 'entity_test_update', FALSE, FALSE);
+    $expected = [
+      'entity_test_update' => [
+        t('The %field_name field needs to be installed.', ['%field_name' => 'new_base_field']),
+      ],
+    ];
+    $this->assertEqual($this->entityDefinitionUpdateManager->getChangeSummary(), $expected, 'EntityDefinitionUpdateManager reports the expected change summary.');

Can we change this to...

    $this->addBaseField('string', 'entity_test_update', FALSE, FALSE);
    $changes = $this->entityDefinitionUpdateManager->getChangeSummary();
    $this->assertCount(1, $changes['entity_test_update']);
    $this->assertEquals('The new_base_field field needs to be installed.', strip_tags($changes['entity_test_update'][0]));

• This doesn't use t()
• Makes the asserts simple to read
• Uses the correct assert functions in the correct way (arguments the right way around)
• Does not have assert messages because the default messages are better.

Agreed with @alexpott & comments addressed in the updated patch. Also, resolved some small nitpicks & added an interdiff.

I think the latest version of this patch looks good - the remarks made by @alexpott have been resolved, this being a bug-fix it probably still is set to the right version.

Committed and pushed f66921a662 to 9.0.x and b3b6a681de to 8.9.x. Thanks!

As a bugfix going to talk to other committers about backport to 8.8.x

@catch +1'd the backport