API page: http://api.drupal.org/api/drupal/includes%21file.inc/function/file_save_...

A better name for the argument might be $file_field_name to stress the fact that it is the name of the '#type' => 'file', field in the form used to upload the file that is required here.

Files: 
CommentFileSizeAuthor
#30 1984378-change-param-name-30-D6.patch6.38 KBJaypan
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#23 1984378-change-param-name-23-D7.patch7.63 KBJaypan
PASSED: [[SimpleTest]]: [MySQL] 40,722 pass(es).
[ View ]
#19 1984378-change-param-name-19-D7.patch7.63 KBJaypan
PASSED: [[SimpleTest]]: [MySQL] 40,730 pass(es).
[ View ]
#12 1984378-change-param-name-12-D8.patch7.53 KBJacobSanford
PASSED: [[SimpleTest]]: [MySQL] 56,336 pass(es).
[ View ]
#12 interdiff_1984378_10-12.txt1.46 KBJacobSanford
#10 1984378-change-param-name-10-D8.patch7.56 KBJacobSanford
PASSED: [[SimpleTest]]: [MySQL] 55,713 pass(es).
[ View ]
#10 interdiff_5-10.txt950 bytesJacobSanford
#5 1984378-change-param-name-5-D8.patch6.78 KBaroq
PASSED: [[SimpleTest]]: [MySQL] 55,439 pass(es).
[ View ]
#3 1984378-change-param-name-3-D8.patch6.78 KBaroq
PASSED: [[SimpleTest]]: [MySQL] 55,536 pass(es).
[ View ]

Comments

jhodgdon’s picture

Version:7.x-dev» 8.x-dev
Issue tags:+Novice, +needs backport to D7

Wow, you are right -- that documentation is totally wrong. Looking at the places calling this function, your interpretation is correct.

Needs to be fixed in 8.x and then backported to 7.x.

aroq’s picture

Assigned:Unassigned» aroq
aroq’s picture

Assigned:aroq» Unassigned
Status:Active» Needs review
StatusFileSize
new6.78 KB
PASSED: [[SimpleTest]]: [MySQL] 55,536 pass(es).
[ View ]

Changed as proposed.

jhodgdon’s picture

Status:Needs review» Needs work

Thanks!

Since there is another concept in Drupal called "field", can we make it clear in the documentation that this is the name of a *form* field -- and also maybe make it clear what "name" means (since there are several properties of form fields that are "names")? For instance, if I have a form array with nested names, what would I need to pass in here to get this to work?

aroq’s picture

Status:Needs work» Needs review
StatusFileSize
new6.78 KB
PASSED: [[SimpleTest]]: [MySQL] 55,439 pass(es).
[ View ]

Changed.

jhodgdon’s picture

Assigned:Unassigned» jhodgdon
Status:Needs review» Reviewed & tested by the community

That looks better to me -- thanks! I'll get it committed sometime soon.

jhodgdon’s picture

Status:Reviewed & tested by the community» Needs work

I gave this patch a bit more scrutiny, and I see that the documentation for $destination is still referencing the old param name $source.

jhodgdon’s picture

Assigned:jhodgdon» Unassigned
JacobSanford’s picture

Assigned:Unassigned» JacobSanford
JacobSanford’s picture

Status:Needs work» Needs review
StatusFileSize
new950 bytes
new7.56 KB
PASSED: [[SimpleTest]]: [MySQL] 55,713 pass(es).
[ View ]

Re-roll of #5 as per review. I'm not in love with the language, but it is correctly stated.

jhodgdon’s picture

Status:Needs review» Needs work

I'm not in love with the language either... How about just saying "the file" and leaving it at that, rather than having all that explanation? I don't think there is any ambiguity about which file it would be.

Do you think the documentation for $form_field_name is clear either? In reading it today, I was confused... Maybe an example would be helpful or maybe it should say "the name of the key for the upload form element in the form array" or something like that?

JacobSanford’s picture

Status:Needs work» Needs review
StatusFileSize
new1.46 KB
new7.53 KB
PASSED: [[SimpleTest]]: [MySQL] 56,336 pass(es).
[ View ]

Thanks for review! A bit less muddled, I think.

Status:Needs review» Needs work
Issue tags:-Novice, -needs backport to D7

The last submitted patch, 1984378-change-param-name-12-D8.patch, failed testing.

The last submitted patch, 1984378-change-param-name-12-D8.patch, failed testing.

jhodgdon’s picture

Status:Needs work» Needs review
Issue tags:+Novice, +needs backport to D7
jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Novice

