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
#50 2871784-50.patch5.82 KBhash6
#50 interdiff_47-50.txt12.28 KBhash6
#47 interdiff_46-47.txt11.86 KBhash6
#47 2871784-47.patch8.86 KBhash6
#46 interdiff_44-46.txt694 byteshash6
#46 2871784-46.patch5.64 KBhash6
#45 interdiff_44-45.txt1.08 KBhash6
#45 2871784-45.patch6.17 KBhash6
#44 2871784-44.patch5.61 KBhash6
#43 test2.txt29.99 KBhash6
#38 2871784-38.patch5.61 KBSivaji_Ganesh_Jojodae
#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
Support from Acquia helps fund testing for Drupal Acquia logo

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.

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

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

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

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

colan’s picture

daffie’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
Sivaji_Ganesh_Jojodae’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.61 KB

Patch #31, re-rolled.

Status: Needs review » Needs work

The last submitted patch, 38: 2871784-38.patch, failed testing. View results

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Sivaji_Ganesh_Jojodae’s picture

Version: 8.8.x-dev » 9.1.x-dev
hash6’s picture

Assigned: Unassigned » hash6
hash6’s picture

FileSize
29.99 KB
 $this->drupalGet('entity_test/list');

    // var_dump($html);
    // Item 51 should not be present.
    $this->assertRaw('Test entity 50', 'Item 50 is shown.');
    $this->assertNoRaw('Test entity 51', 'Item 51 is on the next page.');

    // Browse to the next page.
    $this->clickLink(t('Page 2'));

On checking the above code inside EntityListBuilderTest.php clickLink() should probably be replaced by click()

Also, while trying to find the output using

$html = $this->clickLink('Page 2');
    ini_set('xdebug.var_display_max_data', -1);

    var_dump($html);

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.

hash6’s picture

Re rolled the patch to 9.1.x-dev , working on the two test cases which are failing.

hash6’s picture

removed 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

hash6’s picture

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

hash6’s picture

Removed $pager_page_array to get current pager and replaced that by using pager.manager and its getPager() method.

daffie’s picture

Issue tags: +Bug Smash Initiative
hash6’s picture

hash6’s picture

Lal_’s picture

Status: Needs work » Needs review
hash6’s picture

Assigned: hash6 » Unassigned
msuthars’s picture

Assigned: Unassigned » msuthars
Status: Needs review » Needs work

I'm working on it.

msuthars’s picture

Assigned: msuthars » Unassigned

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.