FIX Tighten transactions

[NightJar]

Fixes #48

Thanks to @dhensby's work to add (nested) transaction support, there were only a few outstanding issues to tidy up in order to get tests to run without error (caused by this module).

The crux of the issue was that table alterations were using transactions but not via the API for it, which was causing issues. Some inspection to reset nesting levels if a query contained DDL was causing nesting errors - now removed and the empty method deprecated.

The logic around transactions is generally tidied up by introducing a few protected methods and supporting variables.

[maxime-rainville]

I've rebased ... test should be all green :crossed_fingers:

[NightJar]

But it was alre... oh because of the other PR being merged. I don't think rebasing is necessary, restarting the job should have been enough :) The recent merge is non-conflicting, and Travis should check out the 2 branch HEAD when running.

[maxime-rainville]

Didn't know travis did that.

Not all test are greens, but it doesn't look like it's related to that.

[NightJar]

@maxime-rainville yeah, it's merging to a HEAD so should ideally test against it, no matter where that HEAD is at the time of PR creation :) It's rebasing onto a HEAD that causes untold pain IMO :/ 🤔 I might look at those other two issues in case they're created by this (I don't think they are, but it's probably worth checking). Even if not, it'd be nice to have the module green again, looks like it's been red for some time.

[NightJar]

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

Can't reproduce at home :/

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

Correct behaviour. The index is defined as fulltext - SQLite3 does not support this without an extension, and is not defined in that manner (i.e. as MySQL does it) - something somewhere seems to convert it to index as below , and of course "fulltext" != "index" so the table is marked for update.

On create: https://github.com/silverstripe/silverstripe-sqlite3/blob/7add192ebf68a5f43c2a7bccf89e2b7c7806899a/code/SQLite3SchemaManager.php#L395-L401 And on read (indexList): https://github.com/silverstripe/silverstripe-sqlite3/blob/7add192ebf68a5f43c2a7bccf89e2b7c7806899a/code/SQLite3SchemaManager.php#L441

I'll see if I can find a way around this.

[NightJar]

Not too pretty, but considering SQLite makes fulltext virtual tables (as opposed to indexes) and the pre-existing code silently converting index types, I figured the latest commit is a middleground that'll pass the testIndexesDontRerequestChanges bar.

[NightJar]

I don't understand travis enough to deal with that last one.

[maxime-rainville]

It's still an improvement over what was there previously. I'm happy to merge as-is and we can address outstanding failures in a separate pull request.

[robbieaverill]

This is a minor change FWIW, purely because of the added deprecation. If you wanted to take that out and make a separate PR for it then we could merge this as a patch change.

[robbieaverill]

I think we'll need to check for compatibility with @sminnee's work in https://github.com/silverstripe/silverstripe-framework/pull/8448, related to transactions

[NightJar]

I've not targeted 2.1 on purpose - ideally I'd like to abandon the 2.x release line (it should never have been stabilised) - but if by chance someone has a constraint set to this line then I feel it fair to at least give a minor version, giving a semi-significant indication that there has been change to stabilise the module... particularly in a release line where version numbers have meant nothing.

That and I'm a bit too lazy to evaluate what looks like API changes in master :>

[robbieaverill]

Hopefully somebody read my comment, it could require further changes here once merged

[NightJar]

I did take a look at Sam's work, and although there are a fair amount of change going on the focus is mostly around MySQL, also accounting for PgSQL and MSSQL. We could reflect those changes here, but I don't see any reason that it couldn't be in a future PR for a future release, because it all looked very backwards compatible.

[robbieaverill]

Great, sounds like you're all over it. Thanks for investigating

[maxime-rainville]

@robbieaverill Sorry, I read your comment as something that needed to be kept in mind for the future, but wasn't an immediate issue. My bad.

[NightJar]

Regardless, as it stands this module can be used with core now, as opposed to when the linked work is merged, so that's still a win I think :)

[robbieaverill]

All good :-)