WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 weeks ago

#37255 accepted enhancement

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

Reported by: wido Owned by: johnbillion
Milestone: 5.7 Priority: normal
Severity: normal Version: 4.6
Component: Media Keywords: has-patch needs-testing needs-dev-note
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 (4)

37255.diff (23.3 KB) - added by Howdy_McGee 9 months ago.
Updated attachment functions to allow post objects.
37255-1.patch (23.2 KB) - added by Howdy_McGee 5 months ago.
Attempt #2
37255.3.patch (29.0 KB) - added by Mista-Flo 2 months ago.
Refresh patch; Improve doc; Fix coding standards; Remove use of global post
37255.4.patch (29.5 KB) - added by Mista-Flo 2 months ago.
Quickfix

Download all attachments as: .zip

Change History (34)

#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
4 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
9 months ago

Updated attachment functions to allow post objects.

#7 @Howdy_McGee
9 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.


8 months ago

#9 @SergeyBiryukov
8 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.


7 months ago

#11 @antpb
7 months ago

  • Milestone changed from Future Release to 5.5

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


5 months ago

#13 @davidbaumwald
5 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
5 months ago

Attempt #2

#14 @Howdy_McGee
5 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.


5 months ago

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


5 months ago

#17 @joemcgill
5 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.

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


2 months ago

#19 follow-ups: @johnbillion
2 months ago

  • Keywords needs-refresh added

Allowing these functions to accept either a post ID or a post object is a good idea. This should be the case for any function that accepts a post ID.

Making the ID parameter optional for these attachment related functions may cause some unexpected behaviour though. There's a limited number of places where calling wp_get_attachment_url() without an ID parameter would work as expected, in fact the only one I think of off the top of my head is when viewing an attachment at its permalink.

I'd like to see this patch land but without the fallback to the global post object.

#20 @hellofromTonya
2 months ago

  • Keywords needs-dev-note added

Adding needs-dev-note for a small call-out on the Misc Dev Note for the filter changes.

We are well into 5.6 with beta coming very soon. This ticket needs the refresh and review suggestions in the next week or so, i.e. to keep it on track for consideration to land in the 5.6 release.

@Mista-Flo
2 months ago

Refresh patch; Improve doc; Fix coding standards; Remove use of global post

#21 @davidbaumwald
2 months ago

  • Keywords needs-refresh removed

@Mista-Flo
2 months ago

Quickfix

#22 in reply to: ↑ 19 @Mista-Flo
2 months ago

Replying to johnbillion:

Allowing these functions to accept either a post ID or a post object is a good idea. This should be the case for any function that accepts a post ID.

Making the ID parameter optional for these attachment related functions may cause some unexpected behaviour though. There's a limited number of places where calling wp_get_attachment_url() without an ID parameter would work as expected, in fact the only one I think of off the top of my head is when viewing an attachment at its permalink.

I'd like to see this patch land but without the fallback to the global post object.

Hi John, I have uploaded a new patch without the fallback to the global post object.

I have also improved the doc and added more consistency to the filters.

Do you think it needs some unit test? If so, which function to test?

BTW: 37255.4.patch is the most accurate version

Last edited 2 months ago by Mista-Flo (previous) (diff)

#23 @johnbillion
2 months ago

  • Owner changed from joemcgill to johnbillion
  • Status changed from reviewing to accepted

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


7 weeks ago

#25 @hellofromTonya
7 weeks ago

Capturing comment from @johnbillion during today's Media scrub

there are many other functions that should be updated which makes this a bit of a rabbit hole. It will probably need a few separate commits. I'm on it.

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


6 weeks ago

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


6 weeks ago

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


6 weeks ago

#29 @johnbillion
6 weeks ago

  • Milestone changed from 5.6 to 5.7

I'm going to bump this as I haven't had a chance to fully test this even though I agree with the overall change, and it's not part of any specific 5.6 focus. In addition, this touches a lot of code that doesn't have test coverage.

Related: #51553 depending on the decision made in that ticket around back-compat for parameter name changes.

#30 in reply to: ↑ 19 @manfcarlo
3 weeks ago

Replying to johnbillion:

Allowing these functions to accept either a post ID or a post object is a good idea. This should be the case for any function that accepts a post ID.

Hi, I've just been alerted to this ticket from the mention at #51373. I see the main focus of this ticket has been attachment-related functions, but are you suggesting with the quoted comment that the ticket should be broadened to functions in other areas as well? get_post_meta is just one I can think of off the top of my head.

Note: See TracTickets for help on using tickets.