It seems that #517814: File API Stream Wrapper Conversion broke file_save_upload(). Right now it only works for file in the root temporary or files directories (private or public). It does not work unless you include a trailing slash in the directory name.

This patch takes the approach that a trailing slash is both allowed and not necessary. I think a better approach might be to not work at all with a trailing slash, but the performance impact is non-existent and it'd be hard to accurately report such an error.

For those wondering, user picture upload still works because it uploads to the root temporary directory and then moves it to files/pictures later.

CommentFileSizeAuthor
#5 save-upload-552066-5.patch3.81 KBpwolanin
#1 file_save_upload_trailing_slash.patch665 bytesquicksketch
file_save_upload_trailing_slash.patch993 bytesquicksketch

Comments

quicksketch’s picture

quicksketch
he/him
commented
StatusFileSize
new file_save_upload_trailing_slash.patch665 bytes

Ha, of course that last patch is going to break uploads directly the root of file directories. This one adds a trailing slash only if it doesn't already end in a trailing slash.

aaron’s picture

aaron commented
Priority: Normal » Critical

good find. is this worthy of a new test, perhaps?

pwolanin’s picture

pwolanin commented

simpler code using substr:

  // A URI may already have a trailing slash or look like "public://".
  if (substr($destination, -1) != '/') {
    $destination .= '/';
  }
pwolanin’s picture

pwolanin commented
Status: Needs review » Needs work

needs a test case also.

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review
StatusFileSize
new save-upload-552066-5.patch3.81 KB
quicksketch’s picture

quicksketch
he/him
commented
Status: Needs review » Reviewed & tested by the community

Much better, looks great to me.

aaron’s picture

aaron commented

I love that there's finally a way to win by running these tests!!! Now it's not just the good feeling you get from seeing all the green bars...

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks a lot!

Status: Fixed » Closed (fixed)

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