The SQLite date handling is wrong and needs some love and care. The handling of arguments is not prefect and needs to have workaround for the expansion of numeric values in the SQL statement, since http://bugs.php.net/bug.php?id=45259 is still unresolved. The handling of arguments will now be done by SQLite's Connection::expandArguments and as a result SQLite's Statement class can be removed. For the argument handling are some extra tests added.

The SQLite date handling and the creation of a arguments override function is combined because the SelectPagerDefaultTest only passes if both problems are fixed.

Solution

  • Update the SQLite date handling
  • Create SQLite Connection::expandArguments override function
  • Remove the SQLite statement class. The default database statement class works just fine for SQLite.

Testing
php ./scripts/run-tests.sh --dburl sqlite://localhost/sqlite/db.sqlite --class 'Drupal\system\Tests\Database\ConnectionUnitTest','Drupal\system\Tests\Database\SelectPagerDefaultTest'
There should be no fails and no exceptions.

Commit credits
Please give @sun commit credits for this issue. All the code from the patch from the first comment is originally written by @sun.

CommentFileSizeAuthor
#9 interdiff.txt782 bytesamateescu
#9 2428695-9.patch14.21 KBamateescu
#4 interdiff.txt1.57 KBamateescu
#4 2428695-4.patch14.2 KBamateescu
#1 2428695-1.patch14.01 KBdaffie
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Active » Needs review
FileSize
2428695-1.patch14.01 KB
daffie’s picture

daffie CreditAttribution: daffie commented

If I run the test locally I get the following result:


Drupal test run
---------------

Tests to be run:
  - Drupal\system\Tests\Database\ConnectionUnitTest
  - Drupal\system\Tests\Database\SelectPagerDefaultTest

Test run started:
  Wednesday, February 18, 2015 - 12:38

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

Drupal\system\Tests\Database\ConnectionUnitTest               28 passes
Drupal\system\Tests\Database\SelectPagerDefaultTest           22 passes

Test run duration: 6 min 8 sec
daffie’s picture

daffie CreditAttribution: daffie commented

This issue is the last one for the critical issue #2318191: [meta] Database tests fail on SQLite.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs review » Reviewed & tested by the community
FileSize
2428695-4.patch14.2 KB
interdiff.txt1.57 KB

I reviewed this patch and I just found a few minor problems, fixed with the interdiff attached.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work

I get fails when running Drupal\system\Tests\Database\ConnectionUnitTest with this patch.

Fail      Other      ConnectionUnitTes  269 Drupal\system\Tests\Database\Connec
    Unserialized Connection property 'prefixes' value array (
      'default' => 'simpletest814757',
    ) is equal to expected array (
      'default' => 'simpletest814757.',
    )
Fail      Other      ConnectionUnitTes  269 Drupal\system\Tests\Database\Connec
    Unserialized Connection property 'prefixReplace' value array (
      0 => 'simpletest814757',
      1 => '',
    ) is equal to expected array (
      0 => 'simpletest814757.',
      1 => '',
    )
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Also this fix causes no postgres fails in Drupal\system\Tests\Database\QueryTest...

Fail      Other      QueryTest.php       77 Drupal\system\Tests\Database\QueryT
    Value true is identical to value '1'.
Fail      Other      QueryTest.php       82 Drupal\system\Tests\Database\QueryT
    Value true is identical to value '1'.
amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs work » Needs review

Re #5: Yes, that's documented as a failing test with this patch applied in this issue of the new SQLite meta: #2454747: SQLite: Fix sub-system tests in system test group.

The problem there is that the prefix on SQLite will never be the same as the one for MySQL and PostreSQL because the sqlite driver adds a period suffix to it in the constructor of its Connection class. In order to fix the test, we need to either provide a driver-specific test override or we can make the test smarter, but I think that's best to be discussed in the issue mentioned above.

Re #6: Since the new test works for MySQL and SQLite, should we open a new issue to fix it in the Postgres db driver?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Needs work

Re #7/#6 this causes the postgres fails so I think not.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Assigned: Unassigned » alexpott
Status: Needs work » Reviewed & tested by the community
FileSize
2428695-9.patch14.21 KB
interdiff.txt782 bytes

I looked into why this new test fails on PostgreSQL and it seems that the return value for comparison operators is boolean TRUE/FALSE on Postgres and 1/0 on MySQL, so the current assertions will never work on all engines.

But the intention of the test is to check the expansion of numeric arguments (i.e. :count expands to 3), not the return value of comparison operators, so we can just tweak the assertions a bit.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 0fafbc3 and pushed to 8.0.x. Thanks!

  • alexpott committed 0fafbc3 on 
    Issue #2428695 by amateescu, daffie: SQLite date handling is wrongly...
amateescu’s picture

amateescu CreditAttribution: amateescu commented
Component: database system » sqlite db driver
Assigned: alexpott » Unassigned

The assertion from #2318191: [meta] Database tests fail on SQLite that SQLite's custom PDO statement is not needed anymore was false, so this patch is being reverted and a proper fix is implemented in #2465633: Bring back the custom Statement class for the SQLite driver.

Status: Fixed » Closed (fixed)

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