Make WordPress Core

Opened 7 years ago

Closed 5 years ago

#43579 closed enhancement (fixed)

Optional post_id param in media_sideload_image and media_handle_sideload function

Reported by: sebastianpisula's profile sebastian.pisula Owned by: antpb's profile antpb
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch needs-testing
Focuses: Cc:

Description

I think that post_id param in media_sideload_image and media_handle_sideload function should be option optional.

media_sideload_image() call media_handle_sideload() with post_id and this function get current time $time = current_time( 'mysql' ); if post date not exists.

Sa this param can be optional.

Attachments (4)

43579.patch (2.0 KB) - added by sebastian.pisula 7 years ago.
435791.patch (2.0 KB) - added by sebastian.pisula 7 years ago.
43579.3.diff (2.2 KB) - added by donmhico 5 years ago.
43579.4.diff (2.2 KB) - added by kirasong 5 years ago.
Refreshed 43579.3.diff.

Download all attachments as: .zip

Change History (12)

#1 @sebastian.pisula
7 years ago

  • Keywords has-patch added

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


6 years ago

#3 @antpb
6 years ago

#38959 was marked as a duplicate.

#4 @antpb
6 years ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 5.3
  • Owner set to antpb
  • Status changed from new to assigned
  • Type changed from feature request to enhancement

Hi @sebastianpisula ! Thanks for contributing this patch. It is a great change. This was also worked on in #38959 and was discussed in the most recent media meeting. We agree that the solution is sound and that it should move forward in the 5.3 milestone. I'll be testing it in the coming week and will work to commit it.

Just wanted to leave this comment to document the work that @dotancohen did in #38959 . You both came to the very same solution and the Media team thinks you should both get props for it. I'm going to make sure that happens. :)

Thanks again, and more info to follow soon as I test.

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


6 years ago

#6 @SergeyBiryukov
6 years ago

Coming from #core to give a second committer opinion as requested :)

Since $post_id is eventually passed as `post_parent` to wp_insert_attachment(), I think the default value of $post_id = 0 would be more appropriate. It would match the documented type (int) and would also be more consistent with the rest of core (~33 instances of $post_id = 0 vs. ~9 of $post_id = null). In get_post(), both values are treated the same way.

Looks good otherwise.

@donmhico
5 years ago

#7 @donmhico
5 years ago

In my attached patch, 43579.3.diff, I made the default value 0 as per @SergeyBiryukov recommendation. I also added @since docs.

@kirasong
5 years ago

Refreshed 43579.3.diff.

#8 @antpb
5 years ago

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

In 46245:

Media: Sets post_id optional in media_sideload_image() and media_handle_sideload().
Props SergeyBiryukov, donmhico, mikeschroder, sebastian.pisula.
Fixes #43579.

Note: See TracTickets for help on using tickets.