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.
Comment | File | Size | Author |
---|---|---|---|
#23 | 3010951-23.patch | 5.73 KB | sarvjeetsingh |
#20 | 3010951-20.patch | 5.73 KB | ayushmishra206 |
#18 | 3010951-18.patch | 5.72 KB | ayushmishra206 |
#16 | 3010951-16.patch | 1.45 KB | ayushmishra206 |
#16 | interdiff_11-16.txt | 1.45 KB | ayushmishra206 |
Comments
Comment #2
camerongreen CreditAttribution: camerongreen commentedComment #3
super_romeo CreditAttribution: super_romeo commentedPatch attached.
Comment #4
super_romeo CreditAttribution: super_romeo commentedComment #5
mikelutzI 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.
Comment #8
quietone CreditAttribution: quietone as a volunteer commentedAnd finally a test for this. The fix from #3 hasn't change, this just adds a kernel test.
Comment #10
quietone CreditAttribution: quietone as a volunteer commentedThat is a bad patch, ignore it. I will make a new one in a bit
Comment #11
quietone CreditAttribution: quietone as a volunteer commentedStarting over. The fix from #3 hasn't change, this just adds a kernel test.
Comment #13
quietone CreditAttribution: quietone as a volunteer commentedAdd tag
Comment #14
mikelutzThank 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: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.
Comment #15
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedComment #16
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedI have made the changes requested by @mikelutz. Please review.
Comment #17
mikelutzThe latest patch seems to have removed the test.
Comment #18
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedSorry for the previous mistake. Please review.
Comment #19
mikelutzGetting 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"
Comment #20
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedThank you for your patience and all the help. I'll keep this in mind going forward.
Comment #21
mikelutzSoo Close! one nit, there's an extra space at the end of this line that needs to be removed. Sorry!
Comment #22
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedComment #23
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedRemoved that extra space and updated the patch.
Comment #24
mikelutzThanks!
Comment #25
larowlanWhilst 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.
Comment #26
mikelutzSetting to NW for the CR.
Comment #27
quietone CreditAttribution: quietone as a volunteer commentedChange record added.
Comment #28
mikelutzCR added, back to RTBC
Comment #29
alexpottCommitted 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.
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.