The documentation looks good to me now! Assuming the test bot agrees, I think it's ready to be committed.

alexpott’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed 08e8246 and pushed to 8.x. Thanks!

jhodgdon’s picture

Issue summary:View changes
Issue tags:+Novice

Someone just reported a duplicate of this issue... we should get this ported. Restoring Novice tag since porting a patch from 8 to 7 is probably a good Novice project?

Jaypan’s picture

StatusFileSize
new7.63 KB
PASSED: [[SimpleTest]]: [MySQL] 40,730 pass(es).
[ View ]

Uploading D7 backport

Jaypan’s picture

Status:Patch (to be ported)» Needs review

I'd like to add a comment that the 3 minutes it would have taken to do the backport to D7 would have saved me 2-3 hours work the other night when I was trying to debug a script that wasn't using a file upload element, but was uploading a file through services.

Jaypan’s picture

How can I trigger a test on that patch? Do I need to re-upload?

Jaypan’s picture

Status:Needs review» Patch (to be ported)

Switching back so I can force a test on the patch I will upload in the next comment.

Jaypan’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new7.63 KB
PASSED: [[SimpleTest]]: [MySQL] 40,722 pass(es).
[ View ]
jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community

Regarding triggering a test, sometimes it just takes a few minutes, as I guess you probably figured out. :) The tests run on a different machine, so there's a bot/process that queues the tests and updates the issue so you can see it's been recognized. Patience will decrease the load of testing the same patch twice. Thanks!

Anyway... This patch looks good to me. Note that although it looks like code has been changed, it is only renaming a function input parameter to be more sensible and I have carefully checked to make sure that (a) the changes are all correct and (b) all the instances of the parameter were changed. It is also the same patch as Drupal 8.

RE #20... Please keep in mind that Drupal is a volunteer-driven community. Rather than complaining that the post to Drupal 7 wasn't done earlier, could we instead say that the community very much appreciates that you posted a patch so that we can update Drupal 7, and that we very much appreciate that Dave posted the issue in the first place, and that Alexander and Jacob took the time to make the Drupal 8 patch, that I took the time to carefully review both the Drupal 7 and 8 patches, and that Alex committed the earlier patch to 8? Thanks!

Jaypan’s picture

Usually I wouldn't complain. I'm a volunteer myself and I do appreciate that they took the time to do the D8 patch in the first place. But if you're going to do the D8 patch, the D7 patch was essentially a copy, and took me less than 3 minutes to do it even having without having previously seen the d8 patch. I guess I should have said that if I had done the D8 patch in the first place, I also would have done the D7 patch, so someone else didn't waste a bunch of time on it in the future. Which is the reason I did the D7 patch now. After all, D7 is the current version, D8 is still in development. The D7 patch is more important now than the D8 patch is, it just happens that the proper workflow needs for the D8 patch to be done first.

But it's neither here nor there now, as both patches have been done.

Jaypan’s picture

I just discovered this also needs a backport to D6. Once the D7 backport has been committed, I'll do the D6 patch.

jhodgdon’s picture

Assigned:Jaypan» David_Rothstein

I'm going to let David handle committing this patch.

David_Rothstein’s picture

Version:7.x-dev» 6.x-dev
Assigned:David_Rothstein» Unassigned
Status:Reviewed & tested by the community» Patch (to be ported)
Jaypan’s picture

Assigned:Unassigned» Jaypan
Jaypan’s picture

StatusFileSize
new6.38 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

Adding patch to bring documentation in line with D7 and D8.

Jaypan’s picture

Status:Patch (to be ported)» Needs review
jhodgdon’s picture

This looks right. Drupal 6 unfortunately does not have a nice big test suite so we need to be much more careful with the patches... someone will need to make time to give this a very careful review.

Jaypan’s picture

Assigned:Jaypan» Unassigned

Removing myself from assignment in the hopes of garnering some attention for this issue.

jhodgdon’s picture

I do not think it is likely that this Drupal 6 issue will ever be reviewed/tested by anyone. It is probably doomed to just die a quiet death when we go around closing issues at its end of life. Sorry about that. It is just too risky to commit 6.x issues like this without a comprehensive testing suite.

Jaypan’s picture

Status:Needs review» Closed (won't fix)

In that case, I'm closing it.

jhodgdon’s picture

Version:6.x-dev» 7.x-dev
Status:Closed (won't fix)» Fixed

Well then let's at least put it back to D7 / fixed.

Status:Fixed» Closed (fixed)

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