Needs work
Project:
Drupal core
Version:
main
Component:
entity system
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
7 Dec 2017 at 10:22 UTC
Updated:
28 Sep 2022 at 16:21 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
tstoecklerHere we go. Extracted this from #2928906: The field schema incorrectly stores serial fields as int. Interdiff is relative to #4 over there and should hopefully fix the tests. Could be that I messed something up in the split though...
Comment #4
mayurjadhav commentedComment #6
tstoecklerLooked at the test failures, but similarly to #2928906: The field schema incorrectly stores serial fields as int this requires #2938951: The primary key is not correctly re-created when updating the storage definition of an identifier field because we are updating the field storage definitions as part of the update, so we need to recreate the primary keys.
Comment #7
tstoecklerComment #8
tstoecklerOK, so here's a re-roll on top of #33 from #2938951: The primary key is not correctly re-created when updating the storage definition of an identifier field.
Once we get a green here, we can start tackling those todos from that issue that point here to prove that they are warranted.
Comment #9
tstoecklerAhh borked the re-roll here. This should be
$schemain both cases, and we should remove$entity_schemaaltogether here. Should in theory not affect green-ness, though. We'll see...Comment #11
tstoecklerThis fixes #9 and one other thing I borked up in the re-roll + bumps the update functions to 8.6
Comment #12
tstoecklerSorry, that's stupid. Will leave that out of the next one.
Comment #14
tstoecklerThis should fix at least some of the fails.
Of course, I ran into #2928906: The field schema incorrectly stores serial fields as int here. Adding more duct-tape for now in order to keep those separate.
Comment #15
tstoecklerD'uh, wrong extension. Here you go, little bot.
Comment #17
tstoecklerOK, more duct tape for #2928906: The field schema incorrectly stores serial fields as int
Comment #19
tstoecklerSo this revealed a bug, yay ;-): When we change the serial field to an int during an update of a field definition we never actually change it back. So we should do that. ...except not for user entities of course, as those do not use serial field at all. Hah, this is so much fun... ;-)
Unless I broke anything else - which is entirely possible - this should be green. Will then attempt to remove some of the hacks introduced by #2938951: The primary key is not correctly re-created when updating the storage definition of an identifier field.
Comment #20
tstoecklerLet's see.
Comment #21
tstoecklerNow testing my theory that the ugly workaround in
SqlContentEntityStorageSchema::processBaseTable()is only needed due to this.Comment #23
gogowitsch commentedComment #24
jibranBlocker is in #2938951: The primary key is not correctly re-created when updating the storage definition of an identifier field
Comment #25
tstoecklerHere's a re-roll. This still contains some cruft which can be removed, but let's see if this is green first.
Comment #27
tstoecklerSo the problem was that when we were merging field schemas, we were passing a schema array with multiple indexes to
addIndex(), but only with the fields that are needed for one of them. So we need to make sure that only a single index is part of the$index_schemaarray.Hope this will be green, will try to remove the primary key stuff if so, as I'm pretty sure that it's no longer needed.
Comment #29
tstoecklerMaybe this time?
Comment #30
jibranUpdated the title.
Comment #31
tstoecklerOK, this time with less cruft. The interdiff does not show the (re-)removal of (the previously introduced here)
AssertSchemaTraitas that is no longer needed because I accidentally removed it with PhpStorm 😡. But the patch should be significantly smaller now.Comment #32
tstoecklerAlright, this now covers all existing entity types. I guess we want to add a change record, but in terms of the patch this should be RTBC-able/reviewable now, at least from my perspective.
Comment #33
berdirI didn't look at the changes in-depth yet, but the fact that entity types like comment and node need their own custom update functions and EntityStorageSchema changes seems very problematic to me. That seems like an non-BC API change and even if we'd decide to allow/require that, it's really quite hard to deal with this in contributed modules.
Even if you add an update function, you need to ensure that it runs at the right time, meaning, it mustr only run after the 8.7 update (assuming it lands there), so modules need to add a version dependency as well as an update hook dependency on that system update function to ensure the necessary change actually exists, otherwise the updateFieldStorageDefinition() call wouldn't actually do anything.
Maybe we can somehow detect these kind of changes and call the update method automatically on all field storage definitions that need the update? But that is also really tricky, in general changes that are active automatically on an entity type and don't require any kind of configuration/definition change. Imagine that in my_awesome_module, I'm doing a change that requires a custom update function because I'm altering the schema and actually have data. If I'm updating to 8.7 and updating that module at the same time, hen the generic update would jump in and run and obviously fail because there is data. Making it extremely hard for my module to be update-able both before/after core.
Comment #41
larowlanDo we still want to do this folks?
Comment #42
ajaypratapsingh commentedtested on drupal 9.4.x
Comment #43
ajaypratapsingh commentedComment #45
immaculatexavier commented#42 Patch Failed to Apply, changing the status to Need Work.
Comment #46
vsujeetkumar commentedRe-roll patch created for 9.5.x, Please have a look.
Comment #47
vsujeetkumar commentedFixed custom command fail issues.