WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 months ago

#37255 reviewing enhancement

Update attachment functions to accept a post object in addition to ID

Reported by: wido Owned by: joemcgill
Milestone: 5.6 Priority: normal
Severity: normal Version: 4.6
Component: Media Keywords: has-patch needs-testing
Focuses: Cc:

Description

Some WordPress functions accept the $post object as parameter instead only the $post_id.

Since most of the time we write our code within the loop or we use the $post object, could be an improvement to standardize the functions that require a post id even support the $post object?

For the wp_get_attachment_caption for example, could be like this:

/**
 * Retrieves the caption for an attachment.
 *
 * @since 4.6.0
 *
 * @param int|WP_Post|null $post Optional. Post ID or post object. Defaults to global $post.
 * @return string|false False on failure. Attachment caption on success.
 */
function wp_get_attachment_caption( $post = null ) {

	if ( ! $post instanceof WP_Post ) {
		if ( ! $post = get_post( $post ) ) {
			return false;
		}
	}

	if ( 'attachment' !== $post->post_type ) {
		return false;
	}

	$caption = $post->post_excerpt;

	/**
	 * Filters the attachment caption.
	 *
	 * @since 4.6.0
	 *
	 * @param string $caption Caption for the given attachment.
	 * @param WP_Post   $post Attachment object.
	 */
	return apply_filters( 'wp_get_attachment_caption', $caption, $post );
}

The get_the_post_thumbnail_caption that use the wp_get_attachment_caption accept the $post object.

Also, instead of passing the $post->ID to the wp_get_attachment_caption, use the $post object, so we can work directly with the object instead of calling again the get_post function to retrieve it.

Attachments (2)

37255.diff (23.3 KB) - added by Howdy_McGee 7 months ago.
Updated attachment functions to allow post objects.
37255-1.patch (23.2 KB) - added by Howdy_McGee 3 months ago.
Attempt #2

Download all attachments as: .zip

Change History (19)

#1 @joemcgill
4 years ago

  • Component changed from Posts, Post Types to Media
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.6
  • Owner set to joemcgill
  • Status changed from new to reviewing

Hi @wido,

Thanks for the suggestion. It may be to late to get this in 4.6, but since this is a brand new function, it's not really an enhancement as much as a possibly missed implementation detail, so let's take a look at it.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


4 years ago

#3 follow-up: @flixos90
4 years ago

  • Milestone changed from 4.6 to Future Release

Most similar attachment functions only accept an ID, so I just moved that over to the new function for wp_get_attachment_caption(). It makes sense to improve them to accept not only an ID, but also a post object IMO.

However, although it is a straightforward fix, it will affect quite a few functions that have been around for a longer time. I wouldn't like only changing it for wp_get_attachment_caption() just because we can (since it's new). Because of this broader scope let's think about it in 4.7. There needs to be a discussion whether we should change it for all the attachment functions or leave it as is.

While it wouldn't hurt to support objects, I personally never felt somewhat limited by using IDs for attachments. Maybe it's just because it has already sunk in that attachment functions accept IDs.

Anyway, the functions that would need to be enhanced would be:

In wp-includes/post.php:

  • wp_get_attachment_metadata()
  • wp_update_attachment_metadata()
  • wp_get_attachment_url()
  • wp_get_attachment_caption()
  • wp_get_attachment_thumb_file()
  • wp_get_attachment_thumb_url()

In wp-includes/media.php:

  • image_downsize()
  • get_image_tag()
  • image_get_intermediate_size()
  • wp_get_attachment_image_src()
  • wp_get_attachment_image()
  • wp_get_attachment_image_url()
  • wp_get_attachment_image_srcset()
  • wp_get_attachment_image_sizes()

I might be missing some more. The only attachment function I found that supports passing an object is wp_attachment_is().

#4 in reply to: ↑ 3 ; follow-up: @wido
4 years ago

There are also these functions that support the WP_Post object as parameter:

post_password_required
setup_postdata
get_post
get_post_type
sanitize_post
wp_trash_post_comments
wp_untrash_post_comments
get_page_uri
get_oembed_response_data
get_page_templates

#5 in reply to: ↑ 4 @flixos90
4 years ago

Replying to wido:

There are also these functions that support the WP_Post object as parameter [...]

I think what we need to discuss here is whether attachment functions are intentionally different from regular post functions (like the ones you mentioned) or whether it just hasn't been improved yet.
I tend to agree with you that these functions should support post objects too, but due to the impact of this change, we probably won't be able to get it in this release cycle.

#6 @shulard
3 years ago

Hello !

I've just faced the issue when using wp_get_attachment_url function. I think that the behaviour of media functions must be normalized with others functions.

I started to work on a patch but it's bigger than expected and can't finish it until next week.

Relying on get_post will not break the current behaviour of the functions : retrieve post from ID when necessary, elsewhere use the already loaded data.

@Howdy_McGee
7 months ago

Updated attachment functions to allow post objects.

#7 @Howdy_McGee
7 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

Hello,

I've updated the listed functions to use the Post ID or Post object. Any function that defaulted to the global post will continue to do so. These are just the attachment related functions, not any of the post functions. I think if we want to modify a list of post related functions to include passable objects we should create a separate ticket for that.

With this patch I've also added an additional post type check so that if the global $post happens to not be an attachment the functions will return false instead of whatever the given function may return ( empty string or empty array ). Additionally, I've passed the post object to the function filter hooks as needed.

Any attachment functions that I may have missed or were not listed we could create separate tickets and handle them on a per function basis.

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


6 months ago

#9 @SergeyBiryukov
6 months ago

  • Summary changed from wp_get_attachment_caption and post parameter to Update attachment functions to accept a post object in addition to ID

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


5 months ago

#11 @antpb
5 months ago

  • Milestone changed from Future Release to 5.5

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


3 months ago

#13 @davidbaumwald
3 months ago

  • Keywords needs-refresh added

Latest patch fails against trunk, so this needs a refresh. @antpb Is this still on your list to handle with 5.5 Beta 1 landing in a few days?

@Howdy_McGee
3 months ago

Attempt #2

#14 @Howdy_McGee
3 months ago

  • Keywords needs-refresh removed

Patch refresh, keeps current updates and fixes wp_get_attachment_metadata(), wp_update_attachment_metadata() and wp_get_attachment_url(). Additionally, while creating this patch I'm not sure the wp_get_attachment_thumb_file() function has worked properly for awhile.

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


3 months ago

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


3 months ago

#17 @joemcgill
3 months ago

  • Milestone changed from 5.5 to 5.6

@Howdy_McGee sorry, I wasn't able to review this before the beta deadline. I'm going to move to the 5.6 milestone so we can land this early in the next cycle.

Note: See TracTickets for help on using tickets.