Improve support for SQLite in memory database

Hi,

After updating Drupal Core from 8.5.6 to 8.6.1 the initial patch does not apply.

I rerolled it.

Since there is a lot of changes in the Core between initial patch and this one I did not add an interdiff. I could try if someone prefer to.

This is not perfect. I think it misses some comments to explain why this is needed.
I did not do it because IMHO the original code is not clear enough and I think it needs to be refactored.
I'll like to have opinion of someone else on this.

Patch #4 seems to be OK. Applied on 8.6.2
Possibly we can check if $url_components['path'] === ':memory:' instead of just trying the first char at line 450, since this value is checked at line 81.

Putting the status of this issue to needs work, because of the feedback/review of @julienjoye.

@julienjoye can you confirm that you have manual tested this patch and it does what is suppost to do?

working on it.

Corrected from comment #7. Kindly review this patch.

@dhirendra.mishra Could you add the interdiff please? :)

I don't understand why you change migrate_drupal. Could you explain please?

i have added below line according to comment 5.
before : if ($url_components['path'][0] === '/' || $url_components['path'][0] === ':') {
After : if ($url_components['path'][0] === '/' || $url_components['path'][0] === ':memory:') {

needs review on this.

Hi dhirendra.mishra,

As noticed with the comment https://www.drupal.org/project/drupal/issues/2983452#comment-12828451,
your patch from https://www.drupal.org/project/drupal/issues/2983452#comment-12827945 contains modifications of the migrate_drupal module. Could you explain why?

I think this needs a proper test to avoid regression in the future and I still think as stated in https://www.drupal.org/project/drupal/issues/2983452#comment-12815429 that:

This is not perfect. I think it misses some comments to explain why this is needed.
I did not do it because IMHO the original code is not clear enough and I think it needs to be refactored.
I'll like to have opinion of someone else on this.

PS: When I asked for an interdiff I meant https://www.drupal.org/documentation/git/interdiff ;)

Hi dhirendra.mishra,

As I wrote on my comment, the full string variable $url_components['path'] should be evaluated to match with ':memory:' instead of just checking the first letter (as you do with [0]).

I fixed this with my patch. I provide an interdiff between mine and Beram's (#4), too.

However, this warns us about the fact that the currently set tests are missing something. Your patch still lets a :memory: file after tests run.
Then, maybe we can add a test that checks if we are in sqlite in :memory: mode during our Kernel tests, and checks that the ':memory:' file is not created at the drupal root.

What do you think?

Hi @julienjoye,

Thanks for the patch and the interdiff. I'll test it.

However, this warns us about the fact that the currently set tests are missing something. Your patch still lets a :memory: file after tests run.
Then, maybe we can add a test that checks if we are in sqlite in :memory: mode during our Kernel tests, and checks that the ':memory:' file is not created at the drupal root

That's because there is no test to cover the :memory: feature added by the patch ;)
The test you described cover the case described in this issue i.e. using SQLite in memory database when running the tests.
The patch allows to run a site with SQLite in memory database and not only when running tests.
We need to unit test those methods IMHO.

@julienjoye What do you think about the following statement?

This is not perfect. I think it misses some comments to explain why this is needed.
I did not do it because IMHO the original code is not clear enough and I think it needs to be refactored.
I'll like to have opinion of someone else on this.

Additionally to memstorage there's no way to point full path to database
for example sqlite:///mnt/tmp.sqlite

To summarise, we should be able to use:

- relative paths: sqlite://localhost/tmp/db.sqlite
- absolute paths: sqlite://localhost//tmp/db.sqlite
- in memory: sqlite://localhost/:memory:

I'm writing a unit test to cover this.

One question though, why did we add the following condition?

&& $this->connectionOptions['database'][0] != ':'

Added unit test in addition to #14.

Adding the missing namespace declaration to ConnectionTest.

The patch does not apply to 9.0.x/9.1.x because the "database name does not start with colon" condition collided with an upstream change that adds a "database file exists" condition.

I've merged and added both conditions - in theory, the file_exists condition should be false for the in-memory database anyway, but why not be safe.

Patching against 9.1.x and 8.9.x.

interdiff from 21->22 on 8.9.x

(no interdiff on reroll for 9.1.x)

Looks like 9.1.x wants return type hints on ::setUp().

@cburschka: Thank you for working on this. Patch looks good! Some minor points to resolve:

1. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/sqlite/ConnectionTest.php
   @@ -0,0 +1,63 @@
   +  const WEB_ROOT = '/var/www/html/web';
   

   Can we change this to: protected $root; and then in the setup() do: $this->root = dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__)), 2);. In that way it works in all test environments.

2. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -185,7 +185,7 @@ public function __destruct() {
   +          if ($count == 0 && $this->connectionOptions['database'][0] != ':' && file_exists($this->connectionOptions['database'] . '-' . $prefix)) {
   

   Can we change: $this->connectionOptions['database'][0] != ':' to: $this->connectionOptions['database'] != ':memory:'. It improves the readability of the code.

3. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/sqlite/ConnectionTest.php
   @@ -0,0 +1,63 @@
   +    $this->mockPdo = $this->createMock('Drupal\Tests\Core\Database\Stub\StubPDO');
   

   This can be moved to the test method. In this way we do not need the class variable. It is only used in one method. Also could you do the following change: createMock(StubPDO::class). It the new way of doing it on Drupal core and I think it helps generating an overview of which code is tested and which is not.

4. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/sqlite/ConnectionTest.php
   @@ -0,0 +1,63 @@
   +  public function getDatabaseUrls(): array {
   

   Could you change the name of the provider method to: "providerCreateConnectionOptionsFromUrl". For better readability.

Implemented the changes suggested in #26.

Corrected the failed testcase from #27.

Thanks @ridhimaabrol24!

I fixed the @dataProvider phpDoc comment that didn't match anymore with the new name of the provider method. About that, the new name of the provider method is not more readable IMO and we even loose the info about what it's doing. We should start method with a verb, like Symfony and us usually do for this (the PHPUnit doc is not a good example). So I renamed it to provideDatabaseUrls.

I added missing use statement for StubPDO.

In the meantime we find a working way I reverted dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__)), 2) to the constant since it didn't provide the web root but the Drupal root.

@ridhimaabrol24: Debugging test is a lot easier when you run the PHPUnit test locally. See: https://www.drupal.org/docs/8/testing/phpunit-in-drupal-8/running-phpuni...

@Kwadz: Could you add an interdiff.txt. It makes reviewing your patch a lot easier. Also the constant WEB_ROOT needs to go. The test needs to work on different environments. The number 2 needs to change to get the right directory, but for the rest it is the right way to do it (dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__)), 2)).

@daffie, all right, I switched to the var that uses dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__)), 2)), and added the interdiff.txt.

Oops, it seems I made a mistake with my patch command. Sorry guys. Going to fix it.

I have made the changes and tested on local. Although I am still facing an error with 'sqlite relative path' but I am updating the patch with interdiff provided from #29.

@ridhimaabrol24: I could very well be that the provider method does not get the right value and that $this->root = dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__)), 2); also needs to happen in the provider method.

Cleaned forgotten comment and renamed $root to $drupal_root.

The test runs OK on my local install which doesn't use a web/ directory.

Patch still needs some more improvement. Thank you all for working on this!

1. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/sqlite/ConnectionTest.php
   @@ -0,0 +1,62 @@
   +    $this->root = dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__)), 2);
   ...
   +    $database = $sqlite_connection->createConnectionOptionsFromUrl($url, $this->drupal_root);
   ...
   +      'sqlite relative path' => ['sqlite://localhost/tmp/test', $this->drupal_root . '/tmp/test'],
   

   You are mixing 2 different variables: $this->root and $this->drupal_root. Please make them the same. See my comments #32 and #36 for more info.

