WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#30831 closed defect (bug) (fixed)

paginate_links() now needs add_args=false

Reported by: bobbingwide Owned by: dd32
Milestone: 4.1.1 Priority: normal
Severity: normal Version: 4.1
Component: Themes Keywords: has-patch fixed-major commit
Focuses: template Cc:

Description

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.

Attachments (6)

30831.test.patch (1018 bytes) - added by boonebgorges 3 years ago.
30831.patch (1.5 KB) - added by boonebgorges 3 years ago.
30831.diff (2.1 KB) - added by obenland 3 years ago.
30831.2.diff (5.0 KB) - added by obenland 3 years ago.
30831.3.diff (2.2 KB) - added by dd32 3 years ago.
30831.4.diff (1.7 KB) - added by boonebgorges 3 years ago.

Download all attachments as: .zip

Change History (29)

#1 @SergeyBiryukov
4 years ago

  • Focuses template added

#3 @nacin
3 years ago

  • Component changed from General to Themes
  • Milestone changed from Awaiting Review to 4.1.1

Hm. Ouch. Moving to 4.1.1 for investigation.

#4 @bobbingwide
3 years ago

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.

#5 @boonebgorges
3 years ago

  • Keywords has-patch 2nd-opinion added

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.

@boonebgorges
3 years ago

@obenland
3 years ago

@obenland
3 years ago

#6 @obenland
3 years ago

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.

#7 @boonebgorges
3 years ago

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.

#8 @obenland
3 years ago

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

#9 @boonebgorges
3 years ago

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.

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


3 years ago

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


3 years ago

#12 @MH Themes
3 years ago

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.

#13 @boonebgorges
3 years ago

  • Keywords fixed-major commit added; 2nd-opinion removed

[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.

#14 @SergeyBiryukov
3 years ago

#31060 was marked as a duplicate.

#15 @sc0ttkclark
3 years ago

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

#16 @ndh01
3 years ago

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

#17 @SergeyBiryukov
3 years ago

#31285 was marked as a duplicate.

#18 follow-up: @dd32
3 years ago

[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

@dd32
3 years ago

@boonebgorges
3 years ago

#19 in reply to: ↑ 18 @boonebgorges
3 years ago

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.

#20 @dd32
3 years ago

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.

#21 @boonebgorges
3 years ago

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

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

#22 @dd32
3 years ago

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].

#23 @dd32
3 years ago

  • Owner set to dd32
  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.