Problem/Motivation

The users_field_data table contains a row with uid set to 0. The logic in SqlContentEntityStorage::countFieldData() does a select query looking for a single, distinct uid. Since the first uid in the table is 0, the method always returns false, indicating there is no data in the table.

The query it runs (for a detected change in the name field, for instance):

MariaDB [d8]> SELECT DISTINCT t.uid AS uid FROM users_field_data t WHERE ((name IS NOT NULL)) LIMIT 1 OFFSET 0;
+-----+
| uid |
+-----+
|   0 |
+-----+
1 row in set (0.00 sec)

It then returns as follows:

    return $as_bool ? (bool) $count : (int) $count;

Since the SqlContentEntityStorageSchema::updateSharedTableSchema() method will simply drop and recreate a field without data:

       // Since there is no data we may be switching from a dedicated table
        // to a schema table schema, hence we should use the proper API.
        $this->performFieldSchemaOperation('delete', $original);
        $this->performFieldSchemaOperation('create', $storage_definition);

The resulting error when the 'name' column is attempted to be dropped:

Duplicate entry 'en' for key 'user__name'

Proposed resolution

Do a proper count query, or add special handling for uid=0.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Title: Logic error in SqlContentEntityStorage::countFieldData() for users_field_data table » Logic error in SqlContentEntityStorage::countFieldData() attempts to drop `name` column
Priority: Normal » Major
Status: Active » Needs review
Issue tags: +D8 upgrade path
FileSize
1.6 KB

This changes the query to be a simple count-query.

Tagging as upgrade path and bumping to major since this would prevent any update hooks from being run if the users_field_data table has schema changes.

Status: Needs review » Needs work

The last submitted patch, 1: count-query-2527816-01.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
1.17 KB
1.8 KB

That refactoring was a bit much.

jhedstrom’s picture

FileSize
737 bytes
1.8 KB

$query is always set, no need to check.

jhedstrom’s picture

Issue summary: View changes

The last submitted patch, 3: count-query-2527816-03.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: count-query-2527816-04.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
1.56 KB
938 bytes

Ah, the tests depend on the distinct count. Adding that bit back.

Berdir’s picture

Priority: Major » Critical

This results in losing data during the upgrade path. If this is not critical then I don't know what is.

A critical I worked on was just RTBC'd, so I'm allowed to set another one to critical :)

catch’s picture

The $as_bool was likely intended to avoid slower queries on large data sets, so not sure about using COUNT(*) here. Although generally this doesn't run much.

Could we rewrite the query in that case to be like this though? Don't need the field value in that case anyway.

SELECT 1 FROM users_field_data t WHERE ((name IS NOT NULL)) LIMIT 1 OFFSET 0;
dawehner’s picture

Issue tags: +Needs tests

.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
Issue tags: +D8 Accelerate London

Picking this up as part of the D8 Accelerate London Critical Midnight Sprint.

pfrenssen’s picture

Issue tags: -Needs tests
FileSize
1.69 KB
2.85 KB

Here's the new approach proposed by @catch, and a test. Thanks to @berdir for the help!``

pfrenssen’s picture

FileSize
2.89 KB
1.69 KB
709 bytes

Oops seems like a line went missing.

The last submitted patch, 13: 2527816-13-test-only.patch, failed testing.

The last submitted patch, 14: 2527816-14.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 14: 2527816-14-test-only.patch, failed testing.

larowlan’s picture

Assigned: pfrenssen » larowlan

tag

larowlan’s picture

Assigned: larowlan » Unassigned

Passes locally, retesting as I've seen that fail on other issues today

larowlan queued 14: 2527816-14.patch for re-testing.

larowlan’s picture

Status: Needs work » Reviewed & tested by the community

Assuming fail is random, this looks ready to me

jhedstrom’s picture

I've confirmed the latest patch here resolves the issue as first discovered over in the head2head module. +1 RTBC.

chx’s picture

FileSize
2.9 KB
600 bytes

The patch is correct but allow the guy who runs phpwtf.org to introduce a miniscule change to the test. This is the place to be strict about your booleans.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 49f5b15 and pushed to 8.0.x. Thanks!

  • alexpott committed 49f5b15 on 8.0.x
    Issue #2527816 by jhedstrom, pfrenssen, chx, catch: Logic error in...
plach’s picture

This is the place to be strict about your booleans.

lol, @chx++

Status: Fixed » Closed (fixed)

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