Make WordPress Core

Opened 11 years ago

Closed 10 years ago

#23686 closed enhancement (fixed)

wp_handle_(upload|sideload) have a lot of copy-pasted code from each other

Reported by: nbachiyski's profile nbachiyski Owned by: wonderboymusic's profile wonderboymusic
Milestone: 4.0 Priority: normal
Severity: normal Version: 3.6
Component: Upload Keywords: has-patch commit
Focuses: Cc:

Description

A huge part of those two functions is the same. It needs to be refactored, so that they share the code.

Attachments (3)

23686.diff (14.9 KB) - added by wonderboymusic 10 years ago.
23686.2.diff (13.3 KB) - added by DrewAPicture 10 years ago.
less whitespacing, more docs
23686.3.diff (18.0 KB) - added by wonderboymusic 10 years ago.

Download all attachments as: .zip

Change History (11)

#1 @DrewAPicture
11 years ago

  • Cc xoodrew@… added

#2 @wonderboymusic
10 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.0

23686.diff combines the functions into a helper

#3 @helen
10 years ago

How many of these changes are whitespace/coding standards only?

#4 @wonderboymusic
10 years ago

very few - the 2 funcs use almost the exact same code - it just makes a new function that has that code that the 2 use

@DrewAPicture
10 years ago

less whitespacing, more docs

#5 @DrewAPicture
10 years ago

  • Keywords commit added

23686.2.diff removes a lot of the whitespacing (sorry Scott :/) and fixes the affected phpDocs.

This ticket was mentioned in IRC in #wordpress-dev by wonderboymusic. View the logs.


10 years ago

#7 @wonderboymusic
10 years ago

Just did a bunch of testing - 23686.3.diff is good to go. There were 3 esoteric places where these functions diverged. To test, load this somewhere:

require_once( ABSPATH . 'wp-admin/includes/file.php' );
require_once( ABSPATH . 'wp-admin/includes/media.php' );
media_sideload_image( 'http://cdn3.pitchfork.com/albums/20890/homepage_large.16aa901a.jpg', {YOUR_POST_ID} );

Turns out, $post_id should be optional all the way down the line. The only function that actually uses it for data is wp_insert_attachment(), which allows 0

#8 @wonderboymusic
10 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 29209:

Merge wp_handle_upload() and wp_handle_sideload() by making them each wrap a new function: _wp_handle_upload().

Props DrewAPicture for docs.
Fixes #23686.

Note: See TracTickets for help on using tickets.