Problem/Motivation

The uid field on media entities is not set to required, but the database field is set to NOT NULL.

To reproduce:

- go to /media/add/image
- fill in the required form elements
- delete the contents of the Authored by form element
- save the form

The site crashes with a storage exception because the uid database field may not be NULL.

Steps to reproduce

Proposed resolution

Ideally, deal with #3260173: EntityOwnerTrait does not define the owner field as required, but the DB field is NOT NULL so it's fixed for all entities.

As an interim, copy the code from Node::preSave() to Media::preSave():

      // If no owner has been set explicitly, make the anonymous user the owner.
      if (!$translation->getOwner()) {
        $translation->setOwnerId(0);
      }
    }

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#17 core-3260175-17.patch2 KBnod_
#8 3260175-8.patch661 bytesjsricardo
#3 3260175-3.patch659 bytesandregp
#3 Saving_media_result.png99.51 KBandregp
#3 Saving_media.png97.55 KBandregp

Issue fork drupal-3260175

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

joachim created an issue. See original summary.

andregp’s picture

andregp commented
Assigned: Unassigned » andregp

I'll work on this issue.

andregp’s picture

andregp commented
Assigned: andregp » Unassigned
Status: Active » Needs review
StatusFileSize
new Saving_media.png97.55 KB
new Saving_media_result.png99.51 KB
new 3260175-3.patch659 bytes

Here is the patch and some prints of the result.

joachim’s picture

joachim commented
Status: Needs review » Needs work

I am wondering whether we can handle this at the field level, the same way that ChangedItem does this:

  public function preSave() {
    parent::preSave();

    // Set the timestamp to request time if it is not set.
    if (!$this->value) {
      $this->value = REQUEST_TIME;
    }

- check the target entity type
- if it's 'user' and the value is empty, set it to 0

andregp’s picture

andregp commented

I asked for help about this issue on Drupal #support channel on slack and @james.williams explained me about it. I tried to find a way to identify the field for the verification (inside EntityReferenceItem) but I couldn't. =/

I'm just posting the thread link here so others can consult it too https://drupal.slack.com/archives/C1BB308HH/p1643989304582169

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

jsricardo’s picture

jsricardo commented
Assigned: Unassigned » jsricardo

I Will Work on it

jsricardo’s picture

jsricardo commented
StatusFileSize
new 3260175-8.patch661 bytes

Hi!
I checked the dialog quoted in comment #5 by @andregp, however I couldn't implement something that solved it.
Otherwise, I followed the steps to reproduce as mentioned in the description of the issue and the upload of files without owner usually occurs by setting anonymous.

jsricardo’s picture

jsricardo commented
Assigned: jsricardo » Unassigned

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

joachim’s picture

joachim commented
Status: Needs work » Reviewed & tested by the community

Patch matches the way it's handled in node module, so LGTM.

I'll maybe open a follow-up for my idea in #4.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

This is the same behaviour we have for nodes so I think this makes sense. I considered the security implications for media being owned by user 0 and I think they are the same as nodes - ie. don't give the anonymous users permissions.

I think we should have test coverage of this.

joachim’s picture

joachim commented
Status: Needs work » Needs review
Issue tags: -Needs tests

Added test cribbed from NodeEditFormTest which tests the same thing.

(Functional tests really need a reloadEntity method BTW!)

joachim opened merge request !2920

joachim’s picture

joachim commented

NO idea why the MR is all crazy with commits -- locally the branch is 3 commits ahead of 10.1.x and that's up to date :/

joachim’s picture

joachim commented

Filed a follow-up: #3318564: entity owner field should handle default value at the field level.

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-3260175-17.patch2 KB

Patch that correspond to the code in the MR

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

If test pass all good for me

catch’s picture

catch
he/him
Primary language English
commented
Version: 10.1.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

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

  • catch committed 417a3e5 on 10.0.x 
    Issue #3260175 by jsricardo, andregp, joachim, nod_, alexpott: Saving...
  • catch committed fbac9e7 on 10.1.x
    Issue #3260175 by jsricardo, andregp, joachim, nod_, alexpott: Saving...
  • catch committed 99524ce on 9.4.x
    Issue #3260175 by jsricardo, andregp, joachim, nod_, alexpott: Saving...
  • catch committed 3baef95 on 9.5.x
    Issue #3260175 by jsricardo, andregp, joachim, nod_, alexpott: Saving...
joachim’s picture

joachim commented

Thanks everyone!

We now need a follow-up issue, to fix this more comprehensively for *all* entity types that use this trait -- see #4.

Status: Fixed » Closed (fixed)

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