Make WordPress Core

Opened 2 years ago

Closed 14 months ago

Last modified 14 months ago

#55751 closed defect (bug) (fixed)

get_the_posts_navigation() is using incorrect functions to output navigation links

Reported by: jongycastillo's profile jongycastillo Owned by: audrasjb's profile audrasjb
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 and previous_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.


2 years ago
#1

  • Keywords has-patch added

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

#2 follow-up: @joyously
2 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.


23 months 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: @geisthanen
23 months 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.


23 months ago

geist-ahnen commented on PR #2725:


23 months ago
#6

This PR can be closed as discussed here: https://core.trac.wordpress.org/ticket/55751

#7 in reply to: ↑ 4 @jongycastillo
23 months 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.

  1. Add pagination links to index.php:
the_posts_navigation( array(
    'prev_text' => 'Prev',
    'next_text' => 'Next',
) );
  1. Add filter for next_posts_link_attributes in functions.php:
add_filter( 'next_posts_link_attributes', function () {
    return 'data-pagination="next-posts"';
} );
  1. 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>
Last edited 23 months ago by jongycastillo (previous) (diff)

#8 in reply to: ↑ 2 @jongycastillo
23 months 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 @geisthanen
23 months 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

Last edited 23 months ago by geisthanen (previous) (diff)

#10 follow-ups: @SergeyBiryukov
23 months 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.

Last edited 23 months ago by SergeyBiryukov (previous) (diff)

#11 in reply to: ↑ 10 @jongycastillo
23 months 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().

Last edited 23 months ago by jongycastillo (previous) (diff)

#12 follow-up: @geisthanen
23 months 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 @jongycastillo
23 months 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.


21 months ago

creative-andrew commented on PR #2792:


20 months ago
#15

@costdev I've taken care of your requested revisions.

#16 in reply to: ↑ 10 @geisthanen
20 months 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: @audrasjb
18 months 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 @geisthanen
18 months 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.

#19 @geisthanen
16 months ago

@audrasjb the PR is ready to be reviewed.

This ticket was mentioned in PR #3808 on WordPress/wordpress-develop by @geisthanen.


16 months 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 @geisthanen
16 months 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 :)

Last edited 16 months ago by geisthanen (previous) (diff)

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


14 months ago

#23 follow-up: @mukesh27
14 months 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 @geisthanen
14 months 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:


14 months ago
#25

@costdev change proposals committed :)

@geisthanen commented on PR #2792:


14 months ago
#26

@costdev I have taken care of your comments :)

#27 @costdev
14 months ago

  • Keywords commit added; changes-requested removed

PR 2792 looks ready for commit consideration to me.

#28 @audrasjb
14 months ago

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

In 55429:

Build/Test Tools: Add test coverage for the get_posts_navigation() function.

This changeset adds a test case to verify that get_the_posts_navigation() only includes the "Older posts" and "Newer" posts links when appropriate.

Props jongycastillo, michelmany, joyously, geisthanen, SergeyBiryukov, mukesh27, audrasjb, costdev, sun, chrisbaltazar.
Fixes #55751.

@audrasjb commented on PR #2725:


14 months ago
#30

closed in favor of #3808.

@audrasjb commented on PR #3808:


14 months ago
#32

Reopening

@audrasjb commented on PR #3808:


14 months ago
#33

Reopening

#34 @audrasjb
14 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening as PR3808 was missed.

#35 @SergeyBiryukov
14 months ago

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

In 55442:

Tests: Add unit tests for get_next_posts_link() and get_previous_posts_link().

The tests ensure that the next_posts_link_attributes and previous_posts_link_attributes filters are applied correctly.

Follow-up to [1383], [5045], [8502], [9632], [55429].

Props geisthanen, mukesh27, costdev, audrasjb, SergeyBiryukov.
Fixes #55751.

@SergeyBiryukov commented on PR #3808:


14 months 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.
Note: See TracTickets for help on using tickets.