Add a SQL index for entity types that are using EntityPublishedInterface

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -931,6 +932,13 @@ protected function initializeDataTable(ContentEntityTypeInterface $entity_type)
+    if (is_subclass_of($entity_type->getClass(), EntityPublishedInterface::class)) {

I think we need to check if the field storage is not a custom storage. I don't think the field is necessarily stored in the entity table. No?

Let's see a different approach. I think the addition should be in the shared table schema.

Also this needs:

• Tests.
• Upgrade path for fields already added through "published" key.
• Upgrade path test.

Oh yes, true, it belongs to entity schema because it's a composite index. But the table should be determined. Not every time is in data table. If an entity lacks the annotation "data_table" key, the published field is placed in the base table.

Added upgrade path and upgrade path tests.

Here we go.

While working on this I discovered this #2834445: Don't fetch the entity type ID on each iteration in SqlContentEntityStorageSchema::getFieldSchemaData().

Improved the update.

Sorry, the patch was wrong.

Looks good to me.

Only minor query would be if the update hook should be specifically for the Comment entity type and reside in comment.install? The change will affect any entity type implementing the EntityPublishedInterface which is only Node and Comment, Node already has the index, so only Comment needs the update. There is a small chance that contrib modules could be implementing the interface, therefore they'd need the update, but the interface was only added in 8.3.x so there shouldn't be any contrib modules that implement it yet.

@timmillwood, I intentionally updated ALL that are implementing the interface. The interface is in for a while so we could expect custom entities implementing it.

UPDATE: You're right, it's only in 8.3.x but this covers all cases.

@claudiu.cristea - ok, I guess it's safer to update everything. I think it would feel tidier to put the update in comment.install just for comments. However, I agree it's safer to update all, just in case.

+++ b/core/modules/system/system.install
@@ -1794,3 +1797,37 @@ function system_update_8203() {
+
+    // Update only if the 'published' field uses the main storage.
+    $field_definition = $entity_update_manager->getFieldStorageDefinition($entity_type->getKey('published'), $entity_type_id);
+    if (!$field_definition->hasCustomStorage()) {
+      // Regenerate entity type indexes, this should add the missed indexes.
+      $entity_update_manager->updateEntityType($entity_type);
+    }
+  }

Using updateEntityType() means that any schema change to any entity that's pending when this update runs will be applied here.

Do we need a way to only update indexes? Or explicitly to add an index?

Tthe only thing we really can do is get the entity type definition from the definition update manager. Then it will not have any definition changes and we at least only update implicit changes that happened due to code changes.

I wouldn't know how to have a proper API to address a certain index only, that's not something that is part of any API, just the generated schema definition.

@catch, @Berdir

Using updateEntityType() means that any schema change to any entity that's pending when this update runs will be applied here.

Why to any entity? We pass a specific $entity_type to updateEntityType(), still don't get why other entity types are updated.

Not any entity type. Any change to a given entity type.

If I'm going to change my entity type and add another key, a different storage schema class or anything that affects the generated schema, then your call will pick that up too and try to adjust the schema for it as well. As you can imagine, that can easily lead to conflicts if we end up adding something that a second update will then want to add as well. Update functions always need to be as predictable and contained as possible.

That's why you need to use the stored entity_type that we use to compare changes and update it with that. See node_update_8003() for an example (in your case, we don't actually make a change to the entity type, so you don't need to change/set back)

RTBCing again now that the entity type update concerns seem to have been addressed.

(18:10:12) berdir: claudiu_cristea: so your test should actually pass, even without your update function?
(18:10:35) claudiu_cristea: berdir: no
(18:11:05) claudiu_cristea: berdir: We need to update the comment entity schema

Yes, it does. We already *are* updating the comment entity type and now that the new code is there, we get the new index and create it.

As you said yourself, we are only in alpha and this interface is new. The published key is also new, there is no reason why someone should already be using it. So anyone who adds the entity key and implements that interface gets the new index in the update function that he has to write anyway.

Cleaning up.

I think having the test is actaually useful, otherwise we don't really know if the index was added? We could also add an assert to the exisisting commen test, that would be fine too I think.

I used the test for comment_update_8301().

Finally is green after several unrelated, random test failures. I used the test of comment_update_8301() to do the assertion.

Kinda awesome, and worrying, that the previous update hook does the update we needed.

As the comment update hook is for 8.3.x this will only cause issues for those using 8.3.x, and as it's alpha then they do it at their own risk.

Looks "Ready To Be Committed"!

Kinda awesome, and worrying, that the previous update hook does the update we needed.

Yep. migrate++ really.

Committed/pushed to 8.3.x, thanks!

Reverting this for now, no hard evidence but it coincided with a big increase in upgrade path testing random failures, so mostly trial-and-error rollback.

This did not help, unfortunately: https://www.drupal.org/pift-ci-job/556437.

Keep in mind the fail already existed -- just the frequency of it was new. So we might still expect a fail even if we revert something exacerbating it. But if the frequency still doesn't go down over the next day then we should probably recommit this.

only in patch2:
unchanged:
--- a/core/modules/comment/src/Tests/Update/CommentUpdateTest.php
+++ b/core/modules/comment/src/Tests/Update/CommentUpdateTest.php
@@ -66,6 +66,9 @@ public function testPublishedEntityKey() {
// Check that the entity key exists and it has the correct value.
$entity_type = \Drupal::entityDefinitionUpdateManager()->getEntityType('comment');
$this->assertEqual('status', $entity_type->getKey('published'));
+
+ // Check that the {comment_field_data} table status index has been created.
+ $this->assertTrue(\Drupal::database()->schema()->indexExists('comment_field_data', 'comment__status_comment_type'));
}

}

only in patch2:
unchanged:
--- a/core/modules/comment/src/Tests/Update/CommentUpdateTest.php
+++ b/core/modules/comment/src/Tests/Update/CommentUpdateTest.php
@@ -66,6 +66,9 @@ public function testPublishedEntityKey() {
// Check that the entity key exists and it has the correct value.
$entity_type = \Drupal::entityDefinitionUpdateManager()->getEntityType('comment');
$this->assertEqual('status', $entity_type->getKey('published'));
+
+ // Check that the {comment_field_data} table status index has been created.
+ $this->assertTrue(\Drupal::database()->schema()->indexExists('comment_field_data', 'comment__status_comment_type'));
}

}

Now, after more than 48 hours, we are sure that this issue is not the cause of #2828559: UpdatePathTestBase tests randomly failing. I think is safe to commit this back.

Yep, I've reverted the revert. Back to fixed.

Yep, I've reverted the revert

@catch, now because of this revert of revert I was not credited with this issue to http://www.drupalcores.com. They are subtracting the "Revert *" commits https://github.com/lauriii/drupalcores/blob/master/app/bin/json.rb#L16 :)

now because of this revert of revert I was not credited with this issue to http://www.drupalcores.com.

That should be fixed upstream. I think https://www.drupal.org/u/claudiu.cristea/issue-credits/3060 is way better than drupalcores now.

reverted:
--- a/core/modules/comment/src/Tests/Update/CommentUpdateTest.php
+++ b/core/modules/comment/src/Tests/Update/CommentUpdateTest.php
@@ -66,6 +66,9 @@ public function testPublishedEntityKey() {
// Check that the entity key exists and it has the correct value.
$entity_type = \Drupal::entityDefinitionUpdateManager()->getEntityType('comment');
$this->assertEqual('status', $entity_type->getKey('published'));
-
- // Check that the {comment_field_data} table status index has been created.
- $this->assertTrue(\Drupal::database()->schema()->indexExists('comment_field_data', 'comment__status_comment_type'));
}

}

reverted:
--- a/core/modules/comment/src/Tests/Update/CommentUpdateTest.php
+++ b/core/modules/comment/src/Tests/Update/CommentUpdateTest.php
@@ -66,6 +66,9 @@ public function testPublishedEntityKey() {
// Check that the entity key exists and it has the correct value.
$entity_type = \Drupal::entityDefinitionUpdateManager()->getEntityType('comment');
$this->assertEqual('status', $entity_type->getKey('published'));
-
- // Check that the {comment_field_data} table status index has been created.
- $this->assertTrue(\Drupal::database()->schema()->indexExists('comment_field_data', 'comment__status_comment_type'));
}

}

OK, opened https://github.com/lauriii/drupalcores/pull/95 upstream.

only in patch2:
unchanged:
--- a/core/modules/comment/src/Tests/Update/CommentUpdateTest.php
+++ b/core/modules/comment/src/Tests/Update/CommentUpdateTest.php
@@ -66,6 +66,9 @@ public function testPublishedEntityKey() {
// Check that the entity key exists and it has the correct value.
$entity_type = \Drupal::entityDefinitionUpdateManager()->getEntityType('comment');
$this->assertEqual('status', $entity_type->getKey('published'));
+
+ // Check that the {comment_field_data} table status index has been created.
+ $this->assertTrue(\Drupal::database()->schema()->indexExists('comment_field_data', 'comment__status_comment_type'));
}

}

See #2849150: The Status for publishing field needs to be updated. if you are running into problems locally due to alpha-to-alpha upgrade problems as foreseen in #35.