WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 6 months ago

Last modified 3 months ago

#22112 closed defect (bug) (fixed)

get_adjacent_post excluded_categories isn't excluded

Reported by: PatNarciso Owned by: wonderboymusic
Milestone: 4.0 Priority: normal
Severity: normal Version: 3.8.1
Component: Posts, Post Types Keywords: has-patch
Focuses: template Cc:

Description (last modified by SergeyBiryukov)

The category I expect to be excluded in $excluded_categories, is not excluded.

With get_adjacent_post,
Assuming $in_same_cat = TRUE
and $excluded_categories = 'ID_OF_CATEGORY'

$cat_array gets built with the ID's of categories it should include. cool.

But, when $excluded_categories contains an ID that $cat_array contains, the ID is removed from the $excluded_categories by array_diff() (link-template.php:1155).

So: the category I expect to be excluded in $excluded_categories, is not excluded.

My resolution: I remove 1154-1157.

if ( ! empty( $cat_array ) ) {
 $excluded_categories = array_diff($excluded_categories, $cat_array);
 $posts_in_ex_cats_sql = '';
}

Attachments (5)

22112.patch (7.1 KB) - added by jessepollak 12 months ago.
22112.patch
22112.2.patch (6.1 KB) - added by jessepollak 12 months ago.
22112.2.patch
22112.diff (6.6 KB) - added by kovshenin 9 months ago.
22112.3.patch (1.1 KB) - added by betzster 6 months ago.
22112.4.patch (1.2 KB) - added by pento 6 months ago.

Download all attachments as: .zip

Change History (31)

comment:1 @SergeyBiryukov2 years ago

  • Component changed from General to Template
  • Description modified (diff)

comment:2 @nacin13 months ago

  • Component changed from Template to Posts, Post Types
  • Focuses template added

comment:3 @jessepollak12 months ago

  • Keywords 2nd-opinion added
  • Version set to 3.8.1

Thanks for the bug report PatNarciso (and a belated-congrats on your first one!), sorry you never got a response!

I just confirmed that this bug is still around in trunk at revision 27076 (though the code has changed since he made the bug report, so things look a little different).

To recreate:

  1. Create a three new posts Post 1, Post 2, and Post 3.
  2. Give both Post 1 and Post 2 a category of "bad" and give both Post 1 and Post 3 a category of "good"
  3. Run
global $post;
$post = POST_1_ID;
$adjacent_post = get_adjacent_post(
    true, // in the same category
    'ID_OF_BAD_CATEGORY', // exclude the test category
    false // should be the next rather than previous post
);

Expected:

This should return Post 3 because, while Post 2 is in the same category as Post 1, it has the excluded_category "bad."

Actual

It returns Post 2 which has the excluded category "bad".

Why

This happens because, as PatNarciso reports, the $excluded_terms array is array_diff'd with the $term_array of acceptable terms. This means that any category that the both the post has and is excluded, will be included.

I think this fix should be as easy as just removing the array_diff line at 1167 — it doesn't seem to serve any purpose but to create this error case. That said, this may not be an error case and this fix will have an affect on the "next" link of a whole lot of blogs, so I'm not sure it's something we want to patch.

I'll defer to core members on whether this is something we want to fix — if so, I'd be happy to write the quick patch for it.

Thanks again PatNarciso!

comment:4 @jessepollak12 months ago

  • Keywords has-patch dev-feedback added

So, I started off by implementing the solution above, but quickly realized that it was insufficient. Removing that array_diff leaves the possibility that we'll have a query that selects posts which are both IN and NOT IN a given term. While discovering the problem with that solution, I also discovered that any time both same_term and excluded_terms are used in get_adjacent_post, we generate a query with both an IN and a NOT IN statement — a little redundant.

In the patch below, I take the following approach:

  1. I get the {{${excluded_terms}}} and $same_term_array.
  2. I call $same_term_array = array_diff($same_term_array, $excluded_terms), which removes all excluded terms from the $same_term_array
  3. If the $same_term_array is not empty, I add an AND IN for the $same_term_array
  4. If the $same_term_array is empty and the $excluded_terms is not empty, I add a AND NOT IN for the $excluded_terms

Since both IN and NOT IN are exclusive, either step (4) or step (5) will ensure the adjacent post has the correct terms.

