Uploading to root directory doesn't work correctly - active directory value is taken from the wrong place - we need to pass -1, but the method getDirectory() returns value NULL which is converted to 0 and the uploaded files don't show up anymore.

I think the juggling between -1 and NULL is not the best, maybe we should just keep -1 as the default value.
Anyway, the attached patch will solve this issue for now.

CommentFileSizeAuthor
#12 3117335-do_not_use_NULL_for_directory-12.patch1.74 KBytsurk
#11 3117335-do_not_use_NULL_for_directory-11.patch1.65 KBytsurk
#6 3117335-do_not_use_NULL_for_directory-6.patch916 bytesytsurk
adding-media-to-root-directory.patch1.47 KBrang501

Comments

rang501 created an issue. See original summary.

rang501’s picture

rang501 commented
Status: Active » Needs review
ytsurk’s picture

ytsurk
Location Switzerland
commented
Status: Needs review » Needs work

I have not seen problems with uploading to the root directory? How to reproduce?
Anyway I would not use the formstate directly, but adjust the method getDirectory, like

  protected function getDirectory(FormStateInterface $form_state) {
    $directory_id = (int) $form_state->get('active_directory');

    if (empty($directory_id)) {
      return MEDIA_DIRECTORY_ROOT;
    }

    return $directory_id;
  }

as I see no sense for the NULL return!

rang501’s picture

rang501 commented

I noticed it one of my projects (the site was reinstalled from scratch multiple times to exclude config related issues), but you are correct that it doesn't seem to show up on simple Drupal installation, I didn't find any custom code that should affect that.

The media entities are created but the directory field value was set to 0 and not NULL as it should be saved. If I tried to debug it I found that the getDirectory() returned NULL, which is not correct to use in request argument (active_directory field), after selecting files I think the empty value was typecasted to zero (on the same method). This NULL is used on saving entity (the createMediaFromValue() or prepareEntities()). I don't know why it does work clean installs :)

I think we really should use -1 as a root directory id value (and set it as the default value on media entity pre-save hook), this probably allows to clean up some extra code (I think I made some views related changes to load empty values), entity reference field shouldn't break if we use this value.

rang501’s picture

rang501 commented

Found the cause of the problem - if the directory field is hidden from the form (form display config), then this can be reproduced, if the field is visible then the browser will send value _empty which will set correct value (NULL). Backend seems to set wrong value by default (0).

ytsurk’s picture

ytsurk
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new 3117335-do_not_use_NULL_for_directory-6.patch916 bytes

So here my previous comment as real patch.

rang501’s picture

rang501 commented

Did you test it? -1 doesn't work because it throws an exception: Drupal\Core\Entity\EntityStorageException: SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'directory' at row 1
That was the reason I was using NULL value to determine root directory. I think what we need to do here is to add presave hook that ensures the correct value is set, like when field value is -1 then change it to empty. That way we could use -1 here or keep the previous logic and set it empty when 0 is set as a value. Third option - use my first patch :)

rang501’s picture

rang501 commented
Status: Needs review » Needs work
ytsurk’s picture

ytsurk
Location Switzerland
commented
Assigned: Unassigned » ytsurk

No -- did not test that .. Sorry. Thought this was a quickie .. Need to dive in deeper ..
I would keep the method and not directly add the form_state.

I come back with a better patch.

rang501’s picture

rang501 commented

Yes, you can keep that method but I think the best way is to create a presave hook and set value empty if the value is -1 (maybe include 0 here as well, it is an invalid value too), hopefully, it doesn't throw an exception because it doesn't try to save it yet.

Anyway, thanks for your time :)

ytsurk’s picture

ytsurk
Location Switzerland
commented
Assigned: ytsurk » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3117335-do_not_use_NULL_for_directory-11.patch1.65 KB

You're right. A presave hook makes it reliable. Still I would use the -1 if empty code.

It would go too far to change all ROOT_DIRECTORY logic...

ytsurk’s picture

ytsurk
Location Switzerland
commented
StatusFileSize
new 3117335-do_not_use_NULL_for_directory-12.patch1.74 KB

Added another if

rang501’s picture

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

Nice work! It's working well and I don't see any issues.

Thanks!

  • ytsurk committed 447779f on 8.x-1.x 
    Issue #3117335 by ytsurk, rang501: Adding media to root directory doesn'...
ytsurk’s picture

ytsurk
Location Switzerland
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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