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.

CommentFileSizeAuthor
#31 2871784-31.patch5.54 KBmondrake
#31 2871784-31-test-only.patch4.11 KBmondrake
#27 interdiff_20-28.txt1.52 KBmondrake
#27 2871784-28.patch4.3 KBmondrake
#27 2871784-28-test-only.patch2.87 KBmondrake
#25 2871784-24.patch5.17 KBohthehugemanatee
#21 2871784-20.patch5.14 KBmondrake
#21 interdiff_19-20.txt784 bytesmondrake
#19 2871784-19.patch5.15 KBmondrake
#19 2871784-19-test-only.patch3.52 KBmondrake
#16 2871784-16.patch1.88 KBmondrake
#16 interdiff_15-16.txt1.17 KBmondrake
#15 interdiff_14-15.txt548 bytesmondrake
#15 2871784-15.patch1.73 KBmondrake
#14 2871784-14.patch2.27 KBmondrake
#14 interdiff_12-14.txt1.61 KBmondrake
#12 2871784-12.patch2.76 KBmondrake
#12 interdiff_10-12.txt970 bytesmondrake
#10 2871784-10.patch2.76 KBmondrake
#10 interdiff_7-10.txt1.98 KBmondrake
#9 Schermafbeelding 2017-04-26 om 14.32.43.png6.95 KBfalc0
#7 2871784-7-test-only.patch800 bytesmondrake
#2 drupal-2871784-1.patch418 bytesfalc0
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

falc0 created an issue. See original summary.

falc0’s picture

cilefen’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Active » Needs review

It 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?

Status: Needs review » Needs work

The last submitted patch, 2: drupal-2871784-1.patch, failed testing.

falc0’s picture

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

cilefen’s picture

I 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/".

mondrake’s picture

Status: Needs work » Needs review
FileSize
800 bytes

Let'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.

mondrake’s picture

Status: Needs review » Needs work

So... 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?

falc0’s picture

In my case this was the setup:

protected function loadFAQs($limit = 10) {
    $query = $this->entity_query->get('node');
    $query->accessCheck();
    $query->condition('status', 1, '=', $this->currentLanguageCode);
    $query->condition('type', 'faq', '=', $this->currentLanguageCode);
    $query->pager($limit);
    $entity_ids = $query->execute();

When visiting the results page and changing the query param to ?page=-1 it breaks. My debugger shows a negative $pager['start'].

mondrake’s picture

Priority: Normal » Major
Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
1.98 KB
2.76 KB

It looks like QueryBase::initializePager calls pager_find_page before the pager is initialized via pager_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.

Status: Needs review » Needs work

The last submitted patch, 10: 2871784-10.patch, failed testing.

mondrake’s picture

Status: Needs review » Needs work

The last submitted patch, 12: 2871784-12.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
FileSize
1.61 KB
2.27 KB

Ok, ok

mondrake’s picture

FileSize
1.73 KB
548 bytes

Maybe in the end we do not need changes to pager.inc at all.

mondrake’s picture

FileSize
1.17 KB
1.88 KB

Actually, calling pager_find_page and then checking that the found page is in the [0:max page] range seems a duplication here, since pager_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.

mondrake’s picture

Status: Needs review » Needs work

NW for the tests.

mondrake’s picture

Assigned: Unassigned » mondrake

working on a test

mondrake’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.52 KB
5.15 KB

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

The last submitted patch, 19: 2871784-19-test-only.patch, failed testing.

mondrake’s picture

FileSize
784 bytes
5.14 KB

Removed unintentional whitspace.

ohthehugemanatee’s picture

Status: Needs review » Reviewed & tested by the community

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

mondrake’s picture

Assigned: mondrake » Unassigned
catch’s picture

Status: Reviewed & tested by the community » Needs work

I'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?

ohthehugemanatee’s picture

Status: Needs work » Needs review
FileSize
5.17 KB

Good point. Converted the global to pager_find_page(), added a couple of explanatory comments.

Status: Needs review » Needs work

The last submitted patch, 25: 2871784-24.patch, failed testing.

mondrake’s picture

Status: Needs work » Needs review
FileSize
2.87 KB
4.3 KB
1.52 KB

Not 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 in pager_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 calls pager_find_page, so removing it from QueryBase seems to me avoid duplication.

The last submitted patch, 27: 2871784-28-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 27: 2871784-28.patch, failed testing.

ohthehugemanatee’s picture

Apart 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

mondrake’s picture

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

The last submitted patch, 31: 2871784-31-test-only.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.