EntityOwnerTrait does not define the owner field as required, but the DB field is NOT NULL

Thanks for opening the issue! Interesting, I would have thought that the default value callback that EntityOwnerTrait adds a value in every case. Are you doing anything particular to have the entity (attempt to) save without a value or am I just wrong about that?

Regardless, however, the underlying issue is valid, I think. A non-required field should not be NOT NULL in the DB. The underlying problem is that we make the assumption that every entity key should have a NOT NULL constraint. That is a problematic assumption for the reason stated here (among others) and is, theoretically, being fixed in #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field. So adding that as a related issue.

Since this is a concrete bug report, though, I think it makes sense to discuss this on its own merits. We already have an exception for the label and the revision translation affected keys to "opt" those "out" of the assumption mentioned above, this happens in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getSharedTableFieldSchema(). Adding the relevant snippet here, because I think the context is useful:

    // A shared table contains rows for entities where the field is empty
    // (since other fields stored in the same table might not be empty), thus
    // the only columns that can be 'not null' are those for required
    // properties of required fields. For now, we only hardcode 'not null' to a
    // few "entity keys", in order to keep their indexes optimized.
    // @todo Fix this in https://www.drupal.org/node/2841291.
    $not_null_keys = $this->entityType->getKeys();
    // Label and the 'revision_translation_affected' fields are not necessarily
    // required.
    unset($not_null_keys['label'], $not_null_keys['revision_translation_affected']);

So one thing we could do is just add the owner key to this list of exceptions and that would fix this concrete issue. Since for nodes the owner will in fact never be NULL, as is noted in the issue summary, this would be an issue if there are SQL queries that rely on the owner index (or node_field__uid__target_id index to be precise) on node_field_data, as those would suffer a performance decrease due to this. I am not sure, though, A) if there are any queries that use that index (from a cursory glance, nothing stuck out at me - at least nothing that would be run on a regular page - but I am a bit out of my depth here to be honest) and B) if there are any what the concrete performance implications on those queries are. I presume the index was not added "explicitly" either and just came "automatically" because the owner field is an entity reference field and that just adds an index automatically, but not sure...

I'll work on this

I've tried adding the owner key to this list of exceptions as mentioned on #3 but neither of the options bellow worked (you can see the errors on the images).

unset($not_null_keys['label'], $not_null_keys['revision_translation_affected'], $not_null_keys['owner']);

unset($not_null_keys['label'], $not_null_keys['revision_translation_affected'], $not_null_keys['uid']);

So I changed "Authored by" field to be required.

Fair enough @joachim. From the issue summary I had - apparently wrongly - assumed that you had a legitimate use-case for saving an entity with an empty owner. Absolutely fine with making it required since it effectively is required already. Does that mean, though, that we can remove the respective code in Node::preSave()?

Okay, sorry, I'll change it :)

So I made a patch with the fix on the right file and made a second patch that also removes the code mentioned on comment #7-#8 in case you want to commit this too.

Now the patch is failing because of the NodeEditFormTest::checkVariousAuthoredByValues test.
In the code bellow (line 274-285) the test tries to make the "authored by" field empty.

    // Change the authored by field to an empty string, which should assign
    // authorship to the anonymous user (uid 0).
    $edit[$form_element_name] = '';
    $this->drupalGet('node/' . $node->id() . '/edit');
    $this->submitForm($edit, 'Save');
    $this->nodeStorage->resetCache([$node->id()]);
    $node = $this->nodeStorage->load($node->id());
    $uid = $node->getOwnerId();
    // Most SQL database drivers stringify fetches but entities are not
    // necessarily stored in a SQL database. At the same time, NULL/FALSE/""
    // won't do.
    $this->assertTrue($uid === 0 || $uid === '0', 'Node authored by anonymous user.');

@joachim @tstoeckler What do you believe is the best approach then? Should we change/remove this part of the test or try another solution?

So I changed the preSave() code in the Media entity to keep consistent with the Node entity. Now you can save the media entities with no Author and it will default to Anonymous.

Is there any other user reference fields I am missing?

Oh sorry, I may have got confused.

Re-upload of patch #6
It seems to be the best approach for this issue since changing the EntityOwnerTrait.php to have Authoring required triggers the NodeEditFormTest::checkVariousAuthoredByValues test, which tests if the entity receives a Anonymous author when its author is omitted.

The failed test was layout_builder\FunctionalJavascript\ContentPreviewToggleTest::testContentPreviewToggle. Seems unrelated to this issue so sending back to needs review.

Not sure the patch is addressing the issue title directly.

Patch prevents drupal from crashing when a media entity doesn't have an author assigned

Committed/pushed to 10.1.x, cherry-picked back through to 9.4.x, thanks!

The commit isn't for the wrong issue, it was patch #20 from this issue, which was based on patch #6, which was RTBCed by @joachim in #18 and again by nod_ in #25. However it was committed with the wrong title, and I think the commit on #3260175: Saving media entity without an owner crashes brings things into line with Node, if not for other modules using the trait.

I think there's a potential field-level fix so that node and media don't have to have the code from #3260175: Saving media entity without an owner crashes, but that felt like follow-up material to me, possibly it should be a new issue and we should just close this one?

That's #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field

SqlContentEntityStorageSchema::getSharedTableFieldSchema() is where it happens.