pager_query_add_page() [in D7, theme_pager_link()] overrides parameters passed programmatically

@mondrake - The bug report looks great and the fix makes sense. A couple things worth mentioning to move this issue forward.

1. This bugfix definitely needs tests (and I've set the status to 'needs work' because tests are a core gate). The good news is that it looks like you (or someone else) already has a pretty good guide for writing an automated test via your steps to reproduce code in the issue summary. Contributor doc on writing automated tests: http://drupal.org/node/1468170

   Adding automated test coverage will prove that this bug exists, and help prevent regressions in the future.

2. I wouldn't say this is blocked on #1778990: Merge theme_pager_link*() theme functions into theme_link(). It would certainly conflict with that patch (and with #1898474: pager.inc - Convert theme_ functions to Twig and possibly others). As long as the patch here is kept up to date, there's really not much to worry about and the order in which any of these patches gets committed is not that important. If this patch got in first, #1778990: Merge theme_pager_link*() theme functions into theme_link() would need to be rerolled, and vice versa. Same with #1898474: pager.inc - Convert theme_ functions to Twig. Welcome to core contribution :)

In short, this is an independent bug that deserves its own issue, and your best bet at seeing this issue resolved is to add that test coverage and keep the patch here up to date.

Tagging for tests and D7 backport.

The fact that this bug has not popped up before is even more reason to add test coverage :)

If the bug depends on having a page callback and cannot be reproduced by just calling theme('pager', …) with additional 'parameters', then you can create a small test module with page callbacks etc. to test against.

See for example block_test.module:
http://drupalcode.org/project/drupal.git/tree/refs/heads/8.x:/core/modul...

If you'd like more help, feel free to stop by Drupal core contribution mentoring:
http://drupal.org/core-office-hours

A better example is aggregator_test.module:
http://drupalcode.org/project/drupal.git/tree/refs/heads/8.x:/core/modul...

The test looks pretty reasonable, good work! A few minor nitpicks ahead:

+++ b/core/modules/system/lib/Drupal/system/Tests/Pager/PagerQueryParametersTest.phpundefined
@@ -0,0 +1,72 @@
+ * Definition of Drupal\system\Tests\Pager\PagerParametersTest.

The format for these changed, should be:
Contains \Drupal\system…

+++ b/core/modules/system/lib/Drupal/system/Tests/Pager/PagerQueryParametersTest.phpundefined
@@ -0,0 +1,72 @@
+use SimpleXMLElement;

I don't think this use is needed for your tests, should be removed.

+++ b/core/modules/system/lib/Drupal/system/Tests/Pager/PagerQueryParametersTest.phpundefined
@@ -0,0 +1,72 @@
+  ¶

+++ b/core/modules/system/tests/modules/pager_test/pager_test.moduleundefined
@@ -0,0 +1,67 @@
+  ¶
...
+  ¶

Few bits of trailing whitespace, please remove per http://drupal.org/coding-standards#indenting.

+++ b/core/includes/pager.incundefined
@@ -355,7 +355,7 @@ function theme_pager_link($variables) {
-    $query = array_merge($query, $query_pager);
+    $query = array_merge($query_pager, $query);

The only other thing I would recommend at this time would be to add a comment above this line you're changing to explain the reasoning behind the order of arguments passed to array_merge().

For the "Contains \Drupal\system…" the initial slash wasn't a typo, please see http://drupal.org/node/1354#file.

Interdiffs are great, too :)

+++ b/core/includes/pager.incundefined
@@ -355,7 +355,12 @@ function theme_pager_link($variables) {
-    $query = array_merge($query, $query_pager);
+    // $query_pager holds the query parameters of the current request; in
+    // case there are colliding parameter values coming from the $variables
+    // array, the latter should prevail. array_merge() overwrites $query_pager
+    // keys with $query keys, and returns the final merged $query array to be
+    // used for building the link.
+    $query = array_merge($query_pager, $query);

Do we really nee 5 lines of comments for this explanation? I think it would be much improved by clearer field names, and a brief plain english comment.

Something like this:

  // Merge the specified pager query parameters with any paramaters from the
  // current request. In case of collision, parameters from the current request
  // take precedence.
  if ($current_request_query = pager_get_query_parameters()) {
    $query = array_merge($current_request_query, $query);
  }

thoughts?

Oops, thanks,... see, it *was* confusing!

Nothing jumps out at me as wrong with the latest patch.

But because this is such an obscure and confusing problem, do you think you could provide a patch file that contains *just* the test (eg suffix the filename with -test-only.patch) to see what Drupal errors on currently?

Hat tip: https://twitter.com/xjmdrupal/status/195113152759865344

+++ b/core/includes/pager.inc
@@ -440,8 +440,11 @@ function pager_query_add_page(array $query, $element, $index) {
+  // Merge the pager query parameters specified in variables, with any
+  // parameters coming from the current request. In case of collision,
+  // parameters specified in variables take precedence.

Oops, I take it back.

The comma placement here makes this difficult to read and the terminology "in variables" is abstract enough to be confusing. Which variables are we talking about?

Could we change this comment again to something like:

Merge the query parameters passed to this function with the parameters from the current request. In case of collision, the parameters passed into this function take precedence.

This small change will give you and opportunity for a new patch, and a test-only patch on the same comment.

Needs work based on #48.

Based on the latest patch this title seems more accurate to me. Perhaps the issue summary should be updated a bit. Happy to see this is still moving :)

I've added a beta phase evaluation to the issue summary. The patch in #50 looks good to me. Marking RTBC.

Committed b7a6866 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

I believe if this fix is implemented, there is going to be a problem with the Date module's implementation of Views exposed select filters. I'm not sure if it is correct or incorrect, but that module generated a form element name that is like name="field_example_date[value][year]" and this issue's fix will cause a fatal error:

Fatal error: Cannot create references to/from string offsets nor overloaded objects in .../includes/common.inc on line 6754

I created an issue on the Date module: #2821200: Unusual form element name attribute for exposed Views select filter?

So it appears that the array_merge bug, outlined in this issue, was allowing the Date module's three-dimensional form element name to work. Fixing this issue will cause the link generated to trigger a fatal error because of the way the Date module (incorrectly or correctly) constructs the form element's name.

Hi,

I am currently facing this issue on Drupal 7.54, and patch #59 works like a charm.

I can see it was merged into 8.x, but why hasn't it been merged into 7.x ? We need this bug resolved in the core.

Thanks