paginate_links() now needs add_args=false

[bobbingwide]

With WordPress 4.1 my simple call to paginate_links() now requires me to specifically set add_args=false.

If this parameter is not set, then on any "page" after 1, all the links point to the current page.

I have a number of shortcodes which implement their own pagination. They use query parms named bwscidn, where n is an integer representing the shortcode instance to be paginated. This allows multiple shortcodes to appear on a page and each to be individually paginated.

e.g.
​http://www.oik-plugins.com/2014/12/pagination-not-working-properly-wordpress-4-1/?bwscid3=2&bwscid4=3

Prior to WordPress 4.1 the code was

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 );
}

Now with WordPress 4.1 I need to add another entry to the $args array.

   , "add_args" => false

Can anyone confirm that this is what I should have expected from the changes in 4.1?

BTW. The codex still says that the default for add_args is False (sic).
BTW2. The current documentation in
​https://developer.wordpress.org/reference/functions/paginate_links/
formats %_% incorrectly.

PS. I think this is the reverse of #24606.
PPS. This change does not adversely affect code running on 3.9.

Conclusion: I have a solution to my problem but there is a need to change the codex and potentially a need for some more unit tests.

[boonebgorges]

See also ​https://buddypress.trac.wordpress.org/ticket/5967

[nacin]

Hm. Ouch. Moving to 4.1.1 for investigation.

[bobbingwide]

props to kovshenin in https://core.trac.wordpress.org/ticket/24606#comment:22 for performing the plugin search. I hadn't written my code then _{so the problem wouldn't have been detected.}

The BuddyPress ticket is remarkably similar and has the same fix.

Given that workarounds exist, is it still necessary to produce a patch for 4.1.1?

I'm ashamed of myself for not having discovered this problem much, much earlier.
But had I done so, then I probably wouldn't have raised this TRAC.

[boonebgorges]

30831.test.patch​ is a test that describes the problem.

The problem dates to [29780] #29636. The root problem has something to do with the fact that get_pagenum_link() doesn't recognize the kinds of custom query arguments passed in the 'base' param of paginate_links(). I'm not sure how to fix this without doing a bunch of refactoring of get_pagenum_link() (and I'm not sure that this would be an appropriate fix anyway). Maybe we set the default $query_args to false if a custom 'base' has been provided? 30831.patch​ takes this route, and it passes all the tests, but I'd like the opinion of someone who is more familiar with the storied history of this set of functions.

[obenland]

I think it's okay for get_pagenum_link() not to be aware of the custom query argument.

30831.diff​ checks the existing arguments for the custom query argument and removes it, so that it can't be overwritten later on.

30831.2.diff​ goes a little beyond that and adds comments to the set up part to make it a little easier to grok. I also added a unit test to make sure that existing arguments and passed arguments get along.

[boonebgorges]

30831.diff​ looks like the right tack to me. .2.diff offers some reasonable enhancements (sniffing $total and $current defaults from $wp_query) but this is beyond what we should do for 4.1.1.

[obenland]

.2.diff really just adds comments, defaults for $total and $current have been around for a while.

[boonebgorges]

Gah, you're right, I was being thrown off by Trac's diff formatting (and the fact that it was Monday morning). The change looks good to me, but I'd like to get a second (er, third) opinion before going with it.

[MH Themes]

Hi, some of our users just reported a similar issue. It also happens when simply implementing paginate_links() as stated in the basic example of the codex: ​http://codex.wordpress.org/Function_Reference/paginate_links#Basic_Example

This started to happen in WP 4.1. Can you confirm that the issue will be fixed in WordPress 4.1.1 or does this require further investigation?

Thanks.

[boonebgorges]

[31203] missed the ticket. obenland, I had a second look and went with your fix - only changes I made were to consolidate and clarify (I hope) some of the comments.

[SergeyBiryukov]

#31060 was marked as a duplicate.

[sc0ttkclark]

Been unknowingly burned by this for a few weeks, glad to see it fixed!

[ndh01]

Spent some time troubleshooting this today. Glad to see a workaround and fix.

[SergeyBiryukov]

#31285 was marked as a duplicate.

[dd32]

[31203] has some compatibility issues, particularly around the solution to this bug.

Previously, you could pass false to add_args in paginate_links(), and it'd simply get handled appropriately, however now we treat the field as an array, resulting in: Warning: array_merge(): Argument #1 is not an array in wp-includes/general-template.php on line 2633.

Unfortunately due to this bug, we can't really expect that others haven't simply added 'add_args' => false to their paginate links function, so now we've probably got to add something such as 30831.3.diff​

[boonebgorges]

Replying to dd32:

[31203] has some compatibility issues, particularly around the solution to this bug.

Previously, you could pass false to add_args in paginate_links(), and it'd simply get handled appropriately, however now we treat the field as an array, resulting in: Warning: array_merge(): Argument #1 is not an array in wp-includes/general-template.php on line 2633.

Unfortunately due to this bug, we can't really expect that others haven't simply added 'add_args' => false to their paginate links function, so now we've probably got to add something such as 30831.3.diff​

Yes, good catch. 30831.4.diff​ adds a unit test demonstrating the error. I've modified your patch slightly so that the $add_args variable is still set to false if $args['add_args'] is empty. Letting it pass through as an empty array will pass the if ( $add_args ) tests further down in the function; an empty array will be passed to add_query_arg(), which will have no effect on the function output, but is a waste of cycles.

[dd32]

I've modified your patch slightly so that the $add_args variable is still set to false if $argsadd_args? is empty. Letting it pass through as an empty array will pass the if ( $add_args ) tests further down in the function; an empty array will be passed to add_query_arg(), which will have no effect on the function output, but is a waste of cycles.

No it won't, as ( array() == false ) === true, so it'll never enter the if, since there's no need to set it to false, we should avoid doing so for clarity.

[boonebgorges]

( array() == false ) === true

Well by George. Ain't PHP neat. Your fix looks right then.

[dd32]

In 31432:

Avoid a PHP Warning when add_args is passed as false to paginate_links().

Props boonebgorges for the unit test.
See #30831 [31203].

[dd32]

In 31433:

In paginate_links(), don't override custom format arguments when setting up default 'add_args'.

Since 4.1 [29780], the default value of the 'add_args' argument in
paginate_links() has been determined by parsing the current URL. This change
had the side effect of overriding custom values of 'format' that changed the
pagination query var, with the result that plugins using paginate_links()
with a custom format generated the incorrect links unless explicitly
declaring 'add_args=false' to prevent the default values from overriding. We
fix this behavior by parsing URL query vars into the 'add_args' array only
after the explicit function params have been parsed, and by skipping the
current page's pagination query var when doing this parsing (to avoid the
override).

Props obenland.
Merges [31203], [31432] to the 4.1 branch.
Fixes #30831.