WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#18986 closed defect (bug) (fixed)

Link categories filter doesn't show all categories

Reported by: ocean90 Owned by: ryan
Milestone: 3.3 Priority: high
Severity: major Version: 3.3
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description

From #18983.

  • Add a new category
  • Add a new link to this new category
  • Go to all links
    • You will see the link, but you can't filter the list to show only links from the new category

Attachments (11)

18986.patch (1.2 KB) - added by SergeyBiryukov 3 years ago.
18986.2.patch (1.7 KB) - added by SergeyBiryukov 3 years ago.
18986.3.patch (2.6 KB) - added by SergeyBiryukov 3 years ago.
18986.minor-typo-fix.patch (972 bytes) - added by SergeyBiryukov 3 years ago.
18986.diff (4.1 KB) - added by nacin 3 years ago.
18986.2.diff (4.6 KB) - added by nacin 3 years ago.
18986.3.diff (4.6 KB) - added by nacin 3 years ago.
18986.4.patch (2.5 KB) - added by SergeyBiryukov 3 years ago.
18986.5.diff (4.6 KB) - added by nacin 3 years ago.
18986.6.diff (4.6 KB) - added by nacin 3 years ago.
18986.minor-typo-fix.2.patch (736 bytes) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (36)

comment:1 ocean903 years ago

Replying to SergeyBiryukov:

Replying to SergeyBiryukov:

On my install the dropdown is currently empty though, which is weird.

And on Link Categories screen, all link counts are zero. Even though there are still 7 default links in the Blogroll category.

Screenshot: http://cl.ly/B3w8

comment:2 ocean903 years ago

Seems like the count won't be updated.

Version 0, edited 3 years ago by ocean90 (next)

comment:3 Ipstenu3 years ago

See also

http://f.cl.ly/items/2u1u3R3M241w221g2r0p/link-cat.jpg

Of note: BOTH 'family' and 'my other sites' existed before any of this started. I added in a new category 'this is a test link cat' and applied it to 'Ask Ipstenu' and it would show up in the Categories column, but not the dropdown.

SergeyBiryukov3 years ago

comment:4 SergeyBiryukov3 years ago

  • Keywords has-patch added; needs-patch removed

Broken in [18932]. Links are not posts (yet).

comment:5 nacin3 years ago

As it is possible to attach taxonomies elswhere, this should probably be:

  • if ( attachment )
  • elseif ( get_post_type_object( $object_type) )
  • else this generic query

SergeyBiryukov3 years ago

comment:6 ryan3 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In [19025]:

Fix term counting for links. Props SergeyBiryukov. fixes #18986

SergeyBiryukov3 years ago

comment:7 follow-up: SergeyBiryukov3 years ago

Since $taxonomy->object_type can be an array, is it possible for a taxonomy to be attached to both a registered post type and some other object? Do we need to take that into account here (perhaps looping over $object_types)? 18986.3.patch illustrates what I'm thinking of.

comment:8 in reply to: ↑ 7 ; follow-up: duck_3 years ago

Replying to SergeyBiryukov:

Since $taxonomy->object_type can be an array, is it possible for a taxonomy to be attached to both a registered post type and some other object? Do we need to take that into account here (perhaps looping over $object_types)? 18986.3.patch illustrates what I'm thinking of.

I think there would be a problem with 18986.3.patch. The term count is updated on every loop iteration so after the function call then it would only equal the number of relationships for the last object type.

I do think there might be a problem here, but haven't tested it yet and not super familiar with the problem being solved. In [19025] we check post_type_exists( $object_type ) and then seem to assume all the objects are post types and implode them for post_type IN. I assume that $object_type is left over from the loop through $object_types and so is the last object type. What if one of the other ones is a link?

comment:9 in reply to: ↑ 8 SergeyBiryukov3 years ago

Replying to duck_:

In [19025] we check post_type_exists( $object_type ) and then seem to assume all the objects are post types and implode them for post_type IN. I assume that $object_type is left over from the loop through $object_types and so is the last object type. What if one of the other ones is a link?

Yeah, that's the exact problem I meant. Need to test this more thoroughly.

comment:10 ryan3 years ago

In [19031]:

Typo fix. Props SergeyBiryukov. see #18986

comment:11 nacin3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Perhaps we can loop through the object types to sort them into one of the three buckets (links, attachments, other post types), then run whatever queries are necessary.

nacin3 years ago

comment:12 nacin3 years ago

18986.diff takes a step back and approaches this from a different perspective.

Essentially, link_category should never be using _update_post_term_count(). In 3.2, it was using the generic handler, which was inline. In 3.3, all taxonomies by default now use _update_post_term_count(). This includes not only link_category, but all custom taxonomies. This will change how custom taxonomies attached to posts are counted, assuming they don't specify an update_count_callback. I always forget to, so perhaps this would normally be for the better for developers in general, but it is technically a backwards incompatible change that will make counts drop pretty quickly.

(Reference thus far: [18783/trunk/wp-includes/taxonomy.php], via #17548.)

So I think what we need to do is create a new generic handler, _update_generic_term_count(). This does restore the original behavior that was changed in #17548, but it doesn't do so at the expense of all other taxonomies.

Note that the patch makes this handler the default callback. I don't know if that's the best idea, given the comments in #17548. So it sounds like maybe we need to do some detection on $object_types when choosing our default callback. If it's nothing but post types, then use _update_post_term_count(), otherwise, we'll need to use the generic handler.

nacin3 years ago

comment:13 nacin3 years ago

18986.2.diff implements a conditional when choosing which "generic" handler to use. It carefully deals with attachment:* object types (1) and then checks to see if *all* object types are indeed a post type. If so, it goes with _update_post_term_count(). If not, it goes with the generic handler.

(Very rarely will a taxonomy include object types of both posts and non-posts. In this case the developer is going to certainly implement their own callback.)

(1) - Note, I think the simpler handling of object types with a colon in _update_post_term_count() is fine, given that these are only supposed to be post types.

nacin3 years ago

comment:14 nacin3 years ago

duck_ pointed out that, of course, array_search() needs to be checked against false in case the attachments index is 0. Slight tweak removes the boolean cast and does a cheeky $check_attachments = false !== $check_attachments; to cast it to a boolean.

comment:15 nacin3 years ago

s/generic/object/ would also be a good idea.

SergeyBiryukov3 years ago

comment:16 follow-up: nacin3 years ago

Sergey, I thought about the approach in 18986.4.patch as well, but the moment that else gets triggered along with either the if or the elseif, the counts get out of whack, as it isn't bound like the previous queries and a lot gets double-counted.

Granted, this situation will occur infrequently (as I argued above), but I think it would be better if we split these into two callbacks. (Especially since someone's custom taxonomy may wish to leverage the existing generic callback.)

comment:17 nacin3 years ago

Final thought, I think.

The handler-choosing logic in [18986.3.diff] fails to account for the backwards compatibility implications that all custom taxonomies were previously used to counting every post, regardless of publish status.

Should the handler-choosing logic only decide to use the post_term_count callback if the only object types are pages, posts, and attachments, and defer to the generic handler the moment a custom post type is involved? That may be the safest approach, as who knows how the CPT is implemented -- the CT counting all items, rather than just published ones, might make sense in many situations.

I think the answer to my question is that custom taxonomies should use the generic_term_count callback (as had occurred in 3.2) except for when the taxonomy is only attached to one or more of posts/pages/attachments and no custom post types, in which case post_term_count is a safe bet.

nacin3 years ago

comment:18 nacin3 years ago

18986.5.diff implements comment 17. Now, custom taxonomies tied to custom post types fall back to the generic handler. They use the post_term handler only when attached to post/page/attachment. This is for compatibility reasons.

It also fixes a parse error, as well as logic and parameter inversions in the use of array_filter() (by replacing that line all together).

comment:19 in reply to: ↑ 16 SergeyBiryukov3 years ago

Replying to nacin:

Granted, this situation will occur infrequently (as I argued above), but I think it would be better if we split these into two callbacks.

Yeah, you're right. I've attached it just in case, to show what I had in mind and failed to implement in 18986.3.patch.

nacin3 years ago

comment:20 nacin3 years ago

18986.6.diff takes a step back from .5 and provides for post status checking for custom taxonomies tied to custom post types.

comment:21 nacin3 years ago

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

In [19035]:

Separate non-post term counting from _update_post_term_count(). Introduce _update_generic_term_count(). The generic handler will be the default whenever the taxonomy is attached to an object type other than a post type (link, user). Otherwise the _update_post_term_count() handler will be the default. fixes #18986. see #17548.

comment:22 nacin3 years ago

In [19036]:

No need to specify an update_count_callback for categories or tags, as the default handler for them will now be _update_post_term_count(). see #18986, [19035].

comment:23 follow-up: nacin3 years ago

In [19037]:

Update the update_count_callback docs. see #18986.

comment:24 in reply to: ↑ 23 SergeyBiryukov2 years ago

Replying to nacin:

In [19037]:

Update the update_count_callback docs. see #18986.

Probably don't need a second "then" (see 18986.minor-typo-fix.2.patch).

comment:25 nacin2 years ago

In [19065]:

Remove duplicate word in docs. props SergeyBiryukov, fixes #18986.

Note: See TracTickets for help on using tickets.