I added a unit test to demonstrate this failure case and verify the patch. I also changed the $term_array to $same_term_array because it's much clearer.

NOTE: this change does affect the get_$adjacent_post_where filter, but I don't think it should be a problem — would appreciate feedback on this.

Last edited 12 months ago by jessepollak (previous) (diff)

@jessepollak12 months ago

22112.patch

@jessepollak12 months ago

22112.2.patch

comment:5 @jessepollak12 months ago

Just added a second patch because I realized I'd mistakenly replaced $term_array in a few places that I didn't mean to. Refer to 22112.2.patch please.

comment:6 @ircbot12 months ago

This ticket was mentioned in IRC in #wordpress-dev by jessepollak. View the logs.

comment:7 @SergeyBiryukov10 months ago

  • Milestone changed from Awaiting Review to 4.0

Just got bitten by this.

comment:8 @jessepollak10 months ago

I sort of tabled this ticket because I presumed it would be fixed in #26937, but since that's been reverted and moved to future release, I think this should be added. Is there anything else I should do to help this get into 4.0?

Last edited 10 months ago by jessepollak (previous) (diff)

@kovshenin9 months ago

comment:9 @kovshenin9 months ago

Hi! I think the query for exclusion is wrong here. The regular NOT IN (5) will look for at least one row that doesn't match 5, and it'll find plenty if the post has more than one category even if there's a 5. I think we should be reversing this in a subquery, like we do with tax queries. Took a stab in 22112.diff.

@jessepollak fyi the unit tests in your patch use wp_set_object_terms with append=false, which means every next call on the same object will overwrite the previous set of categories, rather than append.

comment:10 follow-up: @jessepollak9 months ago

Heh, I guess I should leave the actual patching up to the professionals. That looks much better, thanks :)

comment:11 in reply to: ↑ 10 @kovshenin9 months ago

Replying to jessepollak:

Heh, I guess I should leave the actual patching up to the professionals.

Not really. I actually took your patch to start with which helped a lot, and simply ended up changing a lot of things and reverting some, and when I said "wrong" I was referring to the current code in core, not your patch. You should definitely not feel discouraged. Thank you for your hard work!

comment:12 @jessepollak9 months ago

:thumbsup:

comment:13 @jessepollak9 months ago

For future reference, would you mind pointing me to a place where that subquery strategy is used elsewhere? I feel like it's those kind of small style/pattern things that often elude new contributors :)

comment:14 @kovshenin9 months ago

Sure, here's the tax query code I was referring to.

comment:15 @jessepollak9 months ago

Ah, thanks. Is that done explicitly to avoid situations like this where IN and NOT IN conflict or is there another motive?

comment:16 @DrewAPicture8 months ago

  • Keywords commit added

22112.diff still applies and unit tests pass.

comment:17 @wonderboymusic8 months ago

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

In 29248:

In get_adjacent_post(), make $excluded_terms work as expected.

Adds unit tests.
Props jessepollak, kovshenin.
Fixes #22112.

comment:18 @SergeyBiryukov7 months ago

#29050 was marked as a duplicate.

@betzster6 months ago

comment:19 @betzster6 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I don't think we need a subquery here. Excluding the $excluded_terms from $term_array should be enough to solve the original issue. We do need to check that $term_array is valid (it exists and is not an error) before manipulating it though.

22112.3.patch fixes this.

Currently in trunk:

