The primary key is not correctly re-created when updating the storage definition of an identifier field

Yay, thanks for taking a look at this. I assume this means you are not opposed to tackling this in a separate issue, which is great news!

1.

See #2488258-89: [ignore] Patch testing issue where I checked whether splitting this is feasible. What happens is that various ContactStorageTests install an ID field to the contact message entity which generally doesn't have an ID and, thus, also do not have an id entity key set. And since we explicitly use the last installed entity type during the update calling $entity_type->getKey('id') when building the schema fails.

Note that the patch I linked to there still has the "drop-primary-key" part which I thought was necessary but turns out not to be necessary after all. I don't think that's related to this, though. I can try out another patch with that removed and the hunk you mentioned removed, as well, if you want to be 100% sure.

2.

Sure, that makes total sense. Will tackle that in my next round here (but could be a while, maybe on the weekend...).

3.

Good point! My original intention for the text was to not to uninstall and re-install the field storage definition explicitly but to update it (which then ends up being the same thing, if there's no data, as you know) and then I would have needed some way to assert that the update actually happened, which I wanted to do by checking if the column type was changed from serial to varchar. That turned to be both rather complex and (I think ?!) unnecessary, which is why I then wrote the test as it is now.

So unless I'm missing something else, you're absolutely right, and we should switch (back) to the standard integer ID field.

Thanks for the awesome review.

Re #5: Awesome! ;-)

Regarding the contact stuff: The tests themselves arent't actually doing anything, it's actually contact_storage_test_entity_base_field_info() that's adding the field. And surely enough contact_storage_test_entity_type_alter() does add the ID key (I guess that should be done in *_build(), not *_alter() - strictly speaking - but ignoring that for now). The problem is that we use the last installed installed entity type at this point and that doesn't have that key - and rightly so. In any case, will upload a patch without that to see who's right ;-)

OK, here we go. This fixes 2. and 3. and I am providing a patch to (hopefully) prove that the hunk mentioned in 1. is actually necessary.

Hah, thanks for asking for that test coverage. The test was green for me locally, but before creating the patch I noticed I had the "drop-primary-key" part still applied (which I then removed before creating the patch). So now I know why I introduced that in the first place! We now have dedicated test coverage proving that it - or at least something like it - is necessary. Yay! That said, feel free to come up with something less ugly to make the tests pass. I have to admit that I don't fully grok that part of the problem, including why the hunk fixes it, off the top of my head. I know that I spent quite some time debugging that, but I don't remember the exact details.

Also, the additional contact storage failures showed up for the first patch in #8 like I expected proving that hunk to be necessary.

So here's a patch with the test coverage from #8 and the FieldStorageDefinitionListener hunk + the "drop-primary-key" hunk that I cooked up somewhere in the depth of #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field.

Oops:
X-post with #7 and #11.

Re #7: Thanks. This is only copied, but I do think it makes sense to fix these things, since we are effectively introducing new code here. Will fix in the next iteration

Re #11: Can you elaborate on what you think the fix should be? I tried to explain in #6 why I think the fix is correct, so as of now, I in fact do not agree ;-) There's a good chance you're right (in fact in this area of the code I'd bet on you rather than me, if I had to put money on it ;-)), but I don't really understand what you're getting at specifically.

Wow, didn't know that you could implement hook_module_preinstall() for your own module, very nice.

I'm actually not sure I agree with you conceptually. For reasons that are unknown to me we have chosen to handle creation of module-provided fields automatically in ModuleInstaller. So if we have made that "just work" for normal fields, I think it should also "just work" for ID fields. So while introducing knowledge about entity keys to FieldStorageDefinitionListener is not super nice, it's "fairer" IMO than making Contact Storage Test account for this on its own. On the other hand, I find it rather bizarre that we support entity types without an ID key in the first place, so I'm not too excited about the whole Contact Storage Test thing in the first place. At the end of the day, I don't really feel strongly either way. So I'm fine with this, assuming it's green.

Awesome. So @amateescu do you have any thoughts on the "drop-primary-key" part? Other than that I think this should be ready to fly.

Thanks for the input!!!

Will explain a bit more later, but right now just pressing Save before my battery dies...

OK, here we go.

I agree totally with your assessment that we should test and fix both cases. Thanks for pointing that out. In all the mess of these different issues, I had completely lost track of that not-so-minor detail.

In the test I explicitly used the explicit ->delete*() and ->install*() calls to be able to test that the primary key is gone in between. When calling just ->update*() we can only assert that the primary keys are there after the update and we have to assume that they are there again after having been deleted. At least I couldn't think of any way to assert that.

But, you are 100% correct, that we should test that, so I amended the test to also test the ->update* code path now. The first interdiff then only introduces that without adding any data, because that actually revealed that the test code was using an incorrect field name for the revision ID field. Therefore I also split that out into a separate interdiff (as I didn't know at the time whether the "combined" patch would be green).

Then I added test code that saves an entity and then performs the update again. Surely enough, I hit #2928906: The field schema incorrectly stores serial fields as int so I had to manully add entity IDs and revision IDs. I'd really like to not re-merge those, though. And like you anticipated I hit #2929120: The stored field schema does not contain field-level primary keys, unique keys and indexes again. I worked around the dependency, though, similarly to what I did here and in #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field. Once this is RTBC I will tackle #2929120: The stored field schema does not contain field-level primary keys, unique keys and indexes next, though, so hopefully those workarounds won't be that long-lived. By the way, can I take your mention of that issue as a sign that you generally agree with the direction of that issue? That would be rather sweet.

So yeah. I'm pretty happy that the patch is green, so for me personally this would RTBC material, as it does fix a nasty bug (proven by the tests), but I won't fight you if you would like me to merge one of the other issues back in.

So, yes, I agree with doing this as a first step.

That is great news! I do still feel that with all these issues I am treading in less shallow waters than I am generally comfortable with, so your validation really of the issues' direction and how they play together does mean a lot. Yay!

Fixed the comments in #19.

Interdiffs are hard.

Ahhhhh. No, don't test it. OMG, this is embarassing...

Wanted to retest it anyway...

Re #27: Can you take a look at #13 and #14? I think that @amateescu's point would be that contact_storage currently alters the entity type incorrectly which is exposed by this issue and it should be updated in the same way that contact_storage_test is in this patch.

Alternatively, like I argue in #14 we could consider it a bug that it's currently impossible to provide an ID field for a different entity type without the workaround employed in #13. Reverse-applying the interdiff in #13 and then also #2476637: Remove obsolete setting of provider on the message ID field the Contact Storage test passes.

Like I said before, I don't feel strongly either way, because all of this is pretty weird to me. If we didn't have to account for BC, I would be arguing that having an entity type without an ID field is a bug in the first place, but that's not possible at this point anymore...

OK, here's a proposal: This is fully backwards-compatible (as far as I know, at least Contact Storage's (contrib) and Contact Storage Test's (core) tests pass with this) and it also doesn't introduce knowledge about ID keys to FieldStorageDefinitionListener. It's not necessarily pretty; it adds a workaround for the symptom of this problem in SqlContentEntityStorageSchema - along with a lengthy comment. Please feel free to shorten/improve the comment.

Thoughts?

Ha, since this workaround is only needed in getEntitySchema() (and not when building the field schema), I think we should actually be able to remove this workaround in #2929120: The stored field schema does not contain field-level primary keys, unique keys and indexes as we then shouldn't be needing to build the entity schema during a field update/install/delete. If that is the case, I think that's a pretty strong case for #30. I will verify that - assuming #30 is green - the assumption is correct by working on a patch for #2929120: The stored field schema does not contain field-level primary keys, unique keys and indexes that builds on #30 here.

OK, that's encouraging. Fixed the unit test and added a @todo about removing the workaround per #31. Will try out if that's actually true.

OK, so #21 over there proves my theory: Because processIdentifierSchema() is only called to build the entity schema, not the field schema and that issue alleviates the need for the entity schema in various places, we no longer need that workaround. That's encouraging...

...except I just realized it's actually not encouraging at all because the fact that processIdentifierSchema() is only called to build the entity schema is a bug in its own right, which is fixed in #2928906: The field schema incorrectly stores serial fields as int. So that will force us to keep that workaround around.

So yeah, would love some more input by @amateescu and @alexpott at this point. I'm fine to go with whatever approach you two can agree on ;-)

my understanding is that the approach in the last two patches would just work until we tackle the next issue in the long chain/pool of storage issues

No, actually it will work fine. I just thought that we can remove the ugly workaround in ::processIdentifierSchema() when we tackle the next set of issues, but then I realized that that's not true, so presumably we have to keep this around forever.

So maybe @amateescu can comment on the latest patch. As far as I can tell both @alexpott and @Berdir agree with the approach in the latest patch because it is more backwards-compatible.

Here's a re-roll for #2958429: Properly deprecate SafeMarkup::format().

Thanks for working on this, it's great to not only have some eyes but also some helping hands as this issue is still a fair bit above my paygrade... ;-) I have to admit that I don't remember all the details that brought me to write the patch in the way that I did, but ...

On the other hand, if you are updating one of those three fields, that means you are un-installing and re-installing them, which should work properly.

... I'm fairly sure that the reason I introduced those primary key shenannigans was that there was a test failure in exactly that code path: There was an SQL exception when updating a field about an illegal primary key. It's absolutely possible, however, that "that means we have to delete and recreate the primary key" was an incorrect conclusion from that.

In fact, the tests seem to be on your side as there is no such SQL exception among the fails.

The only fail is the contact issue which seems to be exactly what was discussed above and seems to be distinct from the primary key issue. So hopefully we can get that issue resolved in some way as well and then this will finally be ready.

Thanks for your persistence on removing that code!!!

Crosspost with #43: Awesome that you agree with that. Yeah sure, let's remove that comment. I think I still had "No ID key is a bug" in my mind when I wrote that, and while that's still my theoretical opinion, it's not actually reality unfortunately ;-)

Yay, looks perfect! Thanks - again - a lot!! This certainly has my blessings but I guess I can't RTBC - even though most of the code that I contributed on top of your original code just got ripped out again ;-)

Hmm... with #45 and the patch from #2871036: [PP-1] Fix Content Moderation and Workspace tests and mark the 'workflow', 'moderation_state' and 'workspace' base fields as required again I now get the error again that I was talking about in #44:

SQLSTATE[42000]: Syntax error or access violation: 1072 Key column 'id' doesn't exist in table: ALTER TABLE {block_content_field_data} DROP `id`; Array ( )

This is in the update tests, when the ID field of custom blocks is being updated by being deleted and then re-added again. So I guess there is still some weirdness that at the very least I haven't grokked.

However, since the patch in its current form allows 2 of 3 issues it blocks to proceed and comes with some nice test coverage I would still endorse getting this in as first step to unblock further progress in this area.

OK, finally pinpointed the issue in #47 to #2974722: Fix dropping of columns in a composite primary key on MariaDB >=10.2.8 and make it consistent across database drivers. So we're good here.

Actually going ahead and marking this RTBC now. Pretty much the only code I actually wrote here is the test coverage and that has been approved by @amateescu above.

#2974722: Fix dropping of columns in a composite primary key on MariaDB >=10.2.8 and make it consistent across database drivers revealed that we should add more assertions about the primary keys after fields have been removed. Will work on that.

Well it was valid before #40 because we were explicitly dropping the primary key. But instead of dropping the assertion completely we should add an assertion that actually asserts the correct primary keys after removing a field.

OK, this brings back the assertions. This is green locally together with the patch I'm about to upload in #2974722: Fix dropping of columns in a composite primary key on MariaDB >=10.2.8 and make it consistent across database drivers. So I can only assume this will be green on Drupal.org.

This also adds some sorting to AssertSchemaTrait::assertPrimaryKeyColumns() to (hopefully) make the tests pass on PostgreSQL and SQLite.

I now think, though, that we may want to postpone this on #2881522: Add a Schema::findPrimaryKeyColumns method to remove database specific logic from test as it will alleviate the need for introducing AssertSchemaTrait in the first place.

So apparently PostgreSQL handles the primary keys differently: it just drops them. Here's a patch which puts a compatbility layer in place to recreate them.

Notes:

1. I'm pretty sure we don't want to fix issues in the database layer here. Not sure if we want a separate issue or if I should roll that into #2974722: Fix dropping of columns in a composite primary key on MariaDB >=10.2.8 and make it consistent across database drivers. In any case we will probably want dedicated test coverage and we also probably want to wait for #2881522: Add a Schema::findPrimaryKeyColumns method to remove database specific logic from test with that one, if only for the test coverage.
2. I think the MySQL behavior is actually the odd one here. It's not at all guaranteed that changing the primary key constraint to apply to a subset of columns leaves the table in a valid state. We currently don't support that, but if I have two nodes with primary keys (1, en) and (2, en) and I want to recreate the ID field then the primary key will just be on langcode in the meantime and will be broken. So I think dropping the key (like PostgreSQL does automatically and MariaDB requires you to do manually) is actually more sensible. So maybe we should adapt the MySQL driver to drop the key (and not recreate it) instead?

Ahem.

Will re-roll this, after I am done with #2974722: Fix dropping of columns in a composite primary key on MariaDB >=10.2.8 and make it consistent across database drivers and officially marking postponed on that.

I really tried hard to keep this unpostponed, but my luck failed me this time... ;-)

Here's an interdiff and review-patch to update this patch for patch #15 in #2974722: Fix dropping of columns in a composite primary key on MariaDB >=10.2.8 and make it consistent across database drivers. Not rolling full + tests-only patch yet, until we have expected red/green over there.

Alright, it took a couple more rounds over there than I had hoped, but I think we're pretty close to an RTBC over there so doing a full patch-set here now. Because we changed the approach with regards to primary key in accordance with #53.2, I had to update EntitySchemaTest::testPrimaryKeyUpdate() that is being introduced here.

Will send for a run on all three database drivers just to make sure, but the driver-specific stuff should no longer be relevant here, so we should be seeing the same three fails on all three drivers in the test-only patch. The test also passes locally on MariaDB >10.2.8.

Here's a re-roll for the change in the test to account for findPrimaryKeyColumns() being protected now. Thanks everyone for the awesome recent progress!

(Only sending the tests-only patch for a test on MySQL, as there really isn't anything driver-specific here anymore. The full patch I'm sending for all three runs just to be extra-extra sure not to break anything even though we've consistently verified above that it really shouldn't be necessary, strictly speaking.)

No, actually missed that, thanks for pointing it out.

Well, I feel pretty strongly, that we should add those assertions, as they were in fact necessary to trigger all the bugs/inconsistencies over in #2974722: Fix dropping of columns in a composite primary key on MariaDB >=10.2.8 and make it consistent across database drivers. Those are now explicitly tested with the tests over there, but it still feels unnecessarily incomplete. While you are correct that there are no practical use-cases that will result in e.g. a data table without a primary key and without an ID field, we are still running that code so I see no reason why we shouldn't test that it has the expected side effects - especially when it's quite trivial to do so.

So, escalating this to the Framework Manager level. The concrete question here is:

The current patch includes test coverage for the primary keys at the "in-between" state of updating a field storage definition after it has been deleted and before it has been recreated. This state will never be an actual "end" state after an update, as e.g. there will never be a data table if there is no ID field. @amateescu finds testing for this behavior incorrect precisely because it does not reflect reality and @tstoeckler finds it correct to test this for completeness.

(@amateescu feel free to clarify, of course, in case the statement above does not represent your views accurately)

Alright, I still disagree with this, but I would really love to get this in at some point. So here's an interdiff realtive to #61 to remove the test assertions that @amateescu finds problematic plus the resulting patch. That way the patch that @amateescu would like to see go in is the last one, so that it can be RTBCed. If the committers instead want to side with me I will re-upload #61.

Re #67: Very good question indeed! git blame shows that this was introduced in #2398689: Follow-up: Applying entity schema updates still fails when both field and entity type definitions changes. It can only happen when using EntityDefinitionUpdateManager::applyUpdates() to e.g. make an entity type revisionable because in that case when SqlContentEntityStorageSchema::onEntityTypeUpdate() re-creates the entity type it already uses the "live" field storage definitions so that it will already include e.g. the revision ID field when it creates the shared table schema. Then when SqlContentEntityStorageSchema::onFieldStorageDefinitionCreate() wants to create the revision ID field, the schema already exists in the database. In this case, the entity schema will already include the primary key, though, so in terms of this issue we are good, I guess.

If we do get around to removing EntityDefinitionUpdateManager::applyUpdates() we might be able to remove this again, although over in #2976035: Entity type CRUD operations must use the last installed entity type and field storage definitions we found out that we in fact will need to add the capability to add a field storage definition and update the entity type "in one go", namely when adding a field that is an entity key (or revision metadata key). @amateescu: if you're latest patches in this area are any indication I would bet that you probably will find a way to remove this ugliness in the end ;-)

Alright, so if I understood the proposal correctly, then this is what we are talking about? If so, then this is fine by me, sounds like a fair compromise.

Yay, thanks so much!! This is great, and unblocks lots of progress elsewhere.