Warning: explode() expects parameter 2 to be string, array given in Drupal\Core\Pager\PagerParameters->getPagerQuery()

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Attached patch fixes issue raised by the bot. It also cleans up the comment a tad.

* The both complained about the isset() which was introduced in #28, I've replace it with a simple condition because IMO neither isset() nor !empty is necessary here.

The last submitted patch, 40: 3143617-40.patch, failed testing. View results →

Its been 3 years so its hard to remember but pretty sure the isset was the to preserve the behavior of the empty check as demonstrated by the failing tests. What was the problem with it?

Attached patch fixes issues

Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.

raised by the test bot

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Attached a patch without the use of ->all()['page'] for avoid warning like

Undefined array key "page"

Issue summary should be updated to the standard issue template, even if sections don't apply leaving them with NA is easier for reviews/committers.

Updated.

Turns out its been long enough a few things have changed. Specifically, current versions of PHP throw an exception instead of a warning which makes it a bit harder to see. Specifically, real exception gets gobbled up by a more generic exception and Drupal's exception logging in watchdog doesn't show previous exceptions.

Since the actual exception is hidden and its hard to validate the root cause without hacking the exception handler or using a debugger, I've left the original trace in place. The fix is the same and the trace captures the essence of the problem.

Ran patch #47 and it has a test failure that seems legit.

Recommended to convert to MRs

"pretty sure the isset was the to preserve the behavior of the empty check as demonstrated by the failing tests." 🤷

Added it back because it was there to defensively handle a null value from the getPagerParameter method. Since isset is a language level feature there's no real cost to being defensive like this.

I'm already tired of the 500 remotes and all the commit juggling. I'm going to really miss the simple patch workflow. But ok, its in a MR now.

Turns around it was a little more complicated. I went ahead and took the opportunity to update the tests and make things a little more clear.

Thanks looks good!

I'm triaging RTBC issues → . I read the IS, the comments and the MR.

Thanks for updating the Issue Summary, that is always helpful! The review in #28 by @neclimdul was also useful as it explained what they were thinking and checking. That provided me with insight into understanding the issue and when else I may need to check.

The changes to getPagerQuery and getPagerParameter are changing the behavior, making it conform to the interface. So, it there any ramification for contrib/custom?

I left suggestions in the MR for coding standards and one asking for documentation.

I hide all the patches.

This issue got reported as a security issue for better_exposed_filter, but was later made public. 🐛 Inclusion of parameter exhausts site memory with exposed filter block Needs work

Addressed the feedback

Thanks for working on this.

Regarding @quietone's outstanding question in #56:

The changes to getPagerQuery and getPagerParameter are changing the behavior, making it conform to the interface. So, it there any ramification for contrib/custom?

I think this question has already been answered by @neclimdul in #28 6):

Fixing it to match the interface _could_ break contrib or sites hacking strings or floats through a pager value. However those cases would be broken in findPage which does the cast already so it was probably already effectively broken.

The code changes look good. I added one suggestion and a few comments inline, mostly concerning comments in the test.

The MR introduces new test coverage for the pager parameter. The pipeline is green. The tests fail locally as expected without the fix. I'll run the test only job just before I'm otherwise ready to RTBC, so not quite yet. With the MR applied, I can't reproduce the original issue. I also did some manual testing both with the original and modified (invalid) pager parameters: The administrative content view (using AJAX), a regular view (without AJAX), multiple views with different pager ids on the same page, the administrative URL Alias overview page (no view, probably entity query pager?). I noticed an issue with 3 views on the same page, but re-testing this without the MR applied, the issue was already present before, so no regression.

Tagging security improvement (for contrib) based on #57.

So overall this looks good and almost ready. Ready to RTBC once my very minor MR comments have been addressed.

Did not mean to take so long but gave it a shot. May need to expand on the docs some though.

Thanks Stephen, the changes look good to me.

Looks like the CI runners are a little busy right now, the repeated failures due to GitHub timeouts and no test nodes available gave me sufficient time to also retest this locally. Almost like Drupal CI, oh the good old times, where I could work on another issue while I waited for the tests to complete... Let's call it the DrupalCon review bonus :)

Test only job fails as expected:

Configuration: /builds/issue/drupal-3143617/core/phpunit.xml.dist
..EE.......EE.FF....FEE..F...                                     29 / 29 (100%)
Time: 00:00.034, Memory: 8.00 MB
There were 6 errors:
1) Drupal\Tests\Core\Pager\PagerParametersTest::testFindPage with data set "invalid empty page array" ([], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:57
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:49
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:47
2) Drupal\Tests\Core\Pager\PagerParametersTest::testFindPage with data set "invalid populated array" ([1, 2, 3], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:57
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:49
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:47
3) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerQuery with data set "invalid empty page array" ([], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:57
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:60
4) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerQuery with data set "invalid populated array" ([1, 2, 3], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:57
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:60
5) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerParameter with data set "invalid empty page array" ([], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:83
6) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerParameter with data set "invalid populated array" ([1, 2, 3], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:83
--
There were 4 failures:
1) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerQuery with data set "page 0 as a string" ('0', '0', [0])
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 0
 )
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:60
2) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerQuery with data set "page 0 as a integer" (0, '0', [0])
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 0
 )
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:60
3) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerParameter with data set "defensive null page value" (null, '', [])
Failed asserting that null is identical to ''.
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:83
4) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerParameter with data set "page 0 as a integer" (0, '0', [0])
Failed asserting that 0 is identical to '0'.
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:83
ERRORS!
Tests: 29, Assertions: 33, Errors: 6, Failures: 4.
Exiting with EXIT_CODE=2

The documentation for the data provider return value could always be a bit more verbose of course, but I think documenting that the keys returned by the data provider correspond to the variable names that are also used in the test methods should be enough for developers looking at this test code in the future to get the idea behind this setup. It should also address @quietone's concern:

For whatever reason, I started reading this from the provider and kept wondering why there were three return values when testGetPagerParameter() is only using one.

So let's give the RTBC queue triage team another shot at this... I think this issue is RTBC 🎉!

So let's give the RTBC queue triage team

Who are members of this team? :-)

Thanks for updating the MR and making the improvements to the test documentation. Hopefully, it will help the next person to work on this.
There are no unanswered questions. Leaving at RTBC.

Updating issue credits

Left some comments on the MR. I think we should be using the new Symfony API, not inventing our own protection

trying to update the code as suggested in the MR