2. +++ b/core/tests/Drupal/Tests/Core/Database/Driver/sqlite/ConnectionTest.php
   @@ -0,0 +1,62 @@
   +   * @dataProvider provideDatabaseUrls
   ...
   +  public function provideDatabaseUrls(): array {
   

   Can we change the name of the provider method to: "providorCreateConnectionOptionsFromUrl".

Uploading the patch after implementing changes from #39. I have used the same variable $root at all places. Tested on local and it works fine.

I think would be great to test against SQLite stack(s) as we are working on an issue SQLite related. Queued testing against PHP 7.4 & SQLite 3.27

New patch uploaded, so setting back to Needs review. Thanks, @ridhimaabrol24!

@jungle: This patch only contains Unit tests. They are without a database connection and therefor only need to be tested against a single database (any will do).

@ridhimaabrol24: Thank you for working on the patch. I looks great, only we can make it a little better. This is because we needed to add the line: $this->root = dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__)), 2); to the provider method. We can now add the same line to the method "testCreateConnectionOptionsFromUrl()". When we do that the class variable $root and the setup() method can then be removed. After that the patch will be RTBC for me.

@daffie changes done! Please review.

@daffie, could you explain why in this case we shouldn't factorise dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__)), 2) (DRY principle) like we do elsewhere, for example in core/tests/Drupal/Tests/Core/Database/DatabaseTest?

About that, I don't understand why we don't use UnitTestCase::$root in that case.

@Kwadz: You are right that we should not repeat dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__)), 2). The problem is that when you set the value of $this->root in the method setUp(), for the provider method the value of $this->root does not get set by the method setUp() (it is still NULL). That makes the test of course fail. Therefor we need to set it twice. I took me the first time also by surprise. One of the strange things of PHPUnit testing. When this explanation is not enough, then please say so.

All the code changes look good to me.
The patch has been manual tested.
All my points have been addressed.
A CR has been created.
For me it is RTBC.

Great work everybody!

Yep the provider is run before setUp. It makes sense when you think about it.

+++ b/core/tests/Drupal/Tests/Core/Database/Driver/sqlite/ConnectionTest.php
@@ -0,0 +1,49 @@
+    $root = dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__)), 2);
...
+    $root = dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__)), 2);

Can this be dirname(__DIR__, 8)? I think it is 8 but that needs local testing... less to think about. And less methods involved and this is becoming more common in other tests.

Re #47, tested on my local, it's 8. Thanks @alexpott!

Point of @alexpott is addressed.
Back to RTBC.

So we already use in memory sqlite databases for testing :) We use them in migrate kernel tests. What we don't support is creating connections to them using our db url format which this patch adds so I'm re-titling.

I'm not a huge fan of the db url format because database connections cannot fully be represented as URLs or treated as such. sqlite: is not a valid URL scheme, for example. But that's out-of-scope here.

Also I'm not that convinced of the utility of using sqlite memory tests to speed up kernel testing but that is also not a reason to not fix this bug.

Unfortunately there's an unexpected change in the most recent patch.

+++ b/core/phpcs.xml.dist
@@ -149,6 +149,7 @@
+  <rule ref="DrupalPractice.InfoFiles.NamespacedDependency"/>

This change should not be here.

Sorry, my bad!

Removed the change of <rule ref="DrupalPractice.InfoFiles.NamespacedDependency"/>

My bad too. I only looked at the interdiff.txt file.
+1 for RTBC.

Looks like a random failure. Requeued. I will cycle back later, and set it back to RTBC if testing passes.

Back to RTBC.

Committed and pushed ce8dfe3d27 to 9.1.x and d19389aeb1 to 9.0.x. Thanks!

The patch didn't apply to 8.9.x. We can backport if someone wants to.

Backport to 8.9.x (just needed to undo the forward port from #22).

The patch has been successfully committed to a branch of the project, and still needs to be committed to another, but the current patch doesn't apply to the target branch and needs to be modified in order to do so.

See https://www.drupal.org/issue-queue/status#patch-to-be-ported

Patch (to be ported) means NW. From my understanding, the proper status should be RTBC if the rerolled/backported patch passed your review. And I guess @daffie intended to RTBC, but misunderstand the Patch (to be ported) status. So setting to RTBC.

Committed 7a75718 and pushed to 8.9.x. Thanks!

Publish the CR