Improve source provider missing exception message.

Nice. I do like getting an idea of how to fix the problem but I think a statement is better than a question. Still, I'm just not sure what is 'proper' language to use here.

   Migration d6_contact_settings did not meet the requirements. Missing source
    provider contact. Is the module enabled?

My current favorite is something like this:
"Migration d6_contact_settings did not meet the requirements. Missing source provider. Ensure the 'contact' module is installed"

Something like this?

Migration d6_contact_settings did not meet the requirements. Missing source provider. Ensure the module "contact" is installed.

Looks good!

Unfortunately that failure means we can't use that for our tests. Writing an explicit tests shouldn't be a big deal though.

I'd maybe remove the words "Missing source provider". It's a strange way of saying "module not installed". What's also unclear is that the module is missing from the source database rather than the destination. Maybe something like "Enable the contact module in your source database"?

Well, the reason the "source provider" is missing is simply because it's not being used in the source site - I think telling them to enable a module they don't need is counter-productive. I'd argue against making suggestions and just tell what the situation - "The module @provider_name is not enabled in the source database" or something like that.

I'd argue against making suggestions and just tell what the situation - "The module @provider_name is not enabled in the source database" or something like that.

Yeah, that's fine with me -- I'm merely against alien terminology like "source provider" to refer to a Drupal module.

Sure ok, all valid points.

I tried to come up with a better one but after I fiddled with it I realized I'd word for word written what Mike wrote in #11. Only thing I might change is "database" to "site" since people tend to think of modules being enabled on a site not in the database but I'm sort of just coloring the bikeshed there.

I've made the string change, but needs tests...

For the tests, it seems that pluginDefinition['requirements_met'], pluginDefinition['source_provider'], and pluginDefinition['minimum_schema_version'] need to be initialized. But I'm not sure how to do that.

@quietone: In a test, you can pass the plugin definition directly to the plugin's constructor. The first three arguments to any plugin are the configuration, plugin ID, and plugin definition.

$configuration = array(...);
$plugin_definition = array(...);
$plugin = new MyPlugin($configuration, $plugin_id, $plugin_definition, $additional_argument, $another_argument...);

@phenapromixa, worked on using a kernel test as you suggested, but couldn't instantiate the plugin because it's abstract. So, I decided to revisit using a Unit test and fixed my previous errors. But is this the right place or should this test be a kernel test?

Also, there is a unit test Drupal6SqlBaseTest.php but not one for D7. Shouldn't there be one for D7? But then the tests in Drupal6SqlBaseTest could be used for both, they aren't D6 specific. Unless I am missing something.

Locally patch applied.
I tested and there were few fails.

My previous comment was about patch #17

1. index 5a7af4d..05123e8 100644
   --- a/core/modules/migrate_drupal/src/Tests/Table/d6/System.php
   
   @@ -181,7 +181,7 @@ public function load() {
          'name' => 'contact',
          'type' => 'module',
          'owner' => '',
   -      'status' => '1',
   +      'status' => '0',
          'throttle' => '0',
          'bootstrap' => '0',
          'schema_version' => '6001',
   

   I hate to do this...but the file hash is unchanged, so if you want to disable Contact in the D6 dump, you need to do it using Drupal 6.

   That said, it's not necessary to change the dump files in any way if you're writing a unit test, because unit tests do not use the dump files. So this change should be reverted.

2. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
   @@ -0,0 +1,169 @@
   +  /**
   +   * @var \Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase
   +   */
   +  protected $base;
   

   This property is not needed -- see the next comment.

3. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
   @@ -0,0 +1,169 @@
   +  protected function setUp() {
   +    $plugin = 'placeholder_id';
   +    $entity_manager = $this->getmock('Drupal\Core\Entity\EntityManagerInterface');
   +    $this->base = new TestDrupalSqlBase($this->migrationConfiguration, $plugin, array(), $this->getMigration(), $entity_manager);
   +    $this->base->setDatabase($this->getDatabase($this->databaseContents));
   +  }
   

   There's not much point to this setUp() method -- the plugin is fully created in testCheckRequirements() anyway. The whole method can be removed, along with the $base property.

4. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
   @@ -0,0 +1,169 @@
   +    $plugin = 'placeholder_id';
   

   Not sure why this is defined as its own variable, because it's only used once when instantiating the plugin.

5. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
   @@ -0,0 +1,169 @@
   +    $entity_manager = $this->getmock('Drupal\Core\Entity\EntityManagerInterface');
   

   Should be getMock().

6. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
   @@ -0,0 +1,169 @@
   +    $this->base = new TestDrupalSqlBase($this->migrationConfiguration, $plugin, $plugin_definition, $this->getMigration(), $entity_manager);
   

   This should not pass $this->migrationConfiguration. The plugin's $configuration parameter refers to plugin-specific configuration, not migration configuration. (Remember, the migration is already passed in as the fourth argument :) An empty array will work here.

7. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
   @@ -0,0 +1,169 @@
   +    $this->base->checkRequirementsWrapper();
   

   Not sure why there is a wrapper around checkRequirements(), which is a public method to begin with.

8. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
   @@ -0,0 +1,169 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function fields() {
   +    return array(
   +      'filename' => t('The path of the primary file for this item.'),
   +      'name' => t('The name of the item; e.g. node.'),
   +      'type' => t('The type of the item, either module, theme, or theme_engine.'),
   +      'owner' => t("A theme's 'parent'. Can be either a theme or an engine."),
   +      'status' => t('Boolean indicating whether or not this item is enabled.'),
   +      'throttle' => t('Boolean indicating whether this item is disabled when the throttle.module disables throttleable items.'),
   +      'bootstrap' => t('Boolean indicating whether this module is loaded during Drupal\'s early bootstrapping phase (e.g. even before the page cache is consulted).'),
   +      'schema_version' => t('The module\'s database schema version number.'),
   +      'weight' => t('The order in which this module\'s hooks should be invoked.'),
   +      'info' => t('A serialized array containing information from the module\'s .info file.'),
   +    );
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function query() {
   +    $query = $this->database
   +      ->select('system', 's')
   +      ->fields('s', array('filename', 'name', 'schema_version'));
   +    return $query;
   +  }
   +
   +  /**
   +   * Tweaks DrupalSqlBase to set a new database connection for tests.
   +   *
   +   * @param \Drupal\Core\Database\Connection
   +   *   The new connection to use.
   +   *
   +   * @see \Drupal\Tests\migrate\Unit\MigrateSqlTestCase
   +   */
   +  public function setDatabase(Connection $database) {
   +    $this->database = $database;
   +  }
   +
   +  /**
   +   * Tweaks DrupalSqlBase to set a new module handler for tests.
   +   *
   +   * @param \Drupal\Core\Extension\ModuleHandlerInterface
   +   *   The new module handler to use.
   +   *
   +   * @see \Drupal\Tests\migrate\Unit\MigrateSqlTestCase
   +   */
   +  public function setModuleHandler(ModuleHandlerInterface $module_handler) {
   +    $this->moduleHandler = $module_handler;
   +  }
   +
   +  /**
   +   * Wrapper method to test protected method moduleExists().
   +   */
   +  public function moduleExistsWrapper($module) {
   +    return parent::moduleExists($module);
   +  }
   +
   +  /**
   +   * Wrapper method to test protected method getModuleSchemaVersion().
   +   */
   +  public function getModuleSchemaVersionWrapper($module) {
   +    return parent::getModuleSchemaVersion($module);
   +  }
   +
   +  /**
   +   * Wrapper method to test protected method variableGet().
   +   */
   +  public function variableGetWrapper($name, $default) {
   +    return parent::variableGet($name, $default);
   +  }
   +
   +  /**
   +   * Wrapper method to test protected method variableGet().
   +   */
   +  public function checkRequirementsWrapper() {
   +    return parent::checkRequirements();
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getIds() {
   +    return array();
   +  }
   

   None of these are ever called by the test, so I don't think we need them. It's OK for TestDrupalSqlBase to be empty.

I have not yet addressed all the issues in #22.

#5. Can't figure out how to use getMock.
#8. Removing any of those methods in a fatal error. I used Drupal6SqlBaseTest.php as an example.

HP Fatal error: Class Drupal\Tests\migrate_drupal\Unit\source\TestDrupalSqlBase contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\migrate\Plugin\MigrateSourceInterface::fields) in /opt/sites/md8/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php on line 81
PHP Stack trace:

Going for a walk ... in the rain.

#5. Can't figure out how to use getMock.

It looks like you had it exactly right; it just needs to be getMock() and not getmock().

#8. Removing any of those methods in a fatal error. I used Drupal6SqlBaseTest.php as an example.

To extend the abstract class, you only need to implement fields(), which in this case can return an empty array. As far as I can tell there is no need to override the protected methods as well unless you're planning to call them directly from outside the class.

I seem to have forgotten about this issue. But here's a new patch which I think addresses all of phenaproxima's issues in 22.

Oops, that was the wrong patch.

+++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
@@ -0,0 +1,106 @@
+    $plugin->setDatabase($this->getDatabase($this->databaseContents));
...
+class TestDrupalSqlBase extends DrupalSqlBase {

Lets just use a bit of reflection to set the database rather than an entire mock class? Unless the getIds() call is needed to return empty as well, that would probably justify leaving the test class.

NW for 29

I looked at this but that would mean changing the DrupalSqlBase class to set a database?

As discussed with penyaskito and benjy we would have to change the DrupalSqlBase class not to be abstract anymore in other to use reflection. So we decided to would be better to let the extra TestDrupalSqlBase to be there.

I'm going to test the patch now when I'm done I'll set it to be RTBC

I tested this with a Drupal 7 to 8 Migration using the action module as source.
After applying the patch I get the message "The module action is not enabled in the source site. source_provider: action." when trying to run the migration.

Thanks to penyaskito for mentoring me on the drupal dev days Milan.

So it works! RTBC

1. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
   @@ -0,0 +1,106 @@
   +/**
   + * @file
   + * Contains \Drupal\Tests\migrate_drupal\Unit\source\DrupalSqlBaseTest.
   + */
   

   Not needed any more

2. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
   @@ -0,0 +1,106 @@
   +  /**
   +   * @expectedException \Drupal\migrate\Exception\RequirementsException
   +   * @expectedExceptionMessage is not enabled
   +   */
   +  public function testSourceProviderNotActive() {
   

   Should have an @covers and best practices have changed here... https://thephp.cc/news/2016/02/questioning-phpunit-best-practices

3. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
   @@ -0,0 +1,106 @@
   +  }
   +}
   

   Needs a new line in-between

1-3. Fixed
Also, removed the summary line adding an @coversDefaultClass based on this
"Only PHPUnit tests MAY skip the PHPDoc summary line if their PHPDoc specifies a @coversDefaultClass. See PHPUnit @coversDefaultClass annotation documentation" on https://www.drupal.org/node/2116043. Hope that is correct.

@scuba_fly, sorry! I didn't check to see if this was assigned.

@quietone No problem. I'll try to review your new patch if I can later. But busy at te moment so Unassigning it for now.

+++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
@@ -44,10 +38,10 @@
+    $this->setExpectedException('\Drupal\migrate\Exception\RequirementsException','is not enable');

Could we use RequirementsException::class instead of the class name as a string?

Could we include the full exception message, or at least include the text changed in this issue and the missing source_provider module name so we ensure that it's shown to users?

Thanks for working on this!

I note it here just in case I don't succeed with the patch, that there is a typo in it too: 'is not enable' should be 'is not enabled'.

Attached a patch and interdiff according to penyaskito's suggestions.

+++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
@@ -44,10 +38,10 @@
-   * @expectedException \Drupal\migrate\Exception\RequirementsException
-   * @expectedExceptionMessage is not enabled
...
+    $this->setExpectedException('\Drupal\migrate\Exception\RequirementsException','is not enable');

Is there a standards change discussing and documenting this change other then the blog post? I'd like to discuss it since we're pretty well committed to the annotation.

Previous comment isn't really relevant to getting this committed as the test works the same way either way. +1, this is long overdue.

Tests are passing. Back to RTBC.

No, tests are passing. Back to RTBC.

Committed and pushed 7d6e615 to 8.3.x and a3c47f3 to 8.2.x. Thanks!

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
@@ -102,7 +102,7 @@ public function checkRequirements() {
+          throw new RequirementsException('The module ' . $this->pluginDefinition['source_provider'] . ' is not enabled in the source site.', ['source_provider' => $this->pluginDefinition['source_provider']]);

The array thing ['source_provider' => $this->pluginDefinition['source_provider'] is just not needed. let's remove that dead code here. Fixed on commit.

I had assumed that was left to provide a way to explicitly check the missing requirement(s) when catching the exception which is why it was left.

This is fine. I opened a follow up issue so we can discuss the addition of a test for the case. #2790111: DrupalSqlBase doesn't include missing source in requirements exception