Problem/Motivation

This is the comeback of #2527816: Logic error in SqlContentEntityStorage::countFieldData() attempts to drop `name` column. Turns out, we forgot to do the same fix as we did there for dedicated field tables above.

And in a way, it is worse because we *actually* lose data. If you have a record in there for uid 0 (which shouldn't really happen but for some case I do have it at least in one table), then it just drops the table and re-creates it and your content is gone.

Proposed resolution

Apply the same check to the query as we have for the shared tables.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#10 2559309-10-interdiff.txt1.18 KBberdir
#10 2559309-10.patch2.85 KBberdir
#7 2559309-07.patch2.62 KBjhedstrom
#7 2559309-07-TEST-ONLY.patch1.66 KBjhedstrom
#7 interdiff.txt1.66 KBjhedstrom
#2 count-field-data-dedicated-ooops-2559309-1.patch990 bytesberdir

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new count-field-data-dedicated-ooops-2559309-1.patch990 bytes
catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +beta target, +D8 upgrade path
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

Can we add a test similar to the one in #2527816: Logic error in SqlContentEntityStorage::countFieldData() attempts to drop `name` column?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes, that's exactly what we should do. Might get to do that in a few hours, will assign the issue to me then. If someone has time to look into this now, go for it!

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Assigned: Unassigned » jhedstrom

I'll work on the test.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Assigned: jhedstrom » Unassigned
Issue tags: -Needs tests
StatusFileSize
new interdiff.txt1.66 KB
new 2559309-07-TEST-ONLY.patch1.66 KB
new 2559309-07.patch2.62 KB

Here's a test.

The last submitted patch, 7: 2559309-07-TEST-ONLY.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/core/modules/field/src/Tests/FieldDataCountTest.php
@@ -146,6 +146,20 @@ public function testEntityCountAndHasData() {
+    $field_storage = entity_create('field_storage_config', array(
...
+    entity_create('field_config', array(

I think we spent quite some time dropping entity_create() calls, let's switch to FieldStorageConfig::create() and so on.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new 2559309-10.patch2.85 KB
new 2559309-10-interdiff.txt1.18 KB

Updated that.

plach’s picture

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

Looks good!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

And STAY fixed this time. ;)

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 7ea136e on 8.0.x 
    Issue #2559309 by Berdir, jhedstrom: Logic error in...

Status: Fixed » Closed (fixed)

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