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

[wido]

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.

[joemcgill]

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.

[flixos90]

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().

[wido]

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

[flixos90]

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.

[shulard]

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]

Updated attachment functions to allow post objects.

[Howdy_McGee]

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.

[davidbaumwald]

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]

Attempt #2

[Howdy_McGee]

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.

[joemcgill]

@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.

[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.

[hellofromTonya]

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]

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

[Mista-Flo]

Quickfix

[Mista-Flo]

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

[hellofromTonya]

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.

[johnbillion]

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.

[manfcarlo]

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.

[antpb]

There was general agreement in the recent Media meeting that broadening this to other functions is not desirable and instead it would be more appropriate to create new tickets for other functions that would adopt this structure. Broadening the ticket runs the risk of being across many teams that need to take special consideration to the change.

Also, as @johnbillion mentioned, this has a lot of untested code being touched so this ticket should have careful manual testing and bonus points for test coverage! :D

[hellofromTonya]

Please provide need more information to help testers manually test the patch (including at Test Scrubs):

• What are the steps to test?
• Are there any testing dependencies, such as a plugin or script?
• What is the expected behavior after applying the patch?

Let us know too if there's any specific test data everyone should be providing from their tests, such as screenshots, results, etc.

[antpb]

I dont feel like this ticket has enough testing and confidence to land in 5.7 Beta 1 today. I am moving this to 5.8 so we can discuss a little more. @hellofromTonya 's request for testing steps would be helpful for other contributors to jump in. :)

If anyone feels differently, please feel free to move it back into the milestone. Thanks!

[Howdy_McGee]

Patch Refreshed

[Howdy_McGee]

PHPUnit tests for patched functions

[antpb]

Thanks so much for the patch above Howdy_McGee!

If anyone would like to help this ticket along before folks on the media team get to testing, one action item would be to combine the two prior patches so tests are included with the logic patch.

Thanks!

[antpb]

This ticket touches a lot of areas that would be risky to merge this late in the cycle. We're moving it out of 5.9 to allow it the time needed to verify no breaking changes in the next release cycle.

[antpb]

Also a note, this should be added very early in the next release cycle to give enough time.

[Howdy_McGee]

Patch refreshed with tests.

[azaozz]

could be an improvement to standardize the functions that require a post id even support the $post object?

I mostly agree. Perhaps may be nice to standardize functions that do something with WP posts to accept either $post->ID or $post, purely for consistency. It doesn't make any difference "under the hood", just affects "code readability and self-documenting" a little. That may be a bit "weird" in some places, but being consistent even when weird is not that bad, maybe :)

However I do not agree it is a good idea to accept a WP post object in functions that do not do anything with it, for example all image specific functions. Looking at the current patch, all functions that have image in the name should not be changed to accept a $post object. There is no good excuse for that.

[Howdy_McGee]

Both image_downsize() and image_get_intermediate_size eventually call the get_post() function.

image_downsize -> wp_attachment_is_image -> wp_attachment_is -> get_post()

image_get_intermediate_size -> wp_get_attachment_url -> get_post()

If passed an object we can skip WP_Post::get_instsance( $post_id ) which checks the cache, and makes a possible database call to grab the object. Instead, we can populate the object directly.

[azaozz]

Replying to Howdy_McGee:

Both image_downsize() and image_get_intermediate_size eventually call the get_post()

Yes, but that's not an excuse to accept inconsistent parameters in functions that work on images. That would be exactly the opposite of the reason this could make sense:

Perhaps may be nice to standardize functions that do something with WP posts to accept either $post->ID or $post, purely for consistency. It doesn't make any difference "under the hood", just affects "code readability and self-documenting" a little. That may be a bit "weird" in some places, but being consistent even when weird is not that bad, maybe :)

If passed an object we can skip WP_Post::get_instsance( $post_id ) which checks the cache, and makes a possible database call to grab the object.

Yes, exactly. WP_Post is cached and get_post() uses that cache. It is the proper way to get the WP_Post instance, and retrieving it from the cache is extremely fast. It returns the "proper" instance after filters and actions have run, etc.

[azaozz]

Actually thinking more about it, perhaps it is not a good idea to promote use of the global $post as it might be polluted by third-parties. Think that was a pretty bad problem some years ago. The solution (if I remember correctly) was to do $post = get_post( $post ) regardless of whether $post was int or object. There are over a hundred places in core that do this.

Using get_post() is the preferred method even if the global $post is passed as a param. get_post() retrieves the WP_Post object from cache or fetches it from the DB if needed. Hence it always works and is "guaranteed".

So I don't think it is a good idea to accept the global $post instead of a post ID unless it really makes the function more readable, self-documenting, and easy to read/understand. Looking at 37255.6.diff I don't see any functions that satisfy this requirement, so thinking this should be closed as "wontfix".

[Howdy_McGee]

Yes, but that's not an excuse to accept inconsistent parameters in functions that work on images

Developers can continue to pass an Integer $attachment_id to the image_*() functions. That has not changed. The patch allows developers to also pass a WP_Post object instead of a $attachment_id if they choose to do so. This would make the parameters consistent with the other attachment functions in this patch.

Accepting a WP_Post Object or Post ID is verified in the patch tests as well.

Yes, exactly. WP_Post is cached and get_post() uses that cache.

In 99% of cases, the WP_Post object will be cached, but that is not necessarily a guarantee. Should we allow developers to make that distinction? If they have the WP_Post object not cached and don't want to make additional database calls this patch handles that. If the developer doesn't know the difference between passing a post_id or WP_Post it shouldn't matter since the results are unchanged.

Forcing developers to pass an Integer to a function that's going to call get_post() just seems like an unnecessary inconvenience when the results come out the same.

That all being said there hasn't been much else activity on this ticket other than patch refreshes. Maybe the image/attachment tests can be salvaged?

[azaozz]

Replying to Howdy_McGee:

In 99% of cases, the WP_Post object will be cached, but that is not necessarily a guarantee.

Not sure if that's true. If you can access the WP_Post object for a specific post, that means the post was retrieved from the database with WP_Post::get_instance() and is already cached, see ​https://developer.wordpress.org/reference/classes/wp_post/get_instance/.

Forcing developers to pass an Integer to a function that's going to call get_post() just seems like an unnecessary inconvenience

Perhaps some developers see it that way, however the inconvenience of having to use $post->ID instead of $post is super trivial imho, just 4 more characters to type. On the other hand the inconsistency of some image functions accepting WP_Post objects but others only accepting $attachment_id seems quite annoying and perhaps "wrong" from software architecture point of view.

Maybe the image/attachment tests can be salvaged?

Yep, seems some of the tests can be used but lets move them to a new ticket. Would be a lot clearer that way.