New parameters introduced in MigrateExecutable class constructor

I've updated my usage of this to the following for the time being:

    $key_value_factory = \Drupal::service('keyvalue');
    $time = \Drupal::service('datetime.time');
    $translation_manager = \Drupal::service('string_translation');
    return new MigrateExecutable($migration, $this->migrationMessagesCapture, $key_value_factory, $time, $translation_manager, $migration_options);

Can you tell if this makes things easier to upgrade to the 6.1.x branch?

Left a few comments on the MR.

Posted a response to the last comments.

Ouch. Ran into this today on a client's site after a simple composer update broke all their CSV importers. :-/

Would it make sense to simply revert the specific change that caused this from #3411051: Fix PHPCS issues and put out a quick 6.1.2 release to avoid breaking other sites? Then we can take our time to figure out the *right* way to do it...

I haven't looked closely, but it sounds like it was just a PHPCS fix, so undoing it won't break any new functionality, right?

Oops sorry didn't mean to change status.

It wouldn't be trivial to back out the full phpcs changes as they were done back in January and the impact wasn't caught until recently. The easier way forward is to see these fixes land quickly.

done back in January and the impact wasn't caught until recently

They were committed to the dev branch in January but were included in a stable release ~2 weeks ago, which is why it's just coming up now. Running composer update wouldn't pull the changes in until the stable release was made.

revert the specific change that caused this

Just to clarify, I wasn't suggesting reverting "the full phpcs changes". Just the one that caused this.

But if the proper fix is almost done then full speed ahead! Didn't mean to distract...

I believe this change also caused breakage on sites using Migrate Source UI, as the 6.0.x function signature is used there. Opened #3537749: Migrate Tools v6.1.x compatibility.

Quick workaround can be to downgrade to 6.0.x for sites using Migrate Source UI? That worked for 6.0.5 for me.

I see this MR (and issue) currently targets 6.0.x, but introduces a deprecation notice and fixes handling for 6.1.x - should the MR be targeting 6.1.x or 6.0.x? (Looking closer, I see there's no 6.1.x branch at this time ... but there are tags and releases for 6.1.1 and 6.1.0.)

Suggest that we update "Proposed resolution" so that the intent of this change is clearer?

Thanks for working on this!

I believe this change also caused breakage on sites using Migrate Source UI

Correct. We use Migrate Source UI in farmOS for our CSV importers, and that's what caused the issue for us. But it broke because Migrate Tools introduced a breaking change in a minor release instead of a major release with proper deprecation procedure, as dictated by semantic versioning. Accidents happen. 🤷

(Makes me wonder if there are any PHPStan/PHPCS plugins that can be used to check for changes that would break semantic versioning, to catch things like this before they are merged... 🤔)

Quick workaround can be to downgrade to 6.0.x for sites using Migrate Source UI? That worked for 6.0.5 for me.

This worked as a quick fix for me as well. I'm hoping we can put out a 6.1.2 release soon so I don't need to tag a new release of farmOS that pins Migrate Tools to 6.0.5, though.

Following up on my own question re branch for 6.1.x - FYI @heddn, the 6.1.0 and 6.1.1 releases are from the 6.0.x branch.

(Not meaning to nitpick, just alerting to something which was confusing me when I tried git operations relating to that branch.)

Let's merge this and tag a release. If there are any more issues, we can resolve them in a follow-up and yet another release. I'd rather not leave this hanging forever.

Thanks @heddn and @mrshowerman for the quick fix on this!!

Does the change record also need to be changed from draft to published?

#3411051: Fix PHPCS issues also introduced an additional API change to the constructor method signature for Drupal\migrate_tools\MigrateBatchExecutable beyond the changes made to its parent class here. MigrateBatchExecutable::construct() now expects a MigrationPluginManagerInterface as its sixth parameter and pushes the $options parameter to the end.

Does the change record need to mention this additional change to Drupal\migrate_tools\MigrateBatchExecutable as well?

I've published and updated the change record.

I have just updated to 6.1.2 and am seeing this error:

Drupal\migrate_tools\MigrateBatchExecutable::__construct(): Argument #3 ($keyValue) must be of type Drupal\Core\KeyValueStore\KeyValueFactoryInterface, array given

the custom code calling this is:

$executable = new MigrateBatchExecutable($migration, new MigrateMessage(), $options);

Is there something I need to change in the custom code to stop that error? or is there a problem with the migrate_tools module? $options is an array.

The CR has a fix: https://www.drupal.org/node/3537201

Hi! We are currently running Drupal Core 10.4.8 and Migrate Tools version 6.0.4. Each time I attempt to upgrade past version 6.0.4 and I run a known valid migration, I encounter the following error message immediately after the upgrade.

ArgumentCountError: Too few arguments to function Drupal\migrate_tools\Drush\Commands\MigrateToolsCommands::__construct(), 4 passed in /home/ide/project/docroot/modules/contrib/migrate_orphans/src/Commands/MigrateOrphansCommands.php on line 55 and exactly 7 expected in Drupal\migrate_tools\Drush\Commands\MigrateToolsCommands->__construct()

My apologies if this is not the correct place to post the question however, it's in reference to the construtor function that this patch relates to.

6.1.2 should be a functioning version.

@willp24 it looks like the Migrate Orphans code on that site requires a change to account for the change in this module.

The change required is documented at https://www.drupal.org/node/3537201

Here's an issue #3541615: Migrate Tools v6.1.x compatibility and patch in the Migrate Orphans project.

The change in 6.1.2 to be backwards compatible is inadvertently discarding any $options provided as the third argument to the constructor.

If the MigrateExecutable constructor is invoked with the old method signature (three arguments with the third as an array), the value passed to $keyValue will not be copied over to $options and will be lost.

When an array is provided as the third argument, the code is checking whether $options is already set:

if (!isset($options)) {
  $options = $keyValue;
}

But this is problematic because $options defaults to an empty array so it will always be set. $keyValue is updated to be the KeyValueFactoryInterface and the options array passed to the constructor is lost.

Good find. Can you open a follow-up?

Yep! #3542697: Backwards compatibility in MigrateExecutable constructor is inadvertently discarding $options

@xurizaemon thank you! I'll work on testing these parameters out or using the patch. Much appreciated!