This issue is part of #2454513: [meta] Make Drupal 8 work with SQLite.

Drupal\system\Tests\Database\FetchTest is currently failing on SQLite because Drupal\Core\Database\StatementPrefetch does not take $allowRowCount into consideration.

CommentFileSizeAuthor
#1 2475177.patch801 bytesamateescu

Comments

amateescu’s picture

amateescu commented
Status: Active » Needs review
StatusFileSize
new 2475177.patch801 bytes

Test run before:

Test summary
------------

Drupal\system\Tests\Database\FetchTest                        40 passes   1 fails                            

Test run duration: 4 sec

Test run after:

Test summary
------------

Drupal\system\Tests\Database\FetchTest                        41 passes                                      

Test run duration: 4 sec
amateescu’s picture

amateescu commented

This patch also fixes Drupal\system\Tests\Database\SelectComplexTest which has the same assertion at the bottom of the test class: http://cgit.drupalcode.org/drupal/tree/core/modules/system/src/Tests/Dat...

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

Confirmed that the patch applies cleanly on the latest D8. It also fixes the failing test and makes sense from a read through. I'd suggest leaving the comment that reads Implementations of StatementInterface., and leaving out the {@inheritdoc} docblock. This whole file could use some documentation love and it probably makes more sense to do it as a whole, rather than piecemeal. I could be wrong, I'm new here.

amateescu’s picture

amateescu commented

I think fixing the doxygen block of the function we're dealing with is in scope of this patch, and, yes, we need a different issue to fix the rest of the file.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

I think fixing the doxygen block of the function we're dealing with is in scope of this patch, and, yes, we need a different issue to fix the rest of the file.

Yeah I think its fine, its just a small change and the diff context is still the same / did not got extended.

yes, we need a different issue to fix the rest of the file.

Let's create a follow up for that: #2478355: Fix documentation style in core/lib/Drupal/Core/Database/StatementPrefetch.php

  • xjm committed 4f69708 on 8.0.x 
    Issue #2475177 by amateescu: SQLite: Fix system\Tests\Database\FetchTest
xjm’s picture

xjm
she/her
Primary language English
commented
Priority: Normal » Major
Status: Reviewed & tested by the community » Fixed
Issue tags: +sqlite

@m4olivei, you are actually correct by the way. :) And If the patch were bigger or the docblock updated were in a different hunk, I'd indeed push back on the coding standards update. But in this case I'm just going to commit it because in an 800-byte patch and the same hunk I'd prefer to just get this in, especially as it's a part of a critical.

I can't decide whether it's major as a test failure on a secondary environment or critical as a required part of the critical, but landing on major at least, and as a simple bug fix, it is prioritized during the beta. Committed and pushed to 8.0.x. Thanks @amateescu, @m4olivei, and @dawehner!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.