Follow-up: Applying entity schema updates still fails when both field and entity type definitions changes

Wasn't #2342543: Applying entity schema updates fail when both entity type and base field definitions change at the same time supposed to fix this?

Yes, it was, are you sure you tested with latest dev snapshot and not a beta?

I would almost want to bump this to critical considering that #2342543: Applying entity schema updates fail when both entity type and base field definitions change at the same time was critical and the issue isn't fully resolved yet. But I'll leave it up to someone else to decide on that. I'm changing the title though to make it clear that this closely related to the original issue.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1076,7 +1076,9 @@ protected function performFieldSchemaOperation($operation, FieldStorageDefinitio
+      if (!$this->database->schema()->tableExists($name)) {
+        $this->database->schema()->createTable($name, $table);
+      }
     }
     $this->saveFieldSchemaData($storage_definition, $schema);
   }
@@ -1109,11 +1111,15 @@ protected function createSharedTableSchema(FieldStorageDefinitionInterface $stor

@@ -1109,11 +1111,15 @@ protected function createSharedTableSchema(FieldStorageDefinitionInterface $stor
           $schema[$table_name] = $this->getSharedTableFieldSchema($storage_definition, $table_name, $column_names);
           if (!$only_save) {
             foreach ($schema[$table_name]['fields'] as $name => $specifier) {
-              $schema_handler->addField($table_name, $name, $specifier);
+              if (!$schema_handler->fieldExists($table_name, $name)) {
+                $schema_handler->addField($table_name, $name, $specifier);
+              }
             }
             if (!empty($schema[$table_name]['indexes'])) {
               foreach ($schema[$table_name]['indexes'] as $name => $specifier) {
-                $schema_handler->addIndex($table_name, $name, $specifier);
+                if (!$schema_handler->indexExists($table_name, $name)) {
+                  $schema_handler->addIndex($table_name, $name, $specifier);
+                }
               }

I think it would be good to add a comment to each of these three sections, saying something like "The table/field/index might already have been created as part of the earlier entity type update event."

+++ b/core/modules/system/src/Tests/Entity/EntityDefinitionTestTrait.php
@@ -101,6 +101,21 @@ protected function addBaseField($type = 'string') {
+      ->setLabel(t('A new base field'))

Let's change the description to "A new revisionable base field"

+++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
@@ -559,4 +559,23 @@ public function testEntityTypeSchemaUpdateAndBaseFieldCreateWithoutData() {
+  /**
+   * Tests updating entity schema and creating a revisionable base field at the same time when there are no existing entities.
+   */
+  public function testEntityTypeSchemaUpdateAndRevisionableBaseFieldCreateWithoutData() {

Minor nit-pick - let's also wrap this comment line at 80 characters.

Thanks @jeqq! IMO this one is ready.

Looks great, thanks, only a couple of minor issues:

1. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
   @@ -541,7 +541,8 @@ public function testDefinitionEvents() {
   +   * Tests updating entity schema and creating a base field at the same time
   +   * when there are no existing entities.
   
   @@ -559,4 +560,24 @@ public function testEntityTypeSchemaUpdateAndBaseFieldCreateWithoutData() {
   +   * Tests updating entity schema and creating a revisionable base field at the
   +   * same time when there are no existing entities.
   +   */
   

   These need to be a single line shorter than 80 chars. Let's add a one-line intro followed by a blank line and put the rest in a following paragraph if needed.

2. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
   @@ -559,4 +560,24 @@ public function testEntityTypeSchemaUpdateAndBaseFieldCreateWithoutData() {
   +      $this->pass('Successfully updated entity schema and created revisionable base field at the same time.');
   ...
   +      $this->fail('Successfully updated entity schema and created revisionable base field at the same time.');
   

   Can we use a $message variable to avoid replicating the text?

Back to RTBC

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed d42d61a and pushed to 8.0.x. Thanks!