WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#31939 closed defect (bug) (fixed)

paginate_links() - incorrect links on return to the first page

Reported by: bobbingwide Owned by: boonebgorges
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.1.1
Component: Themes Keywords: has-patch needs-testing
Focuses: Cc:

Description

Sorry about this but this is a direct follow on to #30831.

With the exact same piece of code that I had originally,
when you return to the first page of the paginated content,
none of the links contain the correct paging values...
so they don't work.

function bw_navi_paginate_links( $id, $page, $pages ) {
  $base = add_query_arg( "bwscid$id", "%_%" );
  $format = "%#%";
  $args = array( "base" => $base
               , "format" => $format
               , "total" => $pages
               , "current" => $page
               , "before_page_number" => "["
               , "after_page_number" => "]"
               );

  $links = paginate_links( $args ); 
  e( $links );
}

regardless of whether or not I attempt to apply my workaround

              , "add_args" => false

Expected output:

Assume the original display is:

1 to 2 of 5
one
two
(1) [2] [3] Next

and page 2 display is:

3 to 4 of 5
three
four
Previous [1] (2) [3] Next

where (n) indicates it's not a link

with WordPress 4.0, and a value of 2, for the $id the pagination links were

You could visit page 2, then return to page 1, then visit page 2 again

Actual output:

  • On first visit the links are OK
  • When you visit page 2 the links are OK
  • When you return to page 1 all the links are broken

http://example.com/permalink/?bwscid2

In order to be able to page you need to remove the query parm from the URL.

Demo:

For an example see http://wp-a2z.com/oik_api/paginate_links/
and try to page forwards then backwards through the source code.

Workaround:

Unknown. Please advise.

Attachments (1)

31939.diff (2.4 KB) - added by boonebgorges 4 years ago.

Download all attachments as: .zip

Change History (9)

#1 @bobbingwide
4 years ago

  • Keywords 2nd-opinion added

OK, I have a workaround.

I need to change the value of REQUEST_URI in $_SERVER to remove my special query arg, otherwise
the new code in 4.1.1 will find it as the pagenum_link and add it back into the add_args array...
even though I've specifically passed add_args=>false.

function bw_navi_paginate_links( $id, $page, $pages ) {
  $string = remove_query_arg( "bwscid$id" );
  $_SERVER['REQUEST_URI'] = $string; 
  $base = add_query_arg( "bwscid$id", "%_%" );
  $format = "%#%";
  $args = array( "base" => $base
               , "format" => $format
               , "total" => $pages
               , "current" => $page
               , "before_page_number" => "["
               , "after_page_number" => "]"
               , "add_args" => false
               );
  $links = paginate_links( $args ); 
  e( $links );
}

#2 @boonebgorges
4 years ago

  • Keywords 4.3-early has-patch needs-testing added; 2nd-opinion removed
  • Milestone changed from Awaiting Review to Future Release

Thanks for the report, bobbingwide. The technique used to avoid overwriting the custom pagination query var in [31203] is problematic in two ways.

First, using parse_url( $url, PHP_URL_QUERY ) doesn't work properly when the $url contains a # - anything after the first # is interpreted as part of the 'fragment' rather than the 'query'. See https://core.trac.wordpress.org/browser/tags/4.1.1/src/wp-includes/general-template.php?marks=2632#L2629. When $args['base'] contains # - which it almost always will - this parsing will be broken. Instead of relying on parse_url(), we can just explode on ?.

Second, remove_query_arg() doesn't know what to do with a string like 'foo', you get when you parse the querystring out of example.com/?foo. That is, remove_query_arg( 'foo', 'foo' ) will do nothing. We need a technique for removing the query args that is agnostic between query vars that have values (?foo=bar) and those that don't (?foo). https://core.trac.wordpress.org/browser/tags/4.1.1/src/wp-includes/general-template.php?marks=2636#L2629

Have a look at the new parsing I'm proposing in 31939.diff.

@boonebgorges
4 years ago

#3 @obenland
4 years ago

  • Owner set to boonebgorges
  • Status changed from new to assigned

#4 @obenland
4 years ago

  • Keywords 4.3-early removed
  • Milestone changed from Future Release to 4.3

#5 @boonebgorges
4 years ago

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

In 32359:

In paginate_links(), improve handling of custom pagination query vars.

Custom pagination query vars, as provided in the 'base' parameter, must be
detected in the current page URL and removed before generating fresh pagination
links. The logic introduced in this changeset ensures that these custom
query vars are properly detected in cases where the 'format' param contains
a #.

This is a follow-up to [31203] #30831.

Fixes #31939.

#6 follow-up: @bobbingwide
4 years ago

  • Keywords changed from has-patch, needs-testing to has-patch needs-testing

Sorry I didn't get a chance to provide feedback on the code before it was committed, but I had to adjust my workaround because of the latest security scare etc..

The comment I did want to make was that for performance the test on total pages should be a lot earlier.
Shall I raise a separate TRAC for this?


// Who knows what else people pass in $args
	$total = (int) $args['total'];
	if ( $total < 2 ) {
		return;
	}

And maybe remove the one line comment too?

#7 in reply to: ↑ 6 @boonebgorges
4 years ago

Replying to bobbingwide:

Sorry I didn't get a chance to provide feedback on the code before it was committed, but I had to adjust my workaround because of the latest security scare etc..

The comment I did want to make was that for performance the test on total pages should be a lot earlier.
Shall I raise a separate TRAC for this?


// Who knows what else people pass in $args
	$total = (int) $args['total'];
	if ( $total < 2 ) {
		return;
	}

And maybe remove the one line comment too?

Yeah, please open another ticket for this suggestion. Thanks!

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


4 years ago

Note: See TracTickets for help on using tickets.