#55751 closed defect (bug) (fixed)
get_the_posts_navigation() is using incorrect functions to output navigation links
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.2 | Priority: | normal |
Severity: | normal | Version: | 5.9.3 |
Component: | Posts, Post Types | Keywords: | has-patch has-unit-tests commit |
Focuses: | Cc: |
Description
The filters
next_posts_link_attributes
andprevious_posts_link_attributes
were applying to the incorrect posts navigation links.
Are you using either the latest version of WordPress, or the latest development version?
Website is using WordPress core v5.9.3.
What steps should be taken to consistently reproduce the problem?
I added a filter in my theme, using the filters next_posts_link_attributes
and previous_posts_link_attributes
, and they were applying to the incorrect posts navigation links. The filter next_posts_link_attributes
was applying to the previous nav link and vice versa.
In case it's relevant to the ticket, what is the expected output or result? What did you see instead?
On further investigation, I found the bug in wp-includes/link-template.php
in the function definition for get_the_posts_navigation()
, line 2788 and 2789, where the incorrect functions are being used for $next_link
and $prev_link
.
Change History (36)
This ticket was mentioned in PR #2725 on WordPress/wordpress-develop by michelmany.
3 years ago
#1
- Keywords has-patch added
#2
follow-up:
↓ 8
@
3 years ago
I think it's very confusing, but there are two sets of previous and next being used in this code.
One is the previous and next page of results referred to by the $paged variable.
The other is the previous and next posts regarding the date.
The posts are shown in date descending order, so prev_text defaults to 'Older Posts'. To get to older posts, you have to add 1 to the $paged variable, which is done by the get_next_posts_link
path of functions.
And vice versa for next.
This ticket was mentioned in PR #2792 on WordPress/wordpress-develop by geist-ahnen.
3 years ago
#3
- Keywords has-unit-tests added
Trac ticket: 55751(https://core.trac.wordpress.org/ticket/55751)
This PR adds tests for the get_posts_navigation
function and invalidates the patch submitted in the above mentioned ticket as the current functionality is correct.
#4
follow-up:
↓ 7
@
3 years ago
- Resolution set to invalid
- Status changed from new to closed
The current functionality of get_posts_navigation
is correct. I think the main confusion comes from effect the default descending order has when querying posts.
As mentioned in the documentation:
https://developer.wordpress.org/reference/functions/get_next_posts_link/
Because post queries are usually sorted in reverse chronological order, get_next_posts_link()
usually points to older entries (toward the end of the set) and
get_previous_posts_link()
usually points to newer entries (toward the beginning of the set).
I've created a collection of tests that proves that the current functionality is correct and can be seen here: https://github.com/WordPress/wordpress-develop/pull/2792
This ticket was mentioned in Slack in #core-test by geisthanen. View the logs.
3 years ago
geist-ahnen commented on PR #2725:
3 years ago
#6
This PR can be closed as discussed here: https://core.trac.wordpress.org/ticket/55751
#7
in reply to:
↑ 4
@
3 years ago
- Resolution invalid deleted
- Status changed from closed to reopened
Replying to geisthanen:
The current functionality of
get_posts_navigation
is correct. I think the main confusion comes from effect the default descending order has when querying posts.
As mentioned in the documentation:
https://developer.wordpress.org/reference/functions/get_next_posts_link/
Because post queries are usually sorted in reverse chronological order,
get_next_posts_link()
usually points to older entries (toward the end of the set) and
get_previous_posts_link()
usually points to newer entries (toward the beginning of the set).
I've created a collection of tests that proves that the current functionality is correct and can be seen here: https://github.com/WordPress/wordpress-develop/pull/2792
Thank you for taking the time to review my ticket. The issue is not with the function. The function works as expected. The issue is with the filters used inside the function: next_posts_link_attributes
and previous_posts_link_attributes
. It's misleading that next_posts_link_attributes
applies to the link pointing to previous posts and previous_posts_link_attributes
applies to the link pointing to next posts.
Here is my use case scenario in steps to pinpoint the bug I was having at the time.
- Add pagination links to index.php:
the_posts_navigation( array( 'prev_text' => 'Prev', 'next_text' => 'Next', ) );
- Add filter for
next_posts_link_attributes
in functions.php:
add_filter( 'next_posts_link_attributes', function () { return 'data-pagination="next-posts"'; } );
- Add filter for
previous_posts_link_attributes
in functions.php:
add_filter( 'previous_posts_link_attributes', function () { return 'data-pagination="prev-posts"'; } );
This is the HTML output from the code above, which shows the attributes are not applied to the correct link.
<nav class="navigation posts-navigation" aria-label="Posts"> <h2 class="screen-reader-text">Posts navigation</h2> <div class="nav-links"> <div class="nav-previous"><a href="http://website.test/2020/page/3/" data-pagination="next-posts">Prev</a></div> <div class="nav-next"><a href="http://website.test/2020/" data-pagination="prev-posts">Next</a></div> </div> </nav>
#8
in reply to:
↑ 2
@
3 years ago
Replying to joyously:
I think it's very confusing, but there are two sets of previous and next being used in this code.
One is the previous and next page of results referred to by the $paged variable.
The other is the previous and next posts regarding the date.
The posts are shown in date descending order, so prev_text defaults to 'Older Posts'. To get to older posts, you have to add 1 to the $paged variable, which is done by the
get_next_posts_link
path of functions.
And vice versa for next.
Thank you for providing insight. However, the issue is not the function. The issue is with the filters used in the function.
#9
@
3 years ago
I understand this is confusing and misleading but the logic behind next and previous is inverse if you consider we are looking everything from a descending optic.
get_next_posts_link()
usually points to older entries (toward the end of the set), hence in order to navigate back the label should be Previous Posts.
get_previous_posts_link()
usually points to newer entries (toward the beginning of the set). hence in order to navigate forward the label should be Newer Posts.
The filters you mentioned are applied to those functions.
You can see it here: https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/link-template.php#L2788-L2789
#10
follow-ups:
↓ 11
↓ 16
↓ 21
@
3 years ago
- Milestone changed from Awaiting Review to 6.1
Hi there, welcome to WordPress Trac! Thanks for the ticket and the PRs.
This is indeed super confusing. It makes some sense in theory, but I'm still trying to wrap my head around what would be the expected result for the code in comment:7, and I'm not 100% sure yet that there is no core bug there.
Moving to the milestone to add some unit tests and maybe improve documenation for these functions and filters.
#11
in reply to:
↑ 10
@
3 years ago
Replying to SergeyBiryukov:
Hi there, welcome to WordPress Trac! Thanks for the ticket and the PRs.
This is indeed super confusing. It makes some sense in theory, but I'm still trying to wrap my head around what would be the expected result for the code in comment:7, and I'm not 100% sure yet that there is no core bug there.
Moving to the milestone to add some unit tests and maybe improve documenation for these functions and filters.
The expected result for the code output is:
<nav class="navigation posts-navigation" aria-label="Posts"> <h2 class="screen-reader-text">Posts navigation</h2> <div class="nav-links"> <div class="nav-previous"><a href="http://website.test/2020/page/3/" data-pagination="prev-posts">Prev</a></div> <div class="nav-next"><a href="http://website.test/2020/" data-pagination="next-posts">Next</a></div> </div> </nav>
Even if the_posts_navigation()
switches the order like in geisthanen's explanation, the attributes passed in the filter should stick to their anchor elements based on the name of the parameters in the accepted array for get_the_posts_navigation()
. It may help to clarify in the documentation that because of the reverse chronological order, developers may prefer to directly use get_next_posts_link()
and get_previous_posts_link()
instead of the_posts_navigation()
and get_the_posts_navigation()
.
#12
follow-up:
↓ 13
@
3 years ago
@jongycastillo I see your point now. Tomorrow I will try to reproduce it and do some tests to see the results.
#13
in reply to:
↑ 12
@
3 years ago
Replying to geisthanen:
@jongycastillo I see your point now. Tomorrow I will try to reproduce it and do some tests to see the results.
Thanks! Your explanations helped me clarify the expected results I was looking for too.
This ticket was mentioned in Slack in #core by geisthanen. View the logs.
3 years ago
creative-andrew commented on PR #2792:
3 years ago
#15
@costdev I've taken care of your requested revisions.
#16
in reply to:
↑ 10
@
3 years ago
Replying to SergeyBiryukov:
Hi there, welcome to WordPress Trac! Thanks for the ticket and the PRs.
This is indeed super confusing. It makes some sense in theory, but I'm still trying to wrap my head around what would be the expected result for the code in comment:7, and I'm not 100% sure yet that there is no core bug there.
Moving to the milestone to add some unit tests and maybe improve documentation for these functions and filters.
IMO, this is working correctly but the variables names and documentation don't help to make this clear. Is there anyway we can improve this?
#17
follow-up:
↓ 18
@
2 years ago
- Milestone changed from 6.1 to 6.2
With WP 6.1 RC 1 scheduled tomorrow (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone.
Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next few hours, please feel free to move this ticket back to milestone 6.1.
#18
in reply to:
↑ 17
@
2 years ago
Thanks @audrasjb.
I will work on this then for 6.2
Replying to audrasjb:
With WP 6.1 RC 1 scheduled tomorrow (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone.
Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next few hours, please feel free to move this ticket back to milestone 6.1.
This ticket was mentioned in PR #3808 on WordPress/wordpress-develop by @geisthanen.
2 years ago
#20
Trac ticket: https://core.trac.wordpress.org/ticket/55751
This PR adds tests for next_posts_link_attributes
& previous_posts_link_attributes
filters and invalidates the patch submitted in the above-mentioned ticket as the current functionality is correct.
#21
in reply to:
↑ 10
@
2 years ago
Replying to SergeyBiryukov:
Hi there, welcome to WordPress Trac! Thanks for the ticket and the PRs.
This is indeed super confusing. It makes some sense in theory, but I'm still trying to wrap my head around what would be the expected result for the code in comment:7, and I'm not 100% sure yet that there is no core bug there.
Moving to the milestone to add some unit tests and maybe improve documenation for these functions and filters.
@SergeyBiryukov as you were having doubts based on your comment above, I've added some tests exclusively for the filters:
https://github.com/WordPress/wordpress-develop/pull/3808
What I can tell is that the filters are correctly applied to their links
<a href="http://example.org/?paged=3" data-attribute='next'>Older posts</a>
<a href="http://example.org/" data-attribute='previous'>Newer posts</a>
Nevertheless as mentioned correctly by @jongycastillo, the markup output by
the_posts_navigation( array( 'prev_text' => 'Prev', // the default value here is Older posts 'next_text' => 'Next', // the default value here is Newer posts ) );
creates a really confusing markup:
<nav class="navigation posts-navigation" aria-label="Posts"> <h2 class="screen-reader-text">Posts navigation</h2> <div class="nav-links"> <div class="nav-previous"> <a href="http://example.org/?paged=3" data-attribute='next'>Prev</a> </div> <div class="nav-next"> <a href="http://example.org/" data-attribute='previous'>Next</a> </div> </div> </nav>
Even tho the documentation makes mention of this https://developer.wordpress.org/reference/functions/get_next_posts_link/, maybe there is something to be amended to avoid confusion.
Here we have the original intention by swapping them in the first place: https://core.trac.wordpress.org/ticket/29808
I would really appreciate your review and feedback :)
This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.
2 years ago
#23
follow-up:
↓ 24
@
2 years ago
- Keywords changes-requested added
This ticket was discussed during the recent bug scrub. It looks like it's unlikely that work will be done on this during the 6.2 cycle.
@geisthanen Can you please address the review feedback on PR? @costdev, do you have the capacity to review this PR?
#24
in reply to:
↑ 23
@
2 years ago
Replying to mukesh27:
This ticket was discussed during the recent bug scrub. It looks like it's unlikely that work will be done on this during the 6.2 cycle.
@geisthanen Can you please address the review feedback on PR? @costdev, do you have the capacity to review this PR?
Thanks, @mukesh27.
I have taken care of the review of the PR. Both unit tests are ready to go. One has been already approved by @costdev. It would be cool if they can be included in the upcoming 6.2.0 release.
@audrasjb commented on PR #3808:
2 years ago
#25
@costdev change proposals committed :)
@geisthanen commented on PR #2792:
2 years ago
#26
@costdev I have taken care of your comments :)
#27
@
2 years ago
- Keywords commit added; changes-requested removed
PR 2792 looks ready for commit
consideration to me.
#28
@
2 years ago
- Owner set to audrasjb
- Resolution set to fixed
- Status changed from reopened to closed
In 55429:
@audrasjb commented on PR #2792:
2 years ago
#29
committed in https://core.trac.wordpress.org/changeset/55429
@audrasjb commented on PR #2725:
2 years ago
#30
closed in favor of #3808.
@audrasjb commented on PR #3808:
2 years ago
#31
Committed in https://core.trac.wordpress.org/changeset/55429
@audrasjb commented on PR #3808:
2 years ago
#32
Reopening
@audrasjb commented on PR #3808:
2 years ago
#33
Reopening
#34
@
2 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening as PR3808 was missed.
@SergeyBiryukov commented on PR #3808:
2 years ago
#36
Thanks for the PR! Merged in r55442 with a few minor edits:
- We don't usually have a separate test class just for filters in a function, so this can be a general test class for the function.
- The
@covers
tags should mention the function name, not the filter name. - Instead of a closure,
MockAction::get_call_count()
can be used as a quick way to check that the filter is applied.
The filters next_posts_link_attributes and previous_posts_link_attributes were applying to the incorrect posts navigation
Trac ticket: https://core.trac.wordpress.org/ticket/55751