#37368 closed defect (bug) (fixed)
get_object_taxonomies() ignores output type argument for attachments
Reported by: |
|
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)
Change History (15)
#1
@
9 years ago
- Keywords needs-patch needs-unit-tests added
- Milestone changed from Awaiting Review to Future Release
- Version changed from trunk to 3.0
#3
@
9 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
@
9 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.
9 years ago
#6
@
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?
#9
@
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.
#11
@
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:
↓ 13
@
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
@
8 years ago
Replying to swissspidy:
This breaks on HHVM because
array_unique()
does a string comparison and cannot convert astdClass
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.
get_attachment_taxonomies()
was added toget_object_taxonomies()
in [7520]. The output argument forget_object_taxonomies()
was added in [14479].