silverstripe / silverstripe-sqlite3

SQLite3 DB Adapter for Silverstripe
BSD 3-Clause "New" or "Revised" License
8 stars 19 forks source link

return 0 for non iterable results #43

Closed lekoala closed 6 years ago

lekoala commented 6 years ago

If there are no columns, it's not a iterable result set and we can return 0. This fixes issues with things like CREATE statement.

lekoala commented 6 years ago

@dhensby this is much better than the other one :-)

dhensby commented 6 years ago

So one of those failures, at least, is caused by the fact that #39 isn't merged yet.

One is pre-existing, and one may well be a regression.

lekoala commented 6 years ago

I'm not sure, the nested transaction thing does not seem related. As for the index, maybe the system relies somehow on the count to be something else than 0? Maybe the default behavior is to return 1 for successful queries?

dhensby commented 6 years ago

These are the current failures:

There were 3 failures:

1) SilverStripe\ORM\Tests\DataListTest::testAggregateFilters
Failed asserting that actual size 0 matches expected size 1.

/home/travis/build/silverstripe/silverstripe-sqlite3/vendor/silverstripe/framework/tests/php/ORM/DataListTest.php:1366
/home/travis/build/silverstripe/silverstripe-sqlite3/vendor/phpunit/phpunit/phpunit:52

2) SilverStripe\ORM\Tests\DataObjectSchemaGenerationTest::testIndexesDontRerequestChanges
Failed asserting that true is false.

/home/travis/build/silverstripe/silverstripe-sqlite3/vendor/silverstripe/framework/tests/php/ORM/DataObjectSchemaGenerationTest.php:145
/home/travis/build/silverstripe/silverstripe-sqlite3/vendor/silverstripe/framework/src/ORM/Connect/DBSchemaManager.php:159
/home/travis/build/silverstripe/silverstripe-sqlite3/code/SQLite3SchemaManager.php:120
/home/travis/build/silverstripe/silverstripe-sqlite3/vendor/silverstripe/framework/tests/php/ORM/DataObjectSchemaGenerationTest.php:147
/home/travis/build/silverstripe/silverstripe-sqlite3/vendor/phpunit/phpunit/phpunit:52

3) SilverStripe\ORM\Tests\TransactionTest::testNestedTransaction
Failed asserting that two strings are equal.

--- Expected
+++ Actual
@@ @@
-'roll back transaction'
+'Couldn't run query:
+
+BEGIN
+
+HY000-1: cannot start a transaction within a transaction'

/home/travis/build/silverstripe/silverstripe-sqlite3/vendor/silverstripe/framework/tests/php/ORM/TransactionTest.php:47
/home/travis/build/silverstripe/silverstripe-sqlite3/vendor/phpunit/phpunit/phpunit:52

1) is an existing failure 2) is an existing failure 3) is the nested transaction failure


edit: all failures are existing

dhensby commented 6 years ago

@lekoala does this fix your local problem? could you write a test to cover it then we can merge?

lekoala commented 6 years ago

Sure, I'll have a look at the framework test suite and see if I can add something. Basically it's adding a test to cover queries like "CREATE, ALTER etc.." when calling numRecords on their result.

dhensby commented 6 years ago

Sure, I'll have a look at the framework test suite and see if I can add something. Basically it's adding a test to cover queries like "CREATE, ALTER etc.." when calling numRecords on their result.

when does that happen?

lekoala commented 6 years ago

almost never I guess, but I had the issue when running the debugbar test suite using the in memory sqlite driver.

dhensby commented 6 years ago

ok, well, if this resolves your problem, we can take it in..

Can you confirm other drivers return 0 for these commands too?

lekoala commented 6 years ago

other drivers return null if there is no handle, or an integer. Both works so it's really a matter of choosing one.

i just bumped again into this issue when running test using sqlite3 so I think it's worth merging because I can't be the only one to have the issue

Sample error:

Warning: SQLite3Result::fetchArray(): Unable to execute statement: table "VirtualPage_Versions" already exists in ..\vendor\silverstripe\sqlite3\code\SQLite3Query.php on line 68

dhensby commented 6 years ago

@lekoala ok - do you think you can write a test, or shall I just merge it? it seems pretty safe to me... @tractorcow ?