Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Using entity query with a negative pager throws a MYSQL error.
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '-10' at line 8:
Proposed resolution
Return an absolute value when fetching the pager value.
Comment | File | Size | Author |
---|---|---|---|
#50 | 2871784-50.patch | 5.82 KB | hash6 |
#50 | interdiff_47-50.txt | 12.28 KB | hash6 |
#46 | interdiff_44-46.txt | 694 bytes | hash6 |
#46 | 2871784-46.patch | 5.64 KB | hash6 |
#44 | 2871784-44.patch | 5.61 KB | hash6 |
Comments
Comment #2
falc0 CreditAttribution: falc0 at Dropsolid commentedComment #3
cilefen CreditAttribution: cilefen commentedIt seems to me like the negative value would result from wanting something different from its absolute value. What is the use-case for negative values? Do pagers end up with negative values?
Comment #5
falc0 CreditAttribution: falc0 at Dropsolid commentedThere is no use-case for this. We found these errors in our watchdog and found out if you change the pager to a negative value this error shows.
Comment #6
cilefen CreditAttribution: cilefen commentedI see. I feel like I would rather just make it 0 if someone sends in a negative page. The patch failed to apply because it contains a reference to "docroot/".
Comment #7
mondrakeLet's have a test, first. This patch adds one checking that, if negative page numbers are coming from the URL, still the first page in the pager is returned, along suggestion in #6.
Comment #8
mondrakeSo... this is weird. The pager code already resets negative page numbers coming from URL: in
pager_default_initialize
, the part where it does$pager_page_array[$element] = max(0, min($page, ((int) $pager_total[$element]) - 1));
actually will set the page for the $element to 0 if its input $page is negative. The test-only patch in #7 proves that.
So the fix in #1 is kinda redundant.
The question would be now: how do negative numbers come to the entity query, then?
Comment #9
falc0 CreditAttribution: falc0 at Dropsolid commentedIn my case this was the setup:
When visiting the results page and changing the query param to ?page=-1 it breaks. My debugger shows a negative $pager['start'].
Comment #10
mondrakeIt looks like
QueryBase::initializePager
callspager_find_page
before the pager is initialized viapager_default_initialize
. At this stage, the value returned is not validated against the possible range. In fact, if I am right, you should also get a failure if you set a page number which is above the total, not only if it is negative. This point to a lack of test coverage for QueryBase.Let's see what bot thinks of this patch, which reshuffles a bit how the current page is retrieved for both normal pager and entity query pager. Will need tests for QueryBase.
Bumping to major.
Comment #12
mondrakeMismatched brackets.
Comment #14
mondrakeOk, ok
Comment #15
mondrakeMaybe in the end we do not need changes to
pager.inc
at all.Comment #16
mondrakeActually, calling
pager_find_page
and then checking that the found page is in the [0:max page] range seems a duplication here, sincepager_default_initialize
will also do that, and we need the current page only after initialization of the pager, to determine the offset from where to start the query resultset.Better just invoke
pager_default_initialize
and then get the current page (already sanitized) from the$pager_page_array
global.Folks may complain about using a global variable here, but that is the current API for pagers until when #2044435: Convert pager.inc to a service will be solved.
Comment #17
mondrakeNW for the tests.
Comment #18
mondrakeworking on a test
Comment #19
mondrakeWith tests. The test-only patch (=interdiff) shows the failure reported in the IS, and failure if page requested is above the last one. Full patch has the fix + one additional test for multiple pagers with negative values that I think it's good to have anyway.
Comment #21
mondrakeRemoved unintentional whitspace.
Comment #22
ohthehugemanatee CreditAttribution: ohthehugemanatee as a volunteer commentedTested working by creating a route that shows a paged list of nodes, and adding a negative number to the "page" value in the URL query.
I couldn't test it broken with Views, though. I guess Views sanitizes this somewhere along the way.
Comment #23
mondrakeComment #24
catchI'm not sure from reading the patch why we need to access the global directly instead of using pager_find_page() - could we add some documentation explaining what's going on a bit more?
Comment #25
ohthehugemanatee CreditAttribution: ohthehugemanatee as a volunteer commentedGood point. Converted the global to pager_find_page(), added a couple of explanatory comments.
Comment #27
mondrakeNot using
pager_find_page
is the fix.pager_find_page
returns what is in the URL, cast to int. That also means negative integers (the issue here), or integers above the maximum number of pages in the resultset. 'Sanitizing' the current page is done inpager_default_initialize
, that 'knows' the total number of pages, and results stored in $pager_page_array. It would be good to have a method to access the current page instead of accessing the global, but that's not for here and in fact in scope of #2044435: Convert pager.inc to a service.Finally,
pager_default_initialize
also callspager_find_page
, so removing it from QueryBase seems to me avoid duplication.Comment #30
ohthehugemanatee CreditAttribution: ohthehugemanatee as a volunteer commentedApart from fixing the failed test...
Why not sanitize the value of pager_find_page? The plan is to make pager_find_page a wrapper for the eventual service anyway, so this will save us a little work. It's still better than using the global.
Then the comment doesn't have to be so exhaustive. Just say we have to sanitize the pager value, since pager_default_initialize hasn't been run on it yet. If you like, have individual line comments for each thing you're validating
Comment #31
mondrakeFixing the failing tests, forgot the new test controller code in the patch in #28, just readded that.
@ohthehugemanatee feel free to continue according to #30 if you like. IMHO we should not use that function here, for the reasons I mentioned.
Comment #36
colanSimilar work going on in #3080859: Huge integer on views pager LIMIT causes mysql error.
Comment #37
daffie CreditAttribution: daffie commented#2044435: Convert pager.inc to a service has landed.
Comment #38
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae as a volunteer and commentedPatch #31, re-rolled.
Comment #41
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae as a volunteer and commentedComment #42
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedComment #43
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedOn checking the above code inside EntityListBuilderTest.php
clickLink()
should probably be replaced byclick()
Also, while trying to find the output using
It returns html as attached as test2.html
which should actually show only Content "Test entity 51" for page 2 but it shows all of them.
Comment #44
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedRe rolled the patch to 9.1.x-dev , working on the two test cases which are failing.
Comment #45
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedremoved the unknown(deprecated in d8 and removed in d9) entity.query service and replaced it with entity_type.manager service and getQuery() method to get query
Comment #46
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedremoved the unknown(deprecated in d8 and removed in d9) entity.query service and replaced it with entity_type.manager service and getQuery() method to get query. removed one unwanted change.
Comment #47
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedRemoved $pager_page_array to get current pager and replaced that by using pager.manager and its getPager() method.
Comment #48
daffie CreditAttribution: daffie commentedComment #49
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedComment #50
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedComment #51
Lal_Comment #52
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedComment #53
msutharsI'm working on it.
Comment #54
msuthars