Closed (fixed)
Project:
Drupal core
Version:
7.x-dev
Component:
documentation
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
1 May 2013 at 18:16 UTC
Updated:
4 Mar 2014 at 17:40 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
jhodgdonWow, 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.
Comment #2
aroq commentedComment #3
aroq commentedChanged as proposed.
Comment #4
jhodgdonThanks!
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?
Comment #5
aroq commentedChanged.
Comment #6
jhodgdonThat looks better to me -- thanks! I'll get it committed sometime soon.
Comment #7
jhodgdonI gave this patch a bit more scrutiny, and I see that the documentation for $destination is still referencing the old param name $source.
Comment #8
jhodgdonComment #9
jacobsanfordComment #10
jacobsanfordRe-roll of #5 as per review. I'm not in love with the language, but it is correctly stated.
Comment #11
jhodgdonI'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?
Comment #12
jacobsanfordThanks for review! A bit less muddled, I think.
Comment #15
jhodgdon#12: 1984378-change-param-name-12-D8.patch queued for re-testing.
Comment #16
jhodgdonThe documentation looks good to me now! Assuming the test bot agrees, I think it's ready to be committed.
Comment #17
alexpottCommitted 08e8246 and pushed to 8.x. Thanks!
Comment #18
jhodgdonSomeone 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?
Comment #19
jaypanUploading D7 backport
Comment #20
jaypanI'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.
Comment #21
jaypanHow can I trigger a test on that patch? Do I need to re-upload?
Comment #22
jaypanSwitching back so I can force a test on the patch I will upload in the next comment.
Comment #23
jaypanComment #24
jhodgdonRegarding 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!
Comment #25
jaypanUsually 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.
Comment #26
jaypanI just discovered this also needs a backport to D6. Once the D7 backport has been committed, I'll do the D6 patch.
Comment #27
jhodgdonI'm going to let David handle committing this patch.
Comment #28
David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/e49fde1
Comment #29
jaypanComment #30
jaypanAdding patch to bring documentation in line with D7 and D8.
Comment #31
jaypanComment #32
jhodgdonThis 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.
Comment #33
jaypanRemoving myself from assignment in the hopes of garnering some attention for this issue.
Comment #34
jhodgdonI 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.
Comment #35
jaypanIn that case, I'm closing it.
Comment #36
jhodgdonWell then let's at least put it back to D7 / fixed.