WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 6 months ago

Last modified 2 months ago

#17807 closed defect (bug) (fixed)

get_adjacent_post() doesn't work with custom taxonomies

Reported by: avaly Owned by: nacin
Milestone: 3.8 Priority: normal
Severity: normal Version: 3.1.3
Component: Taxonomy Keywords: has-patch needs-refresh needs-codex
Focuses: Cc:

Description

If you use next_post_link('%link', '%title', true) or previous_post_link('%link', '%title', true) to get the adjacent post for a custom post type which has a taxonomy assigned to it, it doesn't work as intended.

The bug traces back to get_adjacent_post(). If the $in_same_cat parameter is true, then the SQL query built to get the posts are hardcoded using the default 'category' taxonomy. Instead it should allow a custom taxonomy as a parameter and use it in the queries.

Example:

Custom Post Type: product

Custom Taxonomy: color

SQL produced by get_adjacent_post() when calling next_post_link('%link', '%title', true):

SELECT p.* 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 AND tt.taxonomy = 'category' AND tt.term_id IN () WHERE p.post_date > '2011-06-14 19:37:08' AND p.post_type = 'product' AND p.post_status = 'publish' AND tt.taxonomy = 'category' ORDER BY p.post_date ASC LIMIT 1

Attachments (14)

17807.patch (1.7 KB) - added by billerickson 3 years ago.
17807.2.patch (9.0 KB) - added by billerickson 3 years ago.
17807.3.patch (12.7 KB) - added by billerickson 3 years ago.
17807.4.patch (12.9 KB) - added by SergeyBiryukov 3 years ago.
17807.5.diff (19.2 KB) - added by ethitter 2 years ago.
17807.6.patch (19.3 KB) - added by ethitter 20 months ago.
Refreshed for 3.5
17807.5.patch (19.6 KB) - added by DrewAPicture 12 months ago.
Refresh at r24075
17807.7.patch (19.6 KB) - added by ethitter 9 months ago.
Refreshed at r24829
17807-tests.patch (3.0 KB) - added by ethitter 8 months ago.
17807.8.patch (22.7 KB) - added by ethitter 6 months ago.
17807-tests.2.patch (3.1 KB) - added by ethitter 6 months ago.
17807.9.patch (25.8 KB) - added by ethitter 6 months ago.
17807.10.patch (28.1 KB) - added by ethitter 6 months ago.
17807.11.patch (28.2 KB) - added by ethitter 6 months ago.

Download all attachments as: .zip

Change History (70)

comment:1 avaly3 years ago

  • Version set to 3.1.3

comment:2 scribu3 years ago

As a workaround, see my Smarter Navigation plugin.

comment:3 avaly3 years ago

Thanks for the suggestion, scribu. Though for such a simple task as getting these links I'd prefer a clean solution without using cookies. As a temporary workaround I used direct SQL queries to get the adjacent custom posts. I hope this bug get fixed though.

comment:4 wpmuguru3 years ago

  • Cc wpmuguru added

comment:5 billerickson3 years ago

  • Cc billerickson added

comment:6 griffinjt3 years ago

  • Cc thomasgriffinmedia@… added

billerickson3 years ago

comment:7 billerickson3 years ago

  • Keywords has-patch needs-testing added

This patch let's you use the taxonomy slug as the value of $in_same_cat instead of just true. If your value for $in_same_cat is a taxonomy, it will use that taxonomy; else it uses 'category' (so setting $in_same_cat to true still works).

So to use the taxonomy 'type', use:

previous_post_link( '%link', '← Previous', 'type' );

comment:8 nacin3 years ago

  • Keywords needs-testing removed

Interesting idea. Not a huge fan of overloading arguments beyond their original meanings. Might be worth it to just add a new taxonomy argument. Especially since there's also an exclude_categories parameter that we could not easily convert over to terms. (It's possible for in_same_cat to be false but categories to still be excluded -- overloading the argument would prevent excluding terms of another taxonomy.)

Either way:

  • The function argument will need new docs (and a new name, if we overload it). That includes in functions that use get_adjacent_post().
  • $taxonomy should go right into the string, no need to break out of double-quoted strings in PHP.
  • Even better, $taxonomy should get $wpdb->prepare() treatment.

Would like to see an approach that uses a new argument, I think.

billerickson3 years ago

comment:9 billerickson3 years ago

  • Keywords needs-testing added
  • Added a new argument called $taxonomy with a default value of 'category'
  • Fixed the strings (no breaking out of double-quotes)
  • Added the $taxonomy argument to all the other functions that use get_adjacent_post()
  • Updated documentation

I also considered changing $in_same_cat to $in_same_tax, and $excluded_categories to $excluded_terms, but I wasn't sure if it was worth all the edits just to rename variables - it doesn't add any additional functionality.

I tried testing the $excluded_categories but I couldn't get them to work in trunk, without my modifications. I might be doing something wrong (I've never used that feature before). I created three categories: Category 1, Category 2, and Exclude. I created two posts in Cat 1 and Cat 2 individually, and one post with all three. When I do the below code with true or false (5 = term_id of "exclude"), the next link still leads to the post in the exclude category:

<?php next_post_link( '%link', 'Next', true, '5' ); ?>

comment:10 billerickson3 years ago

  • Cc billerickson removed

comment:11 billerickson3 years ago

  • Cc bill.erickson@… added

comment:12 billerickson3 years ago

  • Keywords dev-feedback added

In #17673 the $excluded_categories bug was identified here [1557] and fixed.

Nacin (or anyone else), should I update my code to include the #17673 since it's marked as commit, or should I keep it separate?

I've written a plugin that incorporates this patch because I needed it on a client project. I'd like to publicly release it to help with testing this patch.

comment:13 SergeyBiryukov3 years ago

  • Milestone changed from Awaiting Review to 3.3

comment:14 scribu3 years ago

A more generic approach would be to make get_adjacent_post() use WP_Query somehow so that you could use any query var that WP_Query accepts.

billerickson3 years ago

comment:15 billerickson3 years ago

I've updated my patch to work with the updates from [18478]. I've also created a plugin that simulates this patch for testing, and will post that once it's approved in the repo. It allows you to use functions like be_previous_post_link() and be_get_adjacent_post().

Scribu, I personally don't think it's worth rebuilding get_adjacent_post() and all the related functions to accept query vars other than taxonomy. I think this improvement is needed, and if it other query_vars in get_adjacent_post() become needed then we can generalize it.

comment:17 scribu3 years ago

Marked #13096 as duplicate.

comment:18 scribu3 years ago

  • Keywords needs-patch added; has-patch needs-testing removed

If not using WP_Query, we should at least use get_tax_sql().

Last edited 3 years ago by scribu (previous) (diff)

comment:19 scribu3 years ago

  • Keywords dev-feedback removed

comment:20 scribu3 years ago

Related: #15959

SergeyBiryukov3 years ago

comment:21 SergeyBiryukov3 years ago

Updated the patch with Nacin's suggestions in ticket:13096:6 about $wpdb->prepare() and checking for taxonomy existence.

comment:22 scribu3 years ago

Sergey, if we were to use get_tax_sql(), we wouldn't need to worry about escaping, taxonomy existance etc.

ethitter2 years ago

comment:23 ethitter2 years ago

  • Cc ehitter@… added
  • Keywords has-patch added; needs-patch removed

Building on what SergeyBiryukov had started, I cleaned up his usage of $wpdb->prepare().

In get_boundary_post(), I also changed how the get_posts() query arguments were built for situations where $excluded_terms was empty string that was converted to an array with single key whose value was 0.

Since get_boundary_post_rel_link() has been deprecated since SergeyBiryukov wrote his patch, I left it alone.

Finally, I cleaned up the functions I modified per the coding standards.

comment:24 ethitter2 years ago

Forgot to mention that I explored using get_tax_sql() as scribu suggested, but because of how the get_adjacent_post() function builds its query, the usefulness of get_tax_sql() was too limited to make it worthwhile.

comment:25 nacin2 years ago

  • Keywords 3.4-early added
  • Milestone changed from 3.3 to Future Release
  • Owner set to nacin
  • Status changed from new to reviewing

Nice. I meant to punt this out of the 3.3 milestone a while ago, so let's move this to 3.4-early. otto42 and I nearly tackled this a few weeks ago as we needed it for a project, but we realized it was just way too much churn (even minus the coding standards changes) for 3.3 at this point.

I discussed possibly leveraging WP_Query with ethitter at #wcphilly and now I'm curious if it's possible. Can anyone see something that would prevent us from dropping our direct queries here, beyond needing to eliminate the existing filters on the SQL?

comment:26 ethitter2 years ago

Being somewhat invested in this already, I'll work on the WP_Query changes discussed at #wcphilly. If I can find something else 3.3-related, I'll wait on this until after that's out, otherwise I'll work on this in the meantime.

comment:27 sabreuse2 years ago

  • Cc sabreuse@… added

comment:28 landwire2 years ago

  • Cc sascha@… added

comment:29 ZaneMatthew2 years ago

  • Cc zanematthew@… added

comment:30 stephenh198822 months ago

  • Cc contact@… added

ethitter20 months ago

Refreshed for 3.5

comment:31 ethitter20 months ago

  • Keywords needs-testing added; 3.4-early removed

comment:33 mikkelbreum20 months ago

  • Cc mikkel@… added

comment:35 husobj14 months ago

  • Cc ben@… added

comment:36 jjeaton12 months ago

  • Cc jjeaton added

comment:37 sporkme12 months ago

  • Cc sporkme added

comment:38 follow-up: sporkme12 months ago

Last version of the patch fails on 3.5.1. Can we have an updated patch while waiting?

How are others working around this when using custom post types and custom taxonomies?

comment:39 SergeyBiryukov12 months ago

  • Keywords needs-refresh added

DrewAPicture12 months ago

Refresh at r24075

comment:40 DrewAPicture12 months ago

  • Keywords needs-refresh removed

comment:41 in reply to: ↑ 38 ethitter12 months ago

Replying to sporkme:

How are others working around this when using custom post types and custom taxonomies?

There are filters in get_adjacent_post(), the underlying function that powers the adjacent post link functions. See http://core.trac.wordpress.org/browser/tags/3.5.1/wp-includes/link-template.php#L1173.

Perhaps 3.7 will be the release we can land this for. Unfortunately, until #18694 lands, we can't replace the direct SQL queries with WP_Query.

comment:42 geminorum11 months ago

The dilemma would have been inverted, if a simple filter could override the entire query making process.

Also there are problems ordering only by post_date. we had to rewrite all the functions to just order custom post types by anything beside date.

Either way please don't forget to pass $taxonomy through all the filters.

comment:43 RubenBristian9 months ago

#24794 was marked as a duplicate.

ethitter9 months ago

Refreshed at r24829

comment:44 ethitter9 months ago

17807.7.patch is updated for WP 3.6 and applies cleanly as of r24829. All is well in my testing, and I'm working on unit tests to confirm this now.

comment:45 wonderboymusic8 months ago

  • Keywords needs-unit-tests added
  • Milestone changed from Future Release to 3.7

ethitter8 months ago

comment:46 ethitter8 months ago

I worked on tests for this, but either I'm doing something wrong or my PHPUnit install is borked; this is my first attempt at writing unit tests, so the former is quite likely. 17807-tests.patch reflects how far I got.

comment:47 nacin7 months ago

  • Keywords needs-refresh added; needs-testing needs-unit-tests removed
  • Milestone changed from 3.7 to Future Release

Patch needs to be refreshed. (And is preferably made against develop.svn, with the tests in the same patch.) I'm happy to help shore the tests up for this one, and get this in probably into early 3.8.

ethitter6 months ago

ethitter6 months ago

ethitter6 months ago

comment:48 ethitter6 months ago

17807.9.patch is refreshed against develop.svn and combines the patch and tests.

The tests are passing now as well. I had get_post() where I meant get_posts(). :(

Last edited 6 months ago by ethitter (previous) (diff)

ethitter6 months ago

ethitter6 months ago

comment:49 nacin6 months ago

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

In 25959:

Add a $taxonomy argument to each of the adjacent post functions.

Each took an array of category (IDs) when to search. Those can now be term IDs and each function now has $taxonomy = 'category' as an optional argument.

Functions affected: get_previous_post(), get_next_post(), get_adjacent_post(), get_adjacent_post_rel_link(), adjacent_posts_rel_link(), next_post_rel_link(), prev_post_rel_link(), get_boundary_post(), get_previous_post_link(), previous_post_link(), get_next_post_link(), next_post_link(), get_adjacent_post_link(), adjacent_post_link().

props ethitter.
finally fixes #17807.

comment:50 helen6 months ago

  • Milestone changed from Future Release to 3.8

comment:51 follow-ups: husobj3 months ago

Just a note, the documentation for affected functions needs updating in the codex.

comment:52 in reply to: ↑ 51 ; follow-up: DrewAPicture3 months ago

  • Keywords needs-codex added

Replying to husobj:

Just a note, the documentation for affected functions needs updating in the codex.

FYI, the Codex is a wiki, and anyone with a WordPress.org account can edit it, even yourself if you're so inclined :)

comment:53 in reply to: ↑ 51 ZaneMatthew3 months ago

Replying to husobj:

Just a note, the documentation for affected functions needs updating in the codex.

I made a very minor adjustment, and added it.

comment:54 in reply to: ↑ 52 husobj3 months ago

Replying to DrewAPicture:

Replying to husobj:

Just a note, the documentation for affected functions needs updating in the codex.

FYI, the Codex is a wiki, and anyone with a WordPress.org account can edit it, even yourself if you're so inclined :)

Thanks DrewAPicture, yes I did know it's a wiki.
I just didn't have any time to make any updates yesterday and wanted to make a note so it didn't get forgotten :)

comment:55 follow-up: bucketpress2 months ago

Does this solve the issue where we need to get adjacent posts based on post order other than post date?

comment:56 in reply to: ↑ 55 ethitter2 months ago

Replying to bucketpress:

Does this solve the issue where we need to get adjacent posts based on post order other than post date?

No, but #26937 addresses that by switching to WP_Query. Currently only the filters available within get_adjacent_post() can be used to change the order.

Note: See TracTickets for help on using tickets.