Breaking this off from #1191236: Correct file_copy, file_move, file_save_data, and drupal_realpath to match documentation and their unmanaged counterparts.

The documentation for the $destination parameter of the file_save_data() function says, in part:

If this value is omitted, Drupal's default files scheme will be used, usually "public://".

The statement is correct but incomplete. If the $destination parameter is omitted or is a directory, a randomized name will be generated, as documented in the file_unmanaged_save_data() function which is called by this code.

CommentFileSizeAuthor
#7 file_save_data-better_docs-1192482-7.patch1.35 KBpillarsdotnet
#5 file_save_data-better_docs-1192482-5.patch1.3 KBpillarsdotnet
#1 file_save_data-better_docs-1192482-1.patch1.29 KBpillarsdotnet

Comments

pillarsdotnet’s picture

pillarsdotnet commented
StatusFileSize
new file_save_data-better_docs-1192482-1.patch1.29 KB

Patch copies the relevant documentation from file_unmanaged_save_data() to file_save_data().

pillarsdotnet’s picture

pillarsdotnet commented
Status: Active » Needs review
pillarsdotnet’s picture

pillarsdotnet commented
Title: Improve docs for drupal_save_data(). » Fix docs for drupal_save_data().
Issue tags: +Needs backport to D7
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Probably the grammar should be fixed too, as in one of your other issues?
If no value is provided, a randomized name will be generated and the file is saved using
is -> will be

pillarsdotnet’s picture

pillarsdotnet commented
Status: Needs work » Needs review
StatusFileSize
new file_save_data-better_docs-1192482-5.patch1.3 KB

Corrected as suggested, and re-wrapped at 80 columns.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

It is not wrapped at 80 characters:

  *   A string containing the destination URI.
 *   This must be a stream wrapper URI. If no value is provided, a randomized

First line should have text from second line moved up.

pillarsdotnet’s picture

pillarsdotnet commented
StatusFileSize
new file_save_data-better_docs-1192482-7.patch1.35 KB

Corrected.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks! 8.x/7.x please.

webchick’s picture

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

I'm confused. In the rest of these issues, we've removed the "If no value provided" part. Why is this one special?

pillarsdotnet’s picture

pillarsdotnet commented

Because in this one the code to deal with the "if no value provided" case is present. In the other ones, the code is missing. Note that I also submitted patches for the other functions to make the code match the docs, but since the patch to make the docs match the code was committed, I closed the alternative as a duplicate.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

webchick: we're just trying to make the doc match the code. If someone wants to change the code, that's another matter...

webchick’s picture

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

Ah, ok, understood.

Committed to 8.x and 7.x. Thanks!

Status: Fixed » Closed (fixed)

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

amontero’s picture

amontero
Location Barcelona
commented

Cross linking to related API feature proposal:
#1659116: Define FILE_STATUS_TEMPORARY and expose $status in file_save_data

amontero’s picture

amontero
Location Barcelona
commented
Title: Fix docs for drupal_save_data(). » Fix docs for file_save_data().

Title ammend