Remove broken Fake DB driver

Great work! I recommend changing the comment in UserTest to getDatabase() will not create empty tables, so we populate with some data even if irrelevant or somesuch, the current language does not mesh with the intended level of professionalism of core.

For the sake of history, Fake was written in 2013 predating the sqlite option #1808220: Remove run-tests.sh dependency on existing/installed parent site so we couldn't rely on sqlite because the bots didn't have it yet. Probably it should now be removed from core as broken. Oh well, at least we tried.

Also, reset($rows) is twice in getDatabase.

Perhaps not in this issue but this could and should be extended to replace Fake with this in-memory sqlite version where the relevant insert/merge classes create their tables (while sqlite requires a schema it's typeless; the column specification is merely a type affinity but the values themselves can provide another affinity) and query return peacefully if a table is not found.

+  protected function saveMap(array $map) {

needs doxygen.

protected function getIdMap($database_contents = array(), $connection_options = array(), $prefix = '') {

Do we need those arguments now? Seems not.

+      $fields = array_map(function() { return ['type' => 'text']; }, $map);
+      $schema->createTable($table, ['fields' => $fields]);

This repeats. Perhaps a function in the base then?

$row['options'] = 'a:0:{}';

MMmmmm I guess. Perhaps serialize([]) ? You even changed a serialize(array()) to this, why...? Yes, I see it's in HEAD too but ... let's standardize on serialize([[]) please.

+    // $result needs to be countable, or it will fail certain assertions.

Perhaps the "return value" needs to be countable? I think that's what you meant.

@dataProvider lookupSourceIDMappingData

Core uses the word 'provider' in the name of the provider method in 410 out of the 435 provider methods. I'd say we should too.

+++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
@@ -29,11 +30,15 @@ class MigrateSqlIdMapTest extends MigrateTestCase {
+    'source_id_property' => array(
+      'type' => 'string',
+    ),

@phenaproxima I suggest using the new array [] syntax if you are reformatting or adding arrays.

+++ b/core/modules/migrate_drupal/tests/src/Unit/source/d6/NodeRevisionByNodeTypeTest.php
@@ -40,9 +40,97 @@ class NodeRevisionByNodeTypeTest extends MigrateSqlSourceTestCase {
 
+  protected $databaseContents = array(
+    'node' => array(
+      array(
+        'nid' => 1,
+        'type' => 'page',
+        'language' => 'en',

There are a few more array()s here and in some later files in the patch.

I think this is good to go then.

So no we have no usages of Fake in Core? I think this patch should also remove it.

Migrate tests that dependon SQLite should fail with a requirements error if it is not installed. See TestBase::checkRequirements()

Let's do this, then.

+++ b/core/modules/migrate/tests/src/Unit/MigrateTestCase.php
@@ -7,15 +7,16 @@
+use Drupal\Core\Database\Driver\sqlite\Connection;

@@ -54,21 +55,54 @@ protected function getMigration() {
+    $connection_options['database'] = ':memory:';

This creates a dependency on sqlite - if environment does not have sqlite this will fail hard. We should add a set up method to check that the PHP sqlite3 extension is installed. And previous these tests had no dependency on any database.

There is a ton of changing array syntax from array() to [] in the patch which is a shame since its large enough as it is.

Its odd, I mean we are testing something related to a database. Aren't we doing that in million other tests as well, so yeah I agree with @alexpott that we should not need something special, but I see you are using a pure unit test here.

These two points are definitely just my opinion but these are the reasons these tests are as they are.

It is important to see the migrate Drupal 6 sources being completely independent of Drupal 8. This makes KernelTestBase less usable. There's too much of D8 there. That alter in the select is quite wrong because it introduces a potencial circular dependency: load module -- try to load from cache -- select tries to alter -- modules are not loaded... kaboom! it doesn't because the cache doesn't add alter tags ATM. Because of this I have reverted the mock module handler from the patch and made Select Drupal independent again. DBTNG being Drupal independent was an important design consideration...

On the other hand I have serious problems with many unit tests. See, a typical query method does $this->select('files', 'f')->fields(... and that's it. Mostly. If you write $select_mock->expects()->method('fields)->willReturnSelf() or somesuch you could just as well parse the file and whether the method equals to a pre-stored copy of it character by character, it would roughly be equivalent. You are copying the code with a slightly altered syntax. Utterly useless. Whether the query actually selects anything useful you can only guess since the test is written as a copy-paste of the method tested. I'd rather create a database, put data in it, and see what I retrieve makes sense. If you want to be fancy: I do not see the point in unit testing a function with a cyclomatic complexity of 1. I know this is a personal thing but ... this is what you got when you accepted me as the migrate architect. You could, of course, rewrite this as "pure" unit tests. It's not something I would do but since I am not coding this, feel free. Same for using KTB.

Weird bot fail.

I totally agree that these tests don't make sense if they mock the database connection ...

There is a ton of changing array syntax from array() to [] in the patch which is a shame since its large enough as it is.

I agree with this, pointless noise in the patch which in turn made it much harder to review.

1. +++ b/core/lib/Drupal/Core/Database/Query/Select.php
   @@ -496,12 +496,23 @@ public function preExecute(SelectInterface $query = NULL) {
   -    if (isset($this->alterTags)) {
   +    if (!empty($this->alterTags)) {
   

   What changed around alterTags in that we now need an empty check instead?

2. +++ b/core/lib/Drupal/Core/Database/Query/Select.php
   @@ -496,12 +496,23 @@ public function preExecute(SelectInterface $query = NULL) {
   +      $module_handler = FALSE;
   +      try {
   +        if (class_exists('Drupal')) {
   +          $module_handler = \Drupal::moduleHandler();
   +        }
   +      }
   +      catch (\Exception $e) {
   +        // No container yet, queries can't be altered.
   +      }
   +      if ($module_handler) {
   +        $module_handler->alter($hooks, $query);
   +      }
   

   Why do we check for the Drupal class, is it the container we should be testing for?

3. +++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
   @@ -43,41 +48,64 @@ class MigrateSqlIdMapTest extends MigrateTestCase {
   +      foreach (array_keys($map) as $field) {
   +        if (!$schema->fieldExists($table, $field)) {
   +          $schema->addField($table, $field, ['type' => 'text']);
   +        }
   +      }
   

   Isn't this what crreateSchemaFromRow() does?

4. +++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
   @@ -85,11 +113,16 @@ protected function getIdMap($database_contents = array(), $connection_options =
   +    if ($this->database instanceof SQLiteConnection) {
   +      $defaults['source_row_status'] = (string) $defaults['source_row_status'];
   +      $defaults['rollback_action'] = (string) $defaults['rollback_action'];
   +    }
   

   This could do with a comment, it just casts the values to a string?

5. +++ b/core/modules/migrate/tests/src/Unit/MigrateTestCase.php
   @@ -54,21 +55,54 @@ protected function getMigration() {
   +      throw new \Exception('pdo_sqlite extension is required.');
   

   So the tests only work with the pdo_sqlite extension?

To quote #23

DBTNG being Drupal independent was an important design consideration

I am not sure what else needs to be said? Hardwiring Drupal:: there is probably a bug and just calling moduleHandler without try-catch, as explained, again, in #23 is definitely a bug.

OK reviewed again, i'm glad we removed the stuff from DBTNG from this patch. I think this is ready and a good clean-up.

+++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
@@ -698,18 +742,32 @@ public function testDestroy() {
-    $id_map = $this->getIdMap(array(), array('database' => 'source_database'));
-    $qualified_map_table = $id_map->getQualifiedMapTableName();
-    $this->assertEquals('source_database.migrate_map_sql_idmap_test', $qualified_map_table);
+    $this->database = $this->getDatabase([] , ['database' => 'source_database']);
+
+    if ($this->database->driver() == 'sqlite') {
+      $this->markTestSkipped('The SQLite driver does not support the DATABASE.TABLE format.');
+    }
+    else {

Well this is always going to be skipped now. Since the database is sqlite. What's the point of having this test here? Should we move it to a KernelTestBase test? Also what does this mean about migrating from Drupal installations that use SQLite?

618 insertions, 2412 deletions. nice work! Less to maintain for all of us. Committed fa2a7a5 and pushed to 8.0.x. Thanks!

Migrate is not under beta evaluation.

Nice cleanup! Sadly, this causes a new fail in MigrateSqlIdMapTest for all non MySQL databases (PG-bot / SQLite-bot). Do we just need a follow-up here or new issues for both DB's?
Drupal\Tests\migrate\Unit\MigrateSqlIdMapTest 28 passes 1 fails

@bzrudi71 let's handle this in a new issue.

Created #2504417: Fix Drupal\Tests\migrate\Unit\MigrateSqlIdMapTest::testGetQualifiedMapTablePrefix()

I would say this needs a change record, but I cannot set this status, for some reason : this class was there for a long time, and initializing this SQLite connection uses significantly more code than initializing the FakeConnection did, so there is a risk to have the initialization code be replicated in various guises.

I met this problem in the MongoDB driver, FWIW.

@fgm, thanks for that, do you want to write a change record and post it here?