If only indexes and keys need to be updated, that could be done without a data migration, so making this function detect that that's the only needed change would allow more cases of update.php to work instead of forcing people into a Migrate requirement.

CommentFileSizeAuthor
#7 exclude-index-only-changes-from-data-check-2340993-7.patch8.45 KBberdir
#5 exclude-index-only-changes-from-data-check-2340993-5.patch2.57 KBberdir
#2 exclude-index-only-changes-from-data-check-2340993-1.patch1.03 KBberdir

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Issue tags: +entity storage
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new exclude-index-only-changes-from-data-check-2340993-1.patch1.03 KB

Here's a first patch for this, tested with #2336895: Allow entity type and field storage definition objects to be compared for definition equality, works great there, didn't test yet if it has undesired side effects yet. Also not quite sure where the right place for this check is.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Wrong reference, I meant #2261669: Slow query in NodeRevisionAccessCheck.

Status: Needs review » Needs work

The last submitted patch, 2: exclude-index-only-changes-from-data-check-2340993-1.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new exclude-index-only-changes-from-data-check-2340993-5.patch2.57 KB

Nice, there's a test already, just have to update that because it works now :)

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Title: SqlContentEntityStorageSchema::requiresEntityDataMigration() returns TRUE for cases where it could return FALSE » SqlContentEntityStorageSchema::requiresEntityDataMigration() returns TRUE for cases where it should return FALSE
Related issues: +#2261669: Slow query in NodeRevisionAccessCheck 
+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -260,8 +260,12 @@ public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeI
+    $index_only_changes = $entity_type->isRevisionable() == $original->isRevisionable() &&

Instead of repeating the logic, shouldn't we fix ::requiresEntityDataMigration() to ignore index changes?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new exclude-index-only-changes-from-data-check-2340993-7.patch8.45 KB

We discussed this a bit in IRC, here's what I came up with. A new helper method for the first three conditions that we can use from both methods.

Interdiff is almost as big as the patch and not very useful I think.

yched’s picture

yched commented

Looks reasonable, but I'll leave it to @plach to RTBC

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed b43bf88 on 8.0.x 
    Issue #2340993 by Berdir: SqlContentEntityStorageSchema::...

Status: Fixed » Closed (fixed)

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