API documentation in system.api.php contains @see references to upload_file_load() and upload_file_delete(). Neither of these functions appear to exist.

CommentFileSizeAuthor
#23 d7-1347812-23.patch1.11 KBxjm
#20 1347812-20.patch1.15 KBxjm
#17 documentation-1347812-17.patch2.27 KBnanotube
#15 documentation-1347812-15.patch1.53 KBnanotube
#7 D8-1347812-4.patch736 bytesnanotube
#7 D7-1347812-5.patch716 bytesnanotube
#5 D7 Remove documentation to upload_file_load() and upload_file_delete()-1347812-5.patch716 bytesnanotube
#4 Remove documentation to upload_file_load() and upload_file_delete()-1347812-4.patch736 bytesnanotube

Comments

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Novice

I checked with git blame and these lines date to the introduction of the file in 2008: http://drupalcode.org/project/drupal.git/commit/66df602593230a2483d65389...

Back then there was an upload.module. So we can just delete those lines or replace them with references to file_load(), file_load_multiple(), file_delete(), and file_file_delete() as appropriate.

xjm’s picture

xjm
she/her
Primary language English
commented
Title: upload_file_load() and upload_file_delete() do not appear to exist » Remove/replace documentation references to upload_file_load() and upload_file_delete()
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue tags: +Needs backport to D7

That was done in d7, so needs fixing there too.

nanotube’s picture

nanotube commented
Issue tags: -Novice
StatusFileSize
new Remove documentation to upload_file_load() and upload_file_delete()-1347812-4.patch736 bytes

Searched all files and references only exist in the system.api.php file.

nanotube’s picture

nanotube commented
Status: Active » Needs review
StatusFileSize
new D7 Remove documentation to upload_file_load() and upload_file_delete()-1347812-5.patch716 bytes

Same patch as above, but rolled for D7.

Status: Needs review » Needs work

The last submitted patch, D7 Remove documentation to upload_file_load() and upload_file_delete()-1347812-5.patch, failed testing.

nanotube’s picture

nanotube commented
StatusFileSize
new D7-1347812-5.patch716 bytes
new D8-1347812-4.patch736 bytes

renamed patches and re-upload.

nanotube’s picture

nanotube commented
Status: Needs work » Needs review

.

Status: Needs review » Needs work

The last submitted patch, D7-1347812-5.patch, failed testing.

drewish’s picture

drewish commented

nanotube, you want to name the files 1347812-D7.patch for Drupal 7 and 1347812.patch for Drupal 8. It looks for the -D7 to be a suffix before .patch. And the D8 version shouldn't have a suffix since it's based off the issue's version number.

xjm’s picture

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

Thanks nanotube. It looks like the @see references to the current functions were already there, so just removing these as the patch does should be fine.

I grepped with this patch applied and found one other stray reference to upload.module:

  • upload_munge_filename() is now file_munge_filename(). The stray reference is in includes/file.inc.

Want to add that fix as well? You can name your D7 patch something like 1347812-12-d7.patch so that testbot skips it. For the D8 patch something like 1347812-12.patch is fine. (Testbot skips anything with a -d# right before the file extension.)

There's also a few config variables that should probably be re-namespaced to file_ instead of upload_, but that's not a documentation fix (nor backportable) so we can create a followup issue for that.

Thanks!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

RE #10 - Actually, normally we just make a d8 patch and get it through approval, then reroll for d7 at that point. It saves work, since most patches go through several iterations before approval.

RE #11 - I agree, can we include that one other upload_ function in the fix rather than making another issue.

xjm’s picture

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

Setting to NW for #11. If someone could add that to the patch, that would be great. Thanks!

nanotube’s picture

nanotube commented
Assigned: Unassigned » nanotube

I will get on this in a day or two.

nanotube’s picture

nanotube commented
Assigned: nanotube » Unassigned
Status: Needs work » Needs review
StatusFileSize
new documentation-1347812-15.patch1.53 KB

I didn't see this earlier because I was searching on the files inside the modules/ directory.

Here is the patch to also fix the stray reference in includes/file.inc.

jhodgdon’s picture

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

Looks almost perfect! One thing:

  * @see file_load_multiple()
- * @see upload_file_load()
  */
 function hook_file_load($files) {

Maybe instead of removing that line, it should be replaced with file_load()?

nanotube’s picture

nanotube commented
Status: Needs work » Needs review
StatusFileSize
new documentation-1347812-17.patch2.27 KB

At first, I did not understand why, but a visit to the API website helped. Made the changes and attached a new patch.

xjm’s picture

xjm
she/her
Primary language English
commented

I think you added it to the wrong docblock?

Also, I'm actually not sure that we need to add file_load(), since it just calls file_load_multiple(), which is already referenced. So I actually think #15 is probably good. Let's see what @jhodgdon says, though. :) Thanks!

jhodgdon’s picture

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

I think referencing file_load() is useful.

That patch is weird... it seems to have system.api.php in it twice?

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 1347812-20.patch1.15 KB

Oh, I see. The patch was created using git format-patch. So actually it's just an illusion of the diff that it was added to the wrong docblock. This is why it's better to use git diff to create patches. :) So here's a reroll of #17 created with git diff.

jhodgdon’s picture

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

good deal!

catch’s picture

catch
he/him
Primary language English
commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.x, will need a quick re-roll for 7.x.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Patch (to be ported) » Reviewed & tested by the community
StatusFileSize
new d7-1347812-23.patch1.11 KB

A simple reroll. No differences between D7 and D8 here.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, d7-1347812-23.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
Issue tags: +Needs backport to D7

#23: d7-1347812-23.patch queued for re-testing.

jhodgdon’s picture

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

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

Good catch! DIE UPLOAD MODULE, DIE.

Committed and pushed to 7.x. Thanks! :)

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