Delay sql map table creation

Added related issue.

In the tests, the class \Drupal\Tests\migrate\Unit\TestSqlIdMap is used, which extends the class core/modules/migrate/src/Plugin/migrate/id_map/Sql.php you are changing in this patch.

TestSqlIdMap already sets the property $this->database to a SQLite test database, so \Drupal::database() would never have to be called in the tests.
However in the patch you are explicitly reassigning the database property to \Drupal::database() in Sql class.

This issue can be solved by adding a check whether the database property is already set.

I wonder why the database can't be provided through dependency injection in the Sql plugin class. That would make more sense, but there's probably a valid reason not to do it... I'm not unsure of how this plugin works in detail...

Attached is an updated patch that should no longer break the tests.
Not sure if the test coverage will need to be changed, based on the changed logic in this patch though.

Just a couple of nits - and, of course, we need a test.

1. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
   @@ -717,12 +714,19 @@ public function messageCount() {
   +  protected function countHelper($status = NULL, $table = NULL) {
   

   Update docs to reflect $status is now optionally an array.

2. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
   @@ -717,12 +714,19 @@ public function messageCount() {
   +    } catch (DatabaseException $e) {
   

   Coding standards - put catch on separate line.

Thanks!

Last patch never went through testbot, let's see if it still applies.

Re-rolled.

Setting back to NW for adding tests.

Re-rolled.

Back to NW for tests.

@heddn Can you provide any guidance on writing the tests for this? I had a look when I did the re-roll, but couldn't work it out.

I think we need to do a db table exists check when we don't want the table to exist. Then bootstrap things further and expect the table is now created.

Added test.
(no interdiff because the only change is in the test_only patch)

I see what we are trying to do here. But I wonder if a better fix is to remove init() from getDatabase(). Classically, a getter shouldn't be doing anything but, well, getting.

I totally agree, @heddn, but I'm not sure where the call to init() should go. Should it be not protected, perhaps?

Yes, it would be nice to remove init() from getDatabase. However that was not introduced by the patch, and I suggest it is outside the scope of the issue. How about a new issue for removing init() from getDatabase and this can, probably, be RTBC?

Setting to NR for feedback.

Let's add the new issue to refactor init(). And then this is good to go.

Created #2939767: Remove init() call from Drupal\migrate\Plugin\migrate\id_map\Sql::getDatabase() so this can now be RTBC.

I was expecting the patch here to do what the follow-up will, but I appreciate the performance improvement here as well, so let's keep going in the follow-up (also commented there).

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

Look like it broke HEAD:
https://www.drupal.org/pift-ci-job/870706
https://www.drupal.org/pift-ci-job/870735

Yep I confirmed this broke HEAD.

Sorry, I didn't realise it was only tested on 8.4.x when I set to RTBC.

A couple of alterations resolve the test failures:

1. Use the same database connection for the insert statements in the tests.
2. When looping through the map tables to get the highest ID, continue to the next table rather than breaking out of the loop. This avoids missing the page map table that does exist.

@Jo Fitzgerald, thanks for fixing this. I was about to do this last review before bed but I can't apply the patch to 8.5.x or 8.6.x. Sorry, needs a reroll.

I'm rerolled patch from #36
Please review

@aleevas, thanks for the patch, and one that passed tests.

I was wondering why the patch is nearly twice the size of the previous patch and I now see it. The patch contains a diff of itself. Can you remake the patch so that is does not include a diff of the patch? Thanks.

1. diff --git a/core/2817833-36.patch b/core/2817833-36.patch
   

Oops.

Remove the patch diff.

Attached interdiff for patches 38 and 40.

@bhanuprakashnani, Welcome to Drupal! Thanks for making the interdiff.

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
@@ -977,7 +982,7 @@ function (array $id) {
+        continue;

Is this change necessary? Won't break and continue do the same in this case? And reverted this change and MigrateDrupal6AuditIdsTest and MigrateSqlIdMapTest, the two of the three tests that have previously failed, passed tests.

@quietone, according to what you stated in the previous comment, the task now is to undo the changes made in MigrateDrupal6AuditIdsTest, MigrateSqlIdMapTest and changing 'continue' back to 'break' right?

@chiranjeeb2410, I'm only questioning the change from break to continue, not the test files. So, a patch that reverts that one line would be good. It would either confirm my local testing or prove me wrong.

@quietone,

Reverting 'continue' to 'break' as stated. Should do the trick.

Fresh patch uploaded with above mentioned changes.

@quietone continue moves onto the next item in the loop, while break exits the loop completely without addressing the remaining items. I have added a comment to help clarify things.

@Jo Fitzgerald, well that is what I thought until I doubled checked the php manual. And then I proceeded to not understand the words on the screen! Definitely a case where RTFM did not work, or rather it was me not working. ;-) Thank you for taking the time to add a comment.

I found one thing, the new test is missing a docblock. Sorry I didn't see it earlier.

+++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
@@ -1010,4 +1010,23 @@ private function getIdMapContents() {
+  public function testMapTableCreation() {

Missing doc block

@quietone,

Added missing docblock. Go green.

@chiranjeeb2410 The docblock needs some content, not {@inheritdoc}, see the other test functions in the file for inspiration.

@Jo Fitzgerald,

Uploaded fresh patch as per suggestion. Please review.

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
index 80c55ef..2a6e0ac 100644
--- a/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php

+++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
@@ -1010,4 +1010,27 @@ private function getIdMapContents() {
+   * plugin.

Small nit, but I think I would prefer something like: "Tests the MapTableCreation method". Simple is sometimes better.

@heddn,

Changing it right away. Seems better.

Did we miss uploading a patch in #54?

@heddn,

Done as suggested in #53. Please review.

Super nit: I don't see the parenthesis after the method name in the rest of this test class Can we stay consistent and remove those?

@heddn,

Done as suggested. Please review.

Seems like NW for #59.

@vaplas, what needs to be finalised from #59 for fixing this issue as of now?

Sorry, I didn't notice the uploaded patch in #59. It looks fine to me. Thanks for your fine work.

@vaplas,

Change suits the requirements. RTBC for me too.

Adding reviewer credit.

Committed e96ab4e and pushed to 8.6.x. Thanks!

diff --git a/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
index 0451dbc7d9..bc849916d7 100644
--- a/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
+++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
@@ -1011,7 +1011,7 @@ private function getIdMapContents() {
   }
 
   /**
-   * Tests the delay creation of the "map" and "message" migrate tables.
+   * Tests the delayed creation of the "map" and "message" migrate tables.
    */
   public function testMapTableCreation() {
     $id_map = $this->getIdMap();

Very minor update to the docs on commit.

Discussed the crediting of @catch and myself with other committers. @gaborhojtsy said:

@alexpott I think we do, assuming catch reviewed it when he committed and you figured it needs backport, etc :) those are contributions :)