mysql> EXPLAIN SELECT p.ID FROM wp_posts AS p INNER JOIN wp_term_relationships AS tr ON p.ID = tr.object_id INNER JOIN wp_term_taxonomy tt ON tr.term_taxonomy_id = tt.term_taxonomy_id WHERE p.post_date < '2014-08-26 14:01:03' AND p.post_type = 'post' AND p.post_status = 'publish' AND tt.taxonomy = 'category' AND tt.term_id IN (7386,193894091,828) AND p.ID NOT IN ( SELECT object_id FROM wp_term_relationships WHERE term_taxonomy_id IN (22006310) ) ORDER BY p.post_date DESC LIMIT 1;
+----+--------------------+-----------------------+-----------------+-----------------------------------+------------------+---------+-------------------------------+------+----------------------------------------------+
| id | select_type        | table                 | type            | possible_keys                     | key              | key_len | ref                           | rows | Extra                                        |
+----+--------------------+-----------------------+-----------------+-----------------------------------+------------------+---------+-------------------------------+------+----------------------------------------------+
|  1 | PRIMARY            | tt                    | range           | PRIMARY,term_id_taxonomy,taxonomy | term_id_taxonomy | 106     | NULL                          |    3 | Using where; Using temporary; Using filesort |
|  1 | PRIMARY            | tr                    | ref             | PRIMARY,term_taxonomy_id          | term_taxonomy_id | 8       | wordpress.tt.term_taxonomy_id |   11 | Using where                                  |
|  1 | PRIMARY            | p                     | eq_ref          | PRIMARY,type_status_date          | PRIMARY          | 8       | wordpress.tr.object_id        |    1 | Using where                                  |
|  2 | DEPENDENT SUBQUERY | wp_term_relationships | unique_subquery | PRIMARY,term_taxonomy_id          | PRIMARY          | 16      | func,const                    |    1 | Using index; Using where                     |
+----+--------------------+-----------------------+-----------------+-----------------------------------+------------------+---------+-------------------------------+------+----------------------------------------------+

With 22112.3.patch:

mysql> EXPLAIN SELECT p.ID FROM wp_posts AS p INNER JOIN wp_term_relationships AS tr ON p.ID = tr.object_id INNER JOIN wp_term_taxonomy tt ON tr.term_taxonomy_id = tt.term_taxonomy_id WHERE p.post_date < '2014-08-26 14:01:03' AND p.post_type = 'post' AND p.post_status = 'publish' AND tt.taxonomy = 'category' AND tt.term_id IN (7386,193894091,828) AND tt.taxonomy = 'category' AND tt.term_id NOT IN (22006310) ORDER BY p.post_date DESC LIMIT 1;
+----+-------------+-------+--------+-----------------------------------+------------------+---------+-------------------------------+------+----------------------------------------------+
| id | select_type | table | type   | possible_keys                     | key              | key_len | ref                           | rows | Extra                                        |
+----+-------------+-------+--------+-----------------------------------+------------------+---------+-------------------------------+------+----------------------------------------------+
|  1 | SIMPLE      | tt    | range  | PRIMARY,term_id_taxonomy,taxonomy | term_id_taxonomy | 106     | NULL                          |    3 | Using where; Using temporary; Using filesort |
|  1 | SIMPLE      | tr    | ref    | PRIMARY,term_taxonomy_id          | term_taxonomy_id | 8       | wordpress.tt.term_taxonomy_id |   11 |                                              |
|  1 | SIMPLE      | p     | eq_ref | PRIMARY,type_status_date          | PRIMARY          | 8       | wordpress.tr.object_id        |    1 | Using where                                  |
+----+-------------+-------+--------+-----------------------------------+------------------+---------+-------------------------------+------+----------------------------------------------+

comment:20 @helen6 months ago

  • Keywords 2nd-opinion commit removed

comment:21 @ircbot6 months ago

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.

comment:22 @ircbot6 months ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

@pento6 months ago

comment:23 @pento6 months ago

  • Keywords dev-feedback removed

22112.3.patch is incorrect, it's failing the test_get_adjacent_post_exclude_self_term unit test.

It's incorrect because tt.term_id is in $term_array, which can never contain items from $excluded_terms, so the NOT IN will always be true.

The only way to exclude categories is with a subquery (as we currently have), or a LEFT JOIN, as demonstrated in 22112.4.patch. This patch makes me sad, because it's harder to read than the subquery logic, and doesn't reduce the number of tables examined (so probably won't provide any performance benefits).

Unless there are clear performance benefits to 22112.4.patch (I didn't have time to benchmark it today), I'm fine with re-closing this ticket.

comment:24 @helen6 months ago

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

Let's keep pento's last patch in our minds and hearts, but this is fixed for 4.0.

comment:25 @pento3 months ago

In 30401:

Fix a MySQL warning in the test_get_adjacent_post_exclude_self_term unit test, caused by [30400].

See #21212, #22112.

comment:26 @nacin3 months ago

In 30405:

In get_adjacent_post(), $excluded_terms should check term_id rather than term_taxonomy_id.

Merges [30263] (and [30264] [30401]) to the 4.0 branch.

props boonebgorges.
fixes #29663, see #22112.

Note: See TracTickets for help on using tickets.