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
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff.txt | 600 bytes | chx |
#23 | 2527816_23.patch | 2.9 KB | chx |
#14 | interdiff.txt | 709 bytes | pfrenssen |
#14 | 2527816-14-test-only.patch | 1.69 KB | pfrenssen |
#14 | 2527816-14.patch | 2.89 KB | pfrenssen |
Comments
Comment #1
jhedstromThis 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.Comment #3
jhedstromThat refactoring was a bit much.
Comment #4
jhedstrom$query
is always set, no need to check.Comment #5
jhedstromComment #8
jhedstromAh, the tests depend on the distinct count. Adding that bit back.
Comment #9
BerdirThis 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 :)
Comment #10
catchThe $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.
Comment #11
dawehner.
Comment #12
pfrenssenPicking this up as part of the D8 Accelerate London Critical Midnight Sprint.
Comment #13
pfrenssenHere's the new approach proposed by @catch, and a test. Thanks to @berdir for the help!``
Comment #14
pfrenssenOops seems like a line went missing.
Comment #18
larowlantag
Comment #19
larowlanPasses locally, retesting as I've seen that fail on other issues today
Comment #21
larowlanAssuming fail is random, this looks ready to me
Comment #22
jhedstromI've confirmed the latest patch here resolves the issue as first discovered over in the head2head module. +1 RTBC.
Comment #23
chx CreditAttribution: chx commentedThe 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.
Comment #24
alexpottCommitted 49f5b15 and pushed to 8.0.x. Thanks!
Comment #26
plachlol, @chx++