DatabaseStatementPrefetch::current PHP function array_unshift() are used incorrectly

Good fix!

Hmm this looks like it could be tested, could we add some?

Untagging per #11. Thanks @droplet!

Coming from #2259947: Minor bug fixes in database system

New patch with changes to #10:

• Rebased to current 8.x
• Removed t() from assertion messages.
• Replace PDO with \PDO (backslash prepended).
• Flipped around some lines in testQueryFetchClassType():
  • Assertion on $record for "Classname field not found, as intended" moved into the loop, to avoid $record being undefined.
  • $records[] = $record moved to end of loop.

Style fixes.

The relevant fix was this:

      $this->installSchema('database_test', array(
       'test',
+      'test_classtype',
       'test_people',

The rest is just relocating stuff for aesthetic/conceptual reasons.

Rerolled the patch to match new test locations in head. Test files moved to /core/modules/system/src/Tests/Database

+++ b/core/modules/system/src/Tests/Database/DatabaseTestBase.php
@@ -92,6 +93,15 @@ static function addSampleData() {
+      ¶

Whitespace problem

(sorry for pasting into the summary)

I came here as we found another bug. Should that be in here as you are writing tests too or should I report a new issue?

      case \PDO::FETCH_COLUMN:
          if (isset($this->columnNames[$this->fetchOptions['column']])) {
            return $this->currentRow[$k][$this->columnNames[$this->fetchOptions['column']]];
          }
          else {
            return;
          }

$k is not defined there.

I marked #1555462: Syntactical errors found in StatementPrefetch as a duplicate. The patch in that issue addresses $k being undefined as mentioned in #22, and that bit should be added to this patch.

Hi, I am a high school student who is new to drupal. I have attempted to patch the two issues mentioned above :)

Thanks @Katiemouse, great work.

Could you please upload a version of the patch with only the test and not the fix. That way we can see that the test fails as expected.

Hi this is the test only version :)

Marked #2342191: Subtle bug in Drupal\Core\Database\StatementPrefetch::current() as a duplicate.

If #26 is the test only, it should fail without the fix...

Closed #2679102: Multiple bugs in \Drupal\Core\Database\StatementPrefetch::current and #2499875: Fix bug with prefetch drivers and FETCH_CLASS as duplicates.

Changed the test a bit. The problem is that the test passes for me on my local machine. The bug of this issue is really a bug. Lets see if it passes on the testbot.

For the testbot

The patch with the fix.

Minor change to see if it will fix the MySQL testbot problem.

The patch with the fix and the test passes and the patch with only the test fails.

Ready for a good review.

RTBC - looks great to me

No longer applies, needs a reroll

Rerolled

1. +++ b/core/tests/Drupal/KernelTests/Core/Database/FetchTest.php
   @@ -80,6 +80,27 @@ public function testQueryFetchClass() {
   +        $this->assertIdentical($record->name, 'Kay', 'Kay is found.');
   +        $this->assertIdentical($record->job, 'Web Developer', 'A 26 year old Web Developer.');
   

   The expected value should be the first parameter passed to assertIdentical().

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/FetchTest.php
   @@ -80,6 +80,27 @@ public function testQueryFetchClass() {
   +    $this->assertIdentical(count($records), 1, 'There is only one record.');
   

   We should use assertCount() here.

   Also, I don't see why we need the extra $records variable. Can't we just assert the count for $result?

This patch should be added to Drupal 8.7, or the statement of PHP 7.2 compatibility will be false (since passing just one argument to array_unshift is allowed in 7.3 only :) ).

Based on #49, I have backported patch to D7.

Have ran tests locally, apparently looking ok.

---- DatabaseFetchTestCase ----


Status    Group      Filename          Line Function
--------------------------------------------------------------------------------
Pass      Other      database_test.tes   22 DatabaseTestCase->setUp()
    Enabled modules: database_test

[..snip..]

Pass      Other      database_test.tes   53 DatabaseTestCase->installTables()
    Table test_classtype created successfully.

[..snip..]

Pass      Other      database_test.tes  420 DatabaseFetchTestCase->testQueryFet
    Record is an object of class FakeRecord.
Pass      Other      database_test.tes  421 DatabaseFetchTestCase->testQueryFet
    Kay is found.
Pass      Other      database_test.tes  422 DatabaseFetchTestCase->testQueryFet
    A 26 year old Web Developer.

Further review and improvements would be much appreciated.

@lnunesbr: First the patch for Drupal 8 needs to be committed, before we start with backporting it to Drupal 7.

@maximpodorov: Yes, you are right about array_unshift. The use here of array_unshift is wrong. We are changing it to array_shift.

Fixed the remarks from @amateescu. With the exception of removing the $records variable. I have tried that and an assertCount() with the $results variable does not work.

Looks good, only small nit: @see link should be \Drupal\Tests\system\Functional\Database\FakeRecord instead of \Drupal\system\Tests\Database\FakeRecord

@init90: You are completely right. The class FakeRecord has been moved. Thank you for the good find!

Thanks! All main feedbacks were addressed, so set to RTBC.

Committed and pushed 6add1ca7a3 to 8.8.x and fb20927037 to 8.7.x. Thanks!

Backported to 8.7.x as this is a major bug and completely broken in HEAD. Nice to have test coverage at last.

diff --git a/core/tests/Drupal/KernelTests/Core/Database/FetchTest.php b/core/tests/Drupal/KernelTests/Core/Database/FetchTest.php
index 032339b5e0..a8b25c54bd 100644
--- a/core/tests/Drupal/KernelTests/Core/Database/FetchTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Database/FetchTest.php
@@ -86,7 +86,7 @@ public function testQueryFetchClass() {
    *
    * @see \Drupal\Tests\system\Functional\Database\FakeRecord
    */
-  function testQueryFetchClasstype() {
+  public function testQueryFetchClasstype() {
     $records = [];
     $result = $this->connection->query('SELECT classname, name, job FROM {test_classtype} WHERE age = :age', [':age' => 26], ['fetch' => \PDO::FETCH_CLASS | \PDO::FETCH_CLASSTYPE]);
     foreach ($result as $record) {

Fixed

----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 89 | ERROR | [x] Visibility must be declared on method
    |       |     "testQueryFetchClasstype"
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

on commit.