Problem/Motivation

I thought we had that covered, but apparently there are cases where previously, the uid key on some entity types can be NULL and that fails because adding an entity key tries to make those columns NOT NULL.

Proposed resolution

Update all NULL media item uid's to be 0.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 media-update-null-3040746-5.patch9.06 KBberdir
#5 media-update-null-3040746-5-test-only.patch7.72 KBberdir
#2 media-update-null-3040746-2.patch996 bytesberdir

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new media-update-null-3040746-2.patch996 bytes

Ah, I see, we only did that for file entities in \Drupal\file\FileStorageSchema::getSharedTableFieldSchema().

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: +8.7.0 beta testing
amateescu’s picture

amateescu commented

We need to check whether the entity type uses core's default SQL storage class :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new media-update-null-3040746-5-test-only.patch7.72 KB
new media-update-null-3040746-5.patch9.06 KB

Added the check and media example data, which makes the existing update path test fail.

To create that, I've switched to 8.4, imported the dumped data, created two media entities, manually set the uid for the first to NULL, then switched back to 8.8 and exported the data for all media and. I updated #2544972: Add options to limit tables, exclude data, or only export data to DbDumpCommand with a data-only option for that.

Status: Needs review » Needs work

The last submitted patch, 5: media-update-null-3040746-5.patch, failed testing. View results

sam152’s picture

sam152 commented
Issue summary: View changes

Fix and fixture looks good. The fail is unrelated, but once that's resolved we can get a green run.

My only query was, is there a difference between the owner of a media item being NULL and being owned by 0? For the purposes of access control, it seems like the prevailing logic is: anonymous users can't be the owners of anything:

$is_owner = ($account->id() && $account->id() === $entity->getOwnerId());

The first truth-y check for $account->id() would prevent that. So +1 RTBC.

sam152’s picture

sam152 commented
Status: Needs work » Needs review
amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community

Agreed with @Sam152, I think this looks ready to go! I retested the full patch from #5 and it didn't bump into the random fail again :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

I think this is a great stopgap fix until something like #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field lands. And it's nice to catch and fix update bugs before the beta! Great work everyone.

Committed and pushed 91dc88316b to 8.8.x and 289a17fc2c to 8.7.x. Thanks!

  • alexpott committed 91dc883 on 8.8.x 
    Issue #3040746 by Berdir, amateescu, Sam152: Update functions that set...

  • alexpott committed 289a17f on 8.7.x 
    Issue #3040746 by Berdir, amateescu, Sam152: Update functions that set...
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Issue tags: +8.7.0 alpha testing

Status: Fixed » Closed (fixed)

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