Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#37368 closed defect (bug) (fixed)

get_object_taxonomies() ignores output type argument for attachments

Reported by: rarst's profile Rarst Owned by:
Milestone: 4.7 Priority: normal
Severity: normal Version: 3.0
Component: Taxonomy Keywords: good-first-bug has-patch
Focuses: Cc:

Description

If you request array of taxonomy objects for an attachment it is ignored and array of names is always returned by nested get_attachment_taxonomies() call.

Attachments (1)

37368.diff (1.0 KB) - added by codemovement.pk 8 years ago.

Download all attachments as: .zip

Change History (15)

#1 @ocean90
8 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from trunk to 3.0

get_attachment_taxonomies() was added to get_object_taxonomies() in [7520]. The output argument for get_object_taxonomies() was added in [14479].

#2 @boonebgorges
8 years ago

  • Keywords good-first-bug added

#3 @codemovement.pk
8 years ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Resolution set to worksforme
  • Status changed from new to closed

I have added the patch to fixed the above issue.

#4 @Rarst
8 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

Don't think ticket is supposed to be closed until appropriate patch is actually merged into source.

This ticket was mentioned in Slack in #core by m.usama.masood. View the logs.


8 years ago

#6 @deeptiboddapati
8 years ago

I looked into this and tested out the patch. I definitely recreated the bug and the patch seemingly fixes it. But when I dug deeper it ended up being a lot bigger than it first seemed.

So I looked at 3 functions get_object_taxonomies, get_attachment_taxonomies, get_post_taxonomies.

I looked at get_post_taxonomies because it supposed to work with all WP_Post objects and seems to repeat the get_attachment_taxonomies function.

get_object_taxonomies() and get_attachment_taxonomies()call each other to evaluate WP_Post objects that are attachments. But get_attachment_taxonomies() just passes it back to get_object_taxonomies() after doing a lot of unneeded text parsing.

This circular dependency should be removed because a lot of other functions depend on get_object_taxonomies(). But the patch suggested doesn't remove it so while I think it fixes it, I think for the long run we should go another direction.

It also doesn't address any of the issues I found below. I don't know if we are supposed to address this here or if I should open bug reports for each of the things I found below.

I tested the functions and I found issues with each of the functions and I have questions about how to approach fixing them.
I broke the problems and my questions down here.

get_object_taxonomies()

Return

the names or objects of the taxonomies which are registered for the requested object or object type, such as a post object or post type name.

Allowed input

Param 1-

  • WP_Post object
  • String for post type name
  • Array? The docs don't specify what this array should be.

Question: What is the array?

Param 2-

  • String either 'names' or 'objects'

I/O table

Input I = Invalid ?= unsure Right return for "Objects" Right return for "Names"
(object) Attachment NO YES
(string) "attachment" YES YES
? (array) Attachment YES YES
? (array) attachment objects* NO NO
(object) Post YES YES
(string) "post" YES YES
? (array) Post YES YES
?(array) WP_Post objects assorted post types NO NO
?(array) post_types as strings* YES YES
I (int) id of post or attachment YES YES

Issues

*Docs say that you should be able to pass in an array. They don't specify what kind of array.
It works for a post array.
It doesn't work for an array of post arrays or an array of post objects.

It works if you pass in an array with different post type names. For example:
get_object_taxonomies(['attachment','post'], 'objects');
returns the union of taxonomy objects associated with either attachments or posts
Question: Is this an intended behavior?

For param 2 it evaluates 'objects' as anything that is not 'names' so you could pass anything in.
This returns taxonomy objects:
get_object_taxonomies(WP_Post, false)
Question: Is this an intended behavior or should it specifically check for 'objects'?

get_attachment_taxonomies()

Return

an array of names of taxonomies associated with a given attachment

Allowed input

  • An id of an Attachment
  • An Attachment object
  • Data Array? The docs don't specify what this data array should be.

Question: What is the data array?

I/O table

Input I= invalid ?=unsure Is it correct?
(object) Attachment Yes
?(array) Attachment Yes
(int) id of Attachment Yes
I (string) 'attachment' Yes
?(array) Attachment objects Yes but with Notice*
?(array) Attachment arrays Yes but with Notice*
?(array) Attachment ids Yes but with Notice*
I (object) Post No
I (int) Post Id No

Issues

Passing an array of Attachment objects, ids or arrays returns the names of the taxonomies correctly but displays a notice:

Notice: Undefined property: stdClass::$ID in C:\MAMP\htdocs\trunk\src\wp-includes\media.php on line 2709

It looks like it works by accident. If it wasn't chained on to get_object_taxonomies it wouldn't work.

If you pass in a Post object or id it returns all taxonomy associated with Attachments. It should return an empty array since this is invalid input.

Question: Should this return an empty array for invalid post types or return an array of Taxonomies associated with attachments? Is this within the scope of this bug fix?

Inside this function it does a lot of text parsing and creates an array of the attachment's post_type mime type, file extension etc. Then it checks if there are any taxonomies associated with each of them. Currently taxonomies are only associated with Post Types.
Question:Should I remove the superflous code?
Curiosity Question: When were taxonomies associated with file extension and mime types?

get_post_taxonomies()

Return

an array of names of taxonomies associated with a given WP_Post object.

Allowed input

  • A WP_Post object
  • An id of a WP_Post object

Question: Should this function always return an empty array if input is invalid?

I/O table

Inputs, I = invalid Does it work correctly?
(object) Attachment YES
(int) Attachment Id YES
I (array) Attachment NO*
I (array) Attachment objects NO*
I (array) Attachment arrays NO*
I (string) "attachment" YES Empty Array
(object) Post YES
(int) Post Id YES
I (array) Post NO*
I (array) Post arrays NO*
I (array) Post objects NO*
I (string) "post" YES Empty Array

Issues

*These don't work correctly. Its just defaulting to post id=1. So it returns what ever taxonomies are associated with that post.
It should return an empty array for invalid inputs.

Question: Should I patch this in this bug or is this out of the scope of this bug fix?

Last edited 8 years ago by deeptiboddapati (previous) (diff)

#7 @boonebgorges
8 years ago

In 38290:

Introduce tests for get_object_taxonomies().

See #37368.

#8 @boonebgorges
8 years ago

In 38291:

Introduce tests for get_attachment_taxonomies().

See #37368.

#9 @boonebgorges
8 years ago

  • Keywords needs-unit-tests needs-testing removed
  • Milestone changed from Future Release to 4.7

@codemovement.pk Thank you for your patch! On review, I think we can reduce duplicated code and simplify the logic flow by updating get_attachment_taxonomies() so that it accepts an $output parameter just like get_object_taxonomies() does. I'm going to clean up your patch in this way.

@deeptiboddapati I'll follow up on your comments in a subsequent comment.

#10 @boonebgorges
8 years ago

In 38292:

Allow attachment taxonomies to be fetched as objects.

By adding the $output parameter to get_attachment_taxonomies(), the
function signature matches that of get_object_taxonomies(). The change
also allows for more consistent behavior when passing output=objects
to get_object_taxonomies() for the 'attachment' object type, since
the $output parameter is now passed through the function stack.

Props codemovement.pk.
See #37368.

#11 @boonebgorges
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

@deeptiboddapati Thank you for the detailed comments. Some responses:

This circular dependency should be removed

Fetching taxonomies for an attachment is special, because it's possible to register taxonomies for specific attachment types: attachment:image, attachment:image/jpeg, and so forth. What get_attachment_taxonomies() does (this is the "unneeded text parsing" you mention) is to assemble a list of all taxonomy object types that match a given attachment. These types are then passed to get_object_taxonomies(), which is a general function for fetching taxonomies by object types. The fact that you can pass an 'attachment' to get_object_taxonomies() and have it work is merely a developer convenience. So the functions play different roles, and the "circular dependency" is not a bug but a feature that allows us to minimize the amount of duplicated code.

You did some research into the various parameter values and return values of get_object_taxonomies(). I'm not sure I understand the chart completely, but it does look like you've found a couple of bugs related to PHP notices and mixed post types. These deserve one or two separate bug tickets. You also point out that the documentation is not clear on what the $object parameter accepts. If you have a suggestion about how this could be worded better, please do open a ticket with a suggestion.

#12 follow-up: @swissspidy
8 years ago

This breaks on HHVM because array_unique() does a string comparison and cannot convert a stdClass object to a string.

Could probably be solved by introducing WP_Taxonomy in #36224 with a __toString() method.

#13 in reply to: ↑ 12 @boonebgorges
8 years ago

Replying to swissspidy:

This breaks on HHVM because array_unique() does a string comparison and cannot convert a stdClass object to a string.

Could probably be solved by introducing WP_Taxonomy in #36224 with a __toString() method.

Thanks, @swissspidy. __toString() would suppress the error. But in this case, running array_unique() doesn't do anything when output=objects, because array_merge() will cause duplicate indexes to overwrite each other. So let's save the cycles when we're fetching objects.

#14 @boonebgorges
8 years ago

In 38437:

Remove unnecessary uniqueness check in get_attachment_taxonomies().

Running the taxonomy array through array_unique() is unnecessary
when the function returns objects, because the associative keys already
ensure uniqueness.

This also fixes a bug when running get_attachment_taxonomies() in
HHVM, which doesn't like casting objects to strings for the purposes
of array_unique().

Props swissspidy.
See #37368.

Note: See TracTickets for help on using tickets.