Clean up and test migrate executable and sql idmap

Only went through MigrateExecutable class.
Read all the docs. (made some changes)
read all the changed lines of code in the patch. (looked good)
read only some of the rest of the lines of code in the class.

1. entity_get_controller is depreciated, do we want to fix that in this issue?

2. left the TODO:'s in here. @chx said in irc they will be fixed in this issue.
except for the Handle config id one, so I reformatted that and made an issue for it.

3. phpstorm suggested adding break; in the case. seemed to make sense to me.
and @throws in processRow()

4. added some types to @param and @return
and missing doc on getTimeLimit() (these maybe have been the only core gate blockers so far)

5. added type hint for \Exception in:
public function handleException(\Exception $exception, $save = TRUE) {

6. small things:

a. moved some properties that were in with the methods up with the other properties.

b. made method first sentence a one line summary, starting with third person verb.

c. added punctuation to some sentence ends.

d. changed boolean to bool

e. made indent under @param and @return 2 spaces (instead of one)

f. used (optional) in optional @params

https://drupal.org/node/1354 lists those things.

q: tiny nit (very not a core gate that should not block this) question,
this uses quotes like

   * - 'message': The message string.

Examples in https://drupal.org/node/1354 are just like:

 *   - foo: The foo...

core has quotes like that 377 times though.. grep -R " - \'" core | grep -v yml | grep -v vendor | wc -l

Noting for next change someone finds, we can do as @tim.plunkett said in irc:

diff --git a/core/modules/migrate/lib/Drupal/migrate/Entity/MigrationInterface.php b/core/modules/migrate/lib/Drupal/migrate/Entity/MigrationInterface.php
index 1fa3dad..7fb5f5e 100644
--- a/core/modules/migrate/lib/Drupal/migrate/Entity/MigrationInterface.php
+++ b/core/modules/migrate/lib/Drupal/migrate/Entity/MigrationInterface.php
@@ -63,7 +63,7 @@ public function getSourcePlugin();
    * @param array $process
    *   A process configuration array.
    *
-   * @return array
+   * @return \Drupal\migrate\Plugin\MigrateProcessInterface[]
    *   A list of process plugins.
    */
   public function getProcessPlugins(array $process = NULL);

And then in processRow(), $plugin will be able to recognize methods getPluginDefinition, transform, multiple.

nice.

and it's in https://drupal.org/node/1354#types too

oh. I see now. The g needs to fall into each case so that it is 1024*1024*1024. Thanks chx.

going to start reviewing the next bit.

1. I tried the suggestion in #4, but it didn't help when I did the code inspection in phpstorm. I'm not sure what I did wrong.
2. I dont know if I can really add {@inheritdoc} to things like FakeConnection queryRange() since it throws an exception not listed in the docs of the method it is overriding.

   I did anyway. Will wait for opinion. [edit: added link to issue:] We should help with #1994890: Allow {@inheritdoc} and additional documentation

3. moved methods in FakeConnection to be in the same order as the methods on the class it was extending, Database/Connection just to be tidy. I would not have reordered methods, but this is a new class, so it wont effect the patch size.

   I checked each was there and had docs.

   Note that Database/Connection.php is missing @param and @returns and types on many things.

4. FakeMerge inherits execute() method docs, but QueryMerge didn't have any docs to inherit. :(

5. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/FakeMerge.php
   --- a/core/modules/migrate/tests/Drupal/migrate/Tests/FakeSelect.php
   +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/FakeSelect.php
   
   +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/FakeSelect.php
   @@ -51,21 +51,20 @@ class FakeSelect extends Select {
      /**
       * Constructs a new FakeSelect.
       *
   +   * @param array $database_contents
   +   *   An array of mocked database content.
       * @param string $table
       *   The base table name used within fake select.
       * @param string $alias
       *   The base table alias used within fake select.
   -   * @param array $database_contents
   -   *   An array of mocked database content.
   -   *
       * @param string $conjunction
       *   The operator to use to combine conditions: 'AND' or 'OR'.
       */
   -  public function __construct($table, $alias, array $database_contents, $conjunction = 'AND') {
   +  public function __construct(array $database_contents, $table, $alias, $conjunction = 'AND') {
   +    $this->databaseContents = $database_contents;
   

   why was this param reordered in FakeSelect? Select, which it extends had the original order. [edit: no Select was different. This is reordering what FakeSelect was, as chx answered, to make it consistant with the other FakeThings. ok]

6. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/FakeTruncate.php
   @@ -0,0 +1,20 @@
   +class FakeTruncate {
   

   should this have extended Query/Truncate?

7. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/MigrateExecuteableMemoryExceededTest.php
   @@ -0,0 +1,126 @@
   +   * @param int $memory_usage_second
   +   *   Used to set the second time the memory usage is queried: the first time
   +   *   times this many will be returned.
   

   the first time times this many?

8. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/MigrateSqlIdMapEnsureTablesTest.php
   @@ -0,0 +1,192 @@
   +   * Test the ensureTables method.
   +   */
   +  public function testEnsureTablesNotExist() {
   ...
   +  public function testEnsureTablesExist() {
   

   does the first test when the tables do not exist, and the second test when they do? I'm not sure what to use for the method descriptions.

9. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/MigrateSqlIdMapEnsureTablesTest.php
   @@ -0,0 +1,192 @@
   +   * Actually run the
   +   *
   +   * @param $schema
   

   I think this was actually run the test.

   also, what is the type for $schema?

10. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/MigrateSqlIdMapTest.php
    @@ -0,0 +1,736 @@
    +  protected $mapJoinable = FALSE;
    

    several of the classes have a property like this, what is it for? I'm not sure what the description should be.

I did not finish, but posting to get feedback, and also get answers to some questions, since am repeating some patterns.

I did not yet read all the lines of code in the classes I did go through, mostly just docs standards stuff.

+++ b/core/modules/migrate/tests/Drupal/migrate/Tests/MigrateTestCase.php
@@ -54,43 +58,8 @@ protected function getMigration() {
+  protected function getDatabase($database_contents, $connection_options = array(), $prefix = '') {
+    return new FakeConnection($database_contents, $connection_options, $prefix);

a. is this creating the database to be used in the test? I think so, but we usually use get functions to get stuff that already exists, so this threw me off for a bit.

b. Are $database_contents of type \PDO or \Drupal\Core\Database\Connection ?
nope. @chx points out that the *point* is it is fake, not a database or connection. It is: an array , each key is a table name, each value is a list of table rows

Fixed the fake database contents stuff. Didn't put @see to FakeSelect, but that seemed like a good idea.

talked with @tim.plunkett and worked out the type hinting to get the methods recognized. it was an array of an array of...

Did more docs and type stuff. I know it is in tests and seems a bit silly.

changed some names of somethingIDWhatever to somethingIdWhatever

and sql to SQL and id to ID in comments

migrate/Tests/process/DedupeEntityTest.php changes accidentally got into the patch. took those changes out. @chx said that will be handled in #2152355: Refactor the entity dedupe test to use dataProvider

Next, check if I messed up anything (see if there are no test fails) and would be good to actually read and understand.

to address #14, should:

core/modules/migrate/tests/Drupal/migrate/Tests/MigrateTestCase.php
@@ -56,7 +56,7 @@ protected function getMigration() {
}

/**
- * Creates a fake database for use in tests.
+ * Gets the fake database for use in tests.
*
* @param array $database_contents

---

I'm not sure why I can't rename the file core/modules/migrate/tests/Drupal/migrate/Tests/TestSqlIdmap.php to core/modules/migrate/tests/Drupal/migrate/Tests/TestSqlIdMap.php

when I apply the patch, I can ls both core/modules/migrate/tests/Drupal/migrate/Tests/TestSqlIdmap.php and core/modules/migrate/tests/Drupal/migrate/Tests/TestSqlIdMap.php

So weird.

---

off for a while though. so it's available for someone else.

This testcoverage is really impressive!!

1. +++ b/core/modules/migrate/lib/Drupal/migrate/Entity/MigrationInterface.php
   @@ -63,8 +63,9 @@ public function getSourcePlugin();
   +   * @return \Drupal\migrate\Plugin\MigrateProcessInterface[][]
   

   Just wonder whether this really works.

2. +++ b/core/modules/migrate/lib/Drupal/migrate/MigrateExecutable.php
   @@ -462,34 +468,36 @@ protected function checkStatus() {
   +   *   TRUE if the threshold is exceeded, FALSE if not.
   

   We use "otherwise FALSE." most of the time.

3. +++ b/core/modules/migrate/lib/Drupal/migrate/MigrateExecutable.php
   @@ -462,34 +468,36 @@ protected function checkStatus() {
   +      if ($pct_memory > (.90 * $threshold)) {
   

   I kinda hate to see that also drupal uses that . shortcut. Can't we just add "0" in front of it?

4. +++ b/core/modules/migrate/lib/Drupal/migrate/MigrateExecutable.php
   @@ -509,36 +517,80 @@ protected function memoryExceeded() {
   +    // @TODO: explore kernel reset.
   

   We could also maybe reset some of the container.

5. +++ b/core/modules/migrate/lib/Drupal/migrate/MigrateExecutable.php
   @@ -509,36 +517,80 @@ protected function memoryExceeded() {
   +      if ($pct_time > $this->migration->get('timeThreshold')) {
   +        return TRUE;
   +      }
   

   We could save the if by using return $pctime > $this...

6. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/id_map/Sql.php
   @@ -444,8 +444,8 @@ public function saveMessage(array $source_id_values, $message, $level = Migratio
   +      if (!isset($id_value)) {
   ...
   -      if (empty($id_value)) {
   

   +1

7. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/id_map/Sql.php
   @@ -444,8 +444,8 @@ public function saveMessage(array $source_id_values, $message, $level = Migratio
            print($message);
            return;
   

   Wait, we print out messages?

8. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/MigrateExecutableTest.php
   @@ -7,9 +7,10 @@
   @@ -38,7 +39,7 @@ class MigrateExecutableTest extends MigrateTestCase {
   

   All this code is damn impressive!

9. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/TestSqlIdMap.php
   @@ -0,0 +1,45 @@
   +   * {@inheritdoc}
   +   */
   +  public function getDatabase() {
   +    return parent::getDatabase();
   +  }
   ...
   +   */
   

   This is not needed

---

The interdiff looks great!

As @chx said here is a changes from #2168277: Migrate mapping tables hash column should be varchar 64

Just a small nitpicks

Overall looks great, a couple of smallish things:

1. +++ b/core/modules/migrate/lib/Drupal/migrate/Entity/Migration.php
   @@ -175,6 +175,16 @@ class Migration extends ConfigEntityBase implements MigrationInterface {
   +   * envelope. Defaults to 85%.
   

   This is a decimal notation of a fraction. When I see fraction I usually thing 1/5 or whatever. Then using 85% is yet a third way to say the same thing. We should pick one and stick to it.

2. +++ b/core/modules/migrate/lib/Drupal/migrate/Entity/Migration.php
   @@ -185,6 +195,23 @@ class Migration extends ConfigEntityBase implements MigrationInterface {
   +   * @var float
   

   Same again.

3. +++ b/core/modules/migrate/lib/Drupal/migrate/MigrateExecutable.php
   @@ -386,6 +394,12 @@ protected function timeOptionExceeded() {
   +   *   The time limit, NULL if no limit or if the units were not in seconds.
   

   When would the unit not be in seconds? Should that be an exception rather than null, or is it a valid case that we just don't care about here?

4. +++ b/core/modules/migrate/lib/Drupal/migrate/MigrateExecutable.php
   @@ -509,36 +526,73 @@ protected function memoryExceeded() {
   +    drupal_static_reset();
   

   This is getting less and less true, i.e. our entity caching is now in class properties, but I know it's just moved around here and there's already a @todo for the container.

5. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/FakeConnection.php
   @@ -0,0 +1,160 @@
   +class FakeConnection extends Connection {
   

   This is nifty. However should it be 'mock' connection?

   Also presumably this could be used for non-migrate tests?

Committed/pushed to 8.x, thanks!

Made this very minor issue to track the bit about what to do when units is not in seconds.

#2172351: for migrations when would time not be in units of seconds