WordPress.org

Make WordPress Core

Opened 16 months ago

Last modified 16 months ago

#20046 new enhancement

Unnecessary default category code remains in wp_insert_attachment

Reported by: jeremyfelt Owned by:
Priority: normal Milestone: Awaiting Review
Component: Media Version:
Severity: trivial Keywords: has-patch
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 16 months ago.

Download all attachments as: .zip

Change History (5)

jeremyfelt16 months ago

comment:1 jeremyfelt16 months 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 dd3216 months 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 jeremyfelt16 months 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 jeremyfelt16 months 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.