Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#50972 closed defect (bug) (fixed)

media_handle_sideload() does not allow $post_data to override the post_date that gets used by wp_upload_dir

Reported by: jamesgol's profile jamesgol Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.7 Priority: normal
Severity: minor Version: 5.3
Component: Media Keywords: has-patch commit
Focuses: Cc:

Description

Now that $post_id is an optional attribute to media_handle_sideload() it should be possible to override the $time passed to wp_handle_sideload() via the optional $post_data array.

My use case is calling media_handle_sideload() via a custom plugin, only after the existing library is checked for duplicates is the post actually created so there is no $post_id to reference (yet).

Attachments (6)

media_handle_sideload_date.diff (430 bytes) - added by jamesgol 4 years ago.
50972.patch (435 bytes) - added by mukesh27 4 years ago.
Updated patch.
50972.2.diff (1.7 KB) - added by hellofromTonya 3 years ago.
Optimizes setting the time.
50972.gif (9.4 MB) - added by hellofromTonya 3 years ago.
Results of testing the 3 different scenarios. Works as expected.
50972-before.gif (7.3 MB) - added by hellofromTonya 3 years ago.
Testing 3 scenarios before applying 50972.2.diff patch. Works as expected.
Screenshot 2021-02-08 at 23.57.17.png (137.2 KB) - added by Mista-Flo 3 years ago.
After the 2nd patch - Works as expected

Change History (23)

#1 @mukesh27
4 years ago

#50971 was marked as a duplicate.

#2 @SergeyBiryukov
4 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 5.6
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#3 @mukesh27
4 years ago

Hi there!

isset( $post_data ) value is always true when it was empty array or non empty array.

Update the patch without checking it.

@mukesh27
4 years ago

Updated patch.

#4 @jamesgol
4 years ago

I always assume that someone's going to do something wrong like try to pass in null or false as an argument. So in my opinion it's safer to at least check. One step further would be a check for is_array().

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


4 years ago

#6 @hellofromTonya
4 years ago

  • Keywords needs-unit-tests added

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


4 years ago

#8 @antpb
4 years ago

  • Milestone changed from 5.6 to 5.7

It was mentioned in the recent Media meeting that the sideloading functions could benefit from a refactor to allow testing as it is currently hard to test the same way that media_sideload_image was and did not receive tests.

Being so late in beta cycle it's probably best to move this out of the 5.6 milestone. If anyone feels differently please feel free to move it back in.

#9 @hellofromTonya
4 years ago

  • Version changed from trunk to 5.3

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 years ago

#11 @Mista-Flo
3 years ago

  • Keywords needs-unit-tests removed

So unfortunately, we can not add some unit tests easily for this function.

The patch seems quite minimal, so it shouldn't affect that much.

That said, we need some steps in order to do some manual testing, would be nice to get it tested by 2 different people at least.

In the meantime, we should open another ticket to handle the refactoring part in order to make unit test creation possible for these functions.

@hellofromTonya
3 years ago

Optimizes setting the time.

#12 @hellofromTonya
3 years ago

Patch 50972.2.diff includes the following changes:

  • Fixes a typo in the isset keyname
  • Uses the same year check strategy as with post, i.e. for consistency
  • Optimizes setting the time by doing the get_post if no post_date is passed in the post data

#13 @hellofromTonya
3 years ago

Testing strategy:

Used this code in a must-use script:

<?php
add_action( 'admin_init', function() {
        if ( ! isset( $_GET['test50972'] ) ) {
                return;
        }

        $url = 'https://s.w.org/about/images/logos/wordpress-logo-stacked-rgb.png';
        $tmp = download_url( $url );
        if ( is_wp_error( $tmp ) ) {
                return;
        }

        $file_array = array(
                'name'     => basename( $url ),
                'tmp_name' => $tmp
        );

        $post_id   = 0;
        $post_data = array(); //array( 'post_date' => '2019-01-01 00:00:00' );
        media_handle_sideload( $file_array, $post_id, null, $post_data );
} );

Test an unattached file without passing post date:

  1. Go to the Media Library in the backend
  2. Append ?test50972=1 to the end of the URL in your browser
  3. Press enter/return to load that URL
  4. The logo image should now appear in the library
  5. The image's path should be wp-content/uploads/currentYear/currentMonth/ /Users/hellofromtonya/Dev/wordpress-develop/src/wp-content/uploads/2021/02/wordpress-logo-stacked-rgb.png and the other versions of it

This is the previous expected behavior.

Test when attached to the default "Hello World" post 1 without passing post date:

  1. Change the date of the "Hello World" post to a different year such as "2020"
  2. In the test code above, change the $post_id = 1;
  3. Repeat the above test
  4. The new image's path should be in `wp-content/uploads/currentYear/2020/

This is the previous expected behavior.

Test when passing the post's date:

  1. In the test code above, change the $post_data to use the commented out array: $post_data = array( 'post_date' => '2019-01-01 00:00:00' );
  2. Repeat the above test
  3. The new image's path should be in `wp-content/uploads/currentYear/2019/

This is the new change.

Last edited 3 years ago by hellofromTonya (previous) (diff)

@hellofromTonya
3 years ago

Results of testing the 3 different scenarios. Works as expected.

#14 @jamesgol
3 years ago

FWIW: When I submitted this I tested my original patch and it worked as expected. I ended up just working around the issue because I needed the functionality immediately at the time.

@hellofromTonya
3 years ago

Testing 3 scenarios before applying 50972.2.diff patch. Works as expected.

#15 @Mista-Flo
3 years ago

Just tested the patch as well, it works as expected, great enhancement on the patch @hellofromTonya well done.

@Mista-Flo
3 years ago

After the 2nd patch - Works as expected

#16 @hellofromTonya
3 years ago

  • Keywords commit added

Thanks for testing it @Mista-Flo! Marking this one for commit. Working with @antpb now on it.

#17 @antpb
3 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 50258:

Media: Allow post_date to be respected in media_handle_sideload().

Previously, date information was unable to be changed when using media_handle_sideload().

Now you can override the date for a media item using $post_data['post_date'] before using the function.

Props jamesgol, mukesh27, SergeyBiryukov, hellofromTonya, Mista-Flo.
Fixes #50972.

Note: See TracTickets for help on using tickets.