I was trying to get a list of migrations with a certain tag, but if the tag didn't match anything it seems all migrations are returned, whereas I would expect no migrations to be returned.

So my code is doing:

    $manager = \Drupal::service('plugin.manager.migration');

    $plugins = $manager->createInstancesByTag($tag);

Which should filter the migrations, and does with migrations being found for an "valid" tag:

But for an invalid tag it returns all migrations which seems in error.

  /**
   * {@inheritdoc}
   */
  public function createInstancesByTag($tag) {
    $migrations = array_filter($this->getDefinitions(), function ($migration) use ($tag) {
      return !empty($migration['migration_tags']) && in_array($tag, $migration['migration_tags']);
    });
    return $this->createInstances(array_keys($migrations));
  }

So when $migrations is empty, which to my mind should return an empty array, the following code kicks in which seems to return all migrations for the plugin:


  /**
   * {@inheritdoc}
   */
  public function createInstances($migration_id, array $configuration = []) {
    if (empty($migration_id)) {
      $migration_id = array_keys($this->getDefinitions());
    }

So if I haven't missed something, the fix should be to return an empty array if the filter in createInstancesByTag finds nothing.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

camerongreen created an issue. See original summary.

camerongreen’s picture

Issue summary: View changes
super_romeo’s picture

super_romeo’s picture

Version: 8.6.2 » 8.8.x-dev
Status: Active » Needs review
mikelutz’s picture

Priority: Minor » Normal
Status: Needs review » Needs work
Issue tags: +Needs tests

I agree that this is buggy behavior, and I think the patch is the correct fix.

Sending to NW for tests though. Any bug fix require a test-only patch clearly showing the bug, and a patch with the test+fix showing that the patch fixes the bug.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
7.12 KB
7.21 KB
7.93 KB

And finally a test for this. The fix from #3 hasn't change, this just adds a kernel test.

Status: Needs review » Needs work

The last submitted patch, 8: 3010951-8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

That is a bad patch, ignore it. I will make a new one in a bit

quietone’s picture

Status: Needs work » Needs review
FileSize
4.32 KB
4.27 KB
4.98 KB

Starting over. The fix from #3 hasn't change, this just adds a kernel test.

The last submitted patch, 11: 3010951-11-fail.patch, failed testing. View results

quietone’s picture

Issue tags: +Bug Smash Initiative

Add tag

mikelutz’s picture

Status: Needs review » Needs work
Issue tags: +Novice

Thank you for the test, @quietone. I've considered again the BC ramifications of this, and I still concur with my younger self that this is a bug fix, and we should not bend over backwards to preserve current behavior. One more thing that I would ask, in Drupal\migrate\Plugin\MigratePlugingManagerInterface we have:

/**
   * Create migrations given a tag.
   *
   * @param string $tag
   *   A migration tag we want to filter by.
   *
   * @return array|\Drupal\migrate\Plugin\MigrationInterface[]
   *   An array of migration objects with the given tag
   */
  public function createInstancesByTag($tag);

Can we change the return description to "An array of migration objects with the given tag, or an empty array if no migrations with that tag exist." To make it more clear? Tagging novice for this final piece.

Otherwise, I think this is ready for RTBC.

ayushmishra206’s picture

Assigned: Unassigned » ayushmishra206
ayushmishra206’s picture

Assigned: ayushmishra206 » Unassigned
Status: Needs work » Needs review
FileSize
1.45 KB
1.45 KB

I have made the changes requested by @mikelutz. Please review.

mikelutz’s picture

Status: Needs review » Needs work

The latest patch seems to have removed the test.

ayushmishra206’s picture

Status: Needs work » Needs review
FileSize
5.72 KB

Sorry for the previous mistake. Please review.

mikelutz’s picture

Status: Needs review » Needs work

Getting closer! We cannot have a comment exceed 80 columns though, due to coding standards. It needs to wrap onto the next line after "empty array if no"

ayushmishra206’s picture

Status: Needs work » Needs review
FileSize
5.73 KB

Thank you for your patience and all the help. I'll keep this in mind going forward.

mikelutz’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate/src/Plugin/MigrationPluginManagerInterface.php
@@ -47,7 +47,8 @@ public function createStubMigration(array $definition);
+   *   An array of migration objects with the given tag, or an empty array if no ¶

Soo Close! one nit, there's an extra space at the end of this line that needs to be removed. Sorry!

sarvjeetsingh’s picture

Assigned: Unassigned » sarvjeetsingh
sarvjeetsingh’s picture

Assigned: sarvjeetsingh » Unassigned
Status: Needs work » Needs review
FileSize
5.73 KB

Removed that extra space and updated the patch.

mikelutz’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record

Whilst I agree that the current behaviour is wrong, I think we should provide a change notice here so that if someone finds their things are broken, they might be able to pinpoint why.

mikelutz’s picture

Status: Needs review » Needs work

Setting to NW for the CR.

quietone’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record

Change record added.

mikelutz’s picture

Status: Needs review » Reviewed & tested by the community

CR added, back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 7d1fdd7 and pushed to 9.1.x. Thanks!

Only committed to 9.1.x as we've got a CR and the very small chance of disruption.

diff --git a/core/modules/migrate/src/Plugin/MigrationPluginManager.php b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
index 563580969d..bb4f6c0be8 100644
--- a/core/modules/migrate/src/Plugin/MigrationPluginManager.php
+++ b/core/modules/migrate/src/Plugin/MigrationPluginManager.php
@@ -126,9 +126,7 @@ public function createInstancesByTag($tag) {
     $migrations = array_filter($this->getDefinitions(), function ($migration) use ($tag) {
       return !empty($migration['migration_tags']) && in_array($tag, $migration['migration_tags']);
     });
-    return $migrations
-      ? $this->createInstances(array_keys($migrations))
-      : [];
+    return $migrations ? $this->createInstances(array_keys($migrations)) : [];
   }
 
   /**

This fits comfortably on one line in under 80 characters - I think 1 line should be preferred for ternaries as brevity is kinda the point.

  • alexpott committed 7d1fdd7 on 9.1.x
    Issue #3010951 by quietone, ayushmishra206, sarvjeetsingh, super_romeo,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.