Make WordPress Core

Opened 6 years ago

Closed 4 years ago

#20046 closed enhancement (invalid)

Unnecessary default category code remains in wp_insert_attachment

Reported by: jeremyfelt Owned by:
Milestone: Priority: normal
Severity: trivial Version: 2.0
Component: Media Keywords: has-patch reporter-feedback
Focuses: Cc:


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 6 years ago.

Download all attachments as: .zip

Change History (9)

6 years ago

#1 @jeremyfelt
6 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.

#2 @dd32
6 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.

#3 @jeremyfelt
6 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.

#4 @jeremyfelt
6 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.

#5 @ericlewis
4 years ago

  • Keywords reporter-feedback added
  • Version set to 2.0

Consider the following.

I for some reason want categories to apply to attachments. So, I add this code somewhere (functions.php, mu-plugin, plugin, pick your poison):

add_action( 'init', function() {
        register_taxonomy_for_object_type( 'category', 'attachment' );

I then set the category while creating an attachment somewhere else:

        array( 'post_status' => 'publish', 
                'post_title' => 'Robin Williams dancing to Thriller with Jim Carrey',
                'post_category' => array( 3, 5 ) ) );

The code that's on the chopping block here will be needed to cover this edge case. Suggest a close here as wontfix. @jeremyfelt, thoughts?

Last edited 4 years ago by ericlewis (previous) (diff)

#6 @wonderboymusic
4 years ago

I recently annotated wp_insert_attachment() - it shows where it diverges from wp_insert_post() and how it relates to post_category

#7 @helen
4 years ago

Related, and we should do this please: #21963

#8 @wonderboymusic
4 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

There is no more substance inside wp_insert_attachment() after [28788]. No unnecessary category code runs unless category is enabled for the attachment post type.

Note: See TracTickets for help on using tickets.