WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 2 years ago

#20046 new enhancement

Unnecessary default category code remains in wp_insert_attachment

Reported by: jeremyfelt Owned by:
Milestone: Awaiting Review Priority: normal
Severity: trivial Version:
Component: Media Keywords: has-patch
Focuses: Cc:

Description

After the default arguments are set, post_type is forced to 'attachment', negating any requirement for default categories to be added.

Quick patch removes:

(1) The test performed for 'post' post_type in order to assign default categories to an array. Result was always an empty array.

(2) The call to wp_set_post_categories with this empty array, which sends the post_ID through a bunch of taxonomy related code that passes over it. This does cause the action 'set_object_terms' to be skipped, which should be okay as attachments don't have terms.

Attachments (1)

20046.diff (886 bytes) - added by jeremyfelt 2 years ago.

Download all attachments as: .zip

Change History (5)

jeremyfelt2 years ago

comment:1 jeremyfelt2 years ago

Clarification on the 'set_object_terms' action. It's possible that a plugin could use this to manage taxonomy on attachments as they are uploaded, so it may be useful to add the same do_action that would normally be run to the patch to replace the removed wp_set_post_categories.

do_action('set_object_terms', $object_id, $terms, $tt_ids, $taxonomy, $append, $old_tt_ids);

Would obviously need to change the arguments a bit.

I can redo the patch if it's determined that this action hook is necessary.

comment:2 dd322 years ago

Attachments have been added to the default taxonomy for quit awhile now, there's been previous tickets on this as well. From memory, the basic consensus was that some plugins that categorize attachments will expect the default category to be added, and it remains consistent with being backwards compatible.

comment:3 jeremyfelt2 years ago

I may be confused, and this patch was a result of something I noticed while hunting out another issue, but I don't see default taxonomy being provided for attachments at all.

From the current post.php code, if the post_type in wp_insert_attachment is set to 'attachment', which it always is, the $post_categories array is left empty. Only if post_type matches 'post' will it retrieve a default option for category.

Because of this, wp_set_post_categories is always called with an empty $post_categories array, which then leads to wp_set_object_terms being called with an empty $terms array. This in turn calls wp_get_object_terms at a possible 2 different points for the original attachment ID - only returning a result if a plugin has previously added associated a term with an attachment ID - unlikely in this scenario as we're still in wp_insert_attachment.

I could see moving do_action( 'set_object_terms'.... to post.php so that a plugin could hook in and add taxonomy to an attachment as it wishes, but I can't see how the remainder of the code is relevant.

comment:4 jeremyfelt2 years ago

  • Type changed from defect (bug) to enhancement

Further clarification on what is passed to the 'set_object_terms' hook, located a few functions away in wp_set_object_terms, when an attachment is added.

$terms = array();
$tt_ids = array();
$taxonomy = 'category';
$append = false;
$old_tt_ids = array();

do_action('set_object_terms', $object_id, $terms, $tt_ids, $taxonomy, $append, $old_tt_ids);

It's entirely possible a plugin is using this hook to automatically add category information on wp_insert_attachment, but unlikely. In reality, the same information ($post_ID) is passed to the 'add_attachment' and 'edit_attachment' hooks immediately after, which would be a more reliable place to hook in.

I'm going to leave the patch as is, but definitely open to this action hook being added directly to wp_insert_attachment as a replacement of wp_set_post_categories.

Also, this is mislabeled as a bug, switching to enhancement as it's more for optimization & cleanup.

Note: See TracTickets for help on using tickets.