Migration dependency for comment status is not evaluated

Wow. That's a good observation and a near-perfect way to fix things. Unfortunately, I don't have a very deep understanding of the migration framework and would like to see some more reviews for the patch which you have provided. Thanks for spending time on this.

@srishti.bankar Thanks for providing schema though. I have taken it from your patch and pushed it with the commit for this issue https://www.drupal.org/project/disqus/issues/2289295.

You will have to slightly modify your patch to ensure it works with the latest development release. Thanks.

Excellent work!

The few nits we found:

1. index bcb0bf8..b46ea2a 100644
   --- a/config/schema/disqus.schema.yml
   
   --- a/config/schema/disqus.schema.yml
   +++ b/config/schema/disqus.schema.yml
   
   @@ -56,3 +56,14 @@ disqus.settings:
   +
   +field.value.disqus_comment:
   +  type: mapping
   +  label: 'Disqus field values'
   +  mapping:
   +    status:
   +      type: boolean
   +      label: 'Disqus comments should be shown'
   +    identifier:
   +      type: string
   +      label: 'Identifier of the Disqus discussion'
   

   This should be removed per #7

2. +++ b/disqus.module
   @@ -271,3 +274,65 @@ function disqus_api() {
   +/**
   + * Implements hook_migration_plugins_alter().
   + *
   + */
   

   Please remove this empty func doc line.

3. +++ b/disqus.module
   @@ -271,3 +274,65 @@ function disqus_migrate_prepare_row() {
   +  $disqus_plugin = \Drupal\migrate\Plugin\MigrationDeriverTrait::getSourcePlugin('disqus_enabled_content_types');
   +  assert($disqus_plugin instanceof \Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase);
   

   Please replace these FQCNs with use statements!

4. +++ b/disqus.module
   @@ -271,3 +274,65 @@ function disqus_migrate_prepare_row() {
   +  $disqus_node_types = array_reduce(
   +    iterator_to_array($disqus_plugin, FALSE),
   +    function (array $carry, Row $row) {
   +      $carry[] = $row->getSourceProperty('type');
   +      return $carry;
   +    },
   +    []
   +  );
   

   While I was testing this patch, I noticed that we cannot assume that the source plugin is only available is the appropriate module (in this case: disqus was enabled in the source Drupal instance.

   We have to check whether the source plugin's requirements are met, and if they aren't, then we shouldn't do anything:

   try {
     $disqus_plugin->checkRequirements();
   }
   catch (\Drupal\migrate\Exception\RequirementsException $e) {
     // Disqus module isn't enabled in the source instance, so we don't have to
     // do anything.
     return;
   }
   

5. +++ b/disqus.module
   @@ -271,3 +274,65 @@ function disqus_api() {
   +  $query->fields('d', ['nid', 'status']);
   

   You don't use these column values at all. What you're using is the evaluated SQL expression you add to this query.

   You can simply remove this SelectInterface::fields() call.

6. +++ b/disqus.module
   @@ -271,3 +274,65 @@ function disqus_api() {
   +  $node = $query->execute()
   +    ->fetchAssoc();
   

   and if you did the above, then you can simply call $disqus_status = $query->execute()->fetchField();. I will be either 1 or '0'.

7. +++ b/migrations/d7_disqus_settings.yml
   @@ -18,6 +18,7 @@ source:
   +  source_module: disqus
   

   I just noticed that this configuration doesn't do what I thought:

   https://drupal.slack.com/archives/C226VLXBP/p1643200586343300

   But then how can we prevent these migrations from being executed if the disqus module isn't enabled on the source, but some why we have it installed on the destination site?..

   I see only one solution:

   In your migration_plugins_alter hook, you will check whether the requirements of the disqus_enabled_content_types source plugin are met. If it fails, then before your return statement, you have to manually remove the d7_disqus_settings and the d7_disqus_field_storage migrations:

   try {
     $disqus_plugin->checkRequirements();
   }
   catch (\Drupal\migrate\Exception\RequirementsException $e) {
     // Disqus module isn't enabled in the source instance, so we don't have to
     // do anything but remove the d7_disqus_settings and d7_disqus_field_storage
     // migrations.
     unset($migrations['d7_disqus_settings']);
     unset($migrations['d7_disqus_field_storage']);
     return;
   }
   

@srishti.bankar, @gaurav.kapoor, I want to attach the test infrastructure I'm using.
99% of this patch is a database fixture of a Drupal 7 instance, the actual test is about 8 kilobytes.

Unfortunately, the patch I added in #9 contains the patch file itself as well.

Only three nits:

1. +++ b/disqus.module
   @@ -271,3 +277,74 @@ function disqus_migration_plugins_alter(array &$migrations) {
   +  $disqus_plugin = MigrationDeriverTrait::getSourcePlugin('disqus_enabled_content_types');
   

   I just learned that calling static trait methods is deprecated in PHP 8.1.x, so we shouldn't do this anymore.

   But since DisqusDeriver you're adding uses MigrationDeriverTrait, you can use that class to get the same source plugin instance:

        }
      );
    
   -  $disqus_plugin = MigrationDeriverTrait::getSourcePlugin('disqus_enabled_content_types');
   +  $disqus_plugin = DisqusDeriver::getSourcePlugin('disqus_enabled_content_types');
   +  assert($disqus_plugin instanceof DrupalSqlBase);
      try {
        $disqus_plugin->checkRequirements();
      }
   

2. You shouldn't remove the source_plugin key from the Disqus settings migration, it still contains valuable information for determining the appropriate migration state.
3. Could you please add a migration state yaml file as well?

👌All my previous concerns are addressed in #15.

I'm uploading a simplified test-only patch as well. (Simplified stands for that it does not require Migrate Magician anymore.)

@srishti.bankar @huzooka Thanks for working on this. Regarding this,

$row->setSourceProperty('disqus_identifier', 'node/' . $row->getSourceProperty('tnid'));

This is assuming that the identifier will have a default value of type 'node:{node_id}', but the user can set the identifier value to any string. I am not sure if the D7 version has a feature of allowing users to change the identifier. In case there is an option, this will just set the wrong value for the identifier.

Rest everything looks good to me.

Re #17:

Now it does the same what the previous (standalone) status migration's source plugin did, so we won't add any regressions.

I ask you to allow us address this in a follow-up issue (but we will need some pointers).

Thanks, @huzooka. I quickly scanned the D7 codebase and it turns out there is no option to set a value for identifier and it is always 'node:nid'. Pushing this to 2.0.x.

Cheers!!

I've got a fatal error on drush ms "The "disqus_enabled_content_types" plugin does not exist."
I am attaching a temporary solution.