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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

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

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

Issue tags: +Needs backport to D7

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

nanotube’s picture

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

nanotube’s picture

Same patch as above, but rolled for D7.

Status: Needs review » Needs work
nanotube’s picture

FileSize
716 bytes
736 bytes

renamed patches and re-upload.

nanotube’s picture

Status: Needs work » Needs review

.

Status: Needs review » Needs work

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

drewish’s picture

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

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

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

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

Assigned: Unassigned » nanotube

I will get on this in a day or two.

nanotube’s picture

Assigned: nanotube » Unassigned
Status: Needs work » Needs review
FileSize
1.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

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

Status: Needs work » Needs review
FileSize
2.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

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

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

Status: Needs work » Needs review
FileSize
1.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

Status: Needs review » Reviewed & tested by the community

good deal!

catch’s picture

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

Status: Patch (to be ported) » Reviewed & tested by the community
FileSize
1.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

Status: Needs work » Needs review
Issue tags: +Needs backport to D7

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

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

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.