New parameters introduced in MigrateExecutable class constructor

Created on 17 July 2025, about 2 months ago

Problem/Motivation

Fixes applied for https://www.drupal.org/node/3411051 β†’ added three new readonly parameters to the MigrateExecutable constructor function. This meant that any custom code instantiating a new MigrateExecutable that includes an options array will fail with the error TypeError: Drupal\migrate_tools\MigrateExecutable::__construct(): Argument #3 ($keyValue) must be of type Drupal\Core\KeyValueStore\KeyValueFactoryInterface, array given.

Steps to reproduce

Define a MigrationExecutable with an options array, for example:

$executable = new MigrateExecutable($migration, new MigrateMessage(), ['limit' => 10]);

Run the migration.

Proposed resolution

I am not sure why new parameters were added to a class constructor function based on PHPCS fix suggestions. Where did these come from? Why are they necessary?

πŸ› Bug report
Status

Active

Version

6.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States mgaskey

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @mgaskey
  • πŸ‡ΊπŸ‡ΈUnited States kgthompson

    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);
    
  • First commit to issue fork.
  • Merge request !93Resolve #3536657 "New parameters introduced" β†’ (Merged) created by heddn
  • heddn Nicaragua

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

  • πŸ‡©πŸ‡ͺGermany mrshowerman Munich

    Left a few comments on the MR.

  • heddn Nicaragua

    Posted a response to the last comments.

  • πŸ‡ΊπŸ‡ΈUnited States m.stenta

    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 πŸ“Œ Fix PHPCS issues Active 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?

  • πŸ‡ΊπŸ‡ΈUnited States m.stenta

    Oops sorry didn't mean to change status.

  • heddn Nicaragua

    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.

  • πŸ‡ΊπŸ‡ΈUnited States m.stenta

    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...

  • πŸ‡³πŸ‡ΏNew Zealand xurizaemon Ōtepoti, Aotearoa 🏝

    I believe this change also caused breakage on sites using Migrate Source UI, as the 6.0.x function signature is used there. Opened πŸ› Migrate Tools v6.1.x compatibility Active .

    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?

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

    Thanks for working on this!

  • πŸ‡ΊπŸ‡ΈUnited States m.stenta

    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.

  • πŸ‡³πŸ‡ΏNew Zealand xurizaemon Ōtepoti, Aotearoa 🏝

    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.)

  • Pipeline finished with Skipped
    about 2 months ago
    #556090
    • heddn β†’ committed 8c7029dd on 6.0.x
      Issue #3536657 by heddn, mrshowerman: New parameters introduced in...
  • heddn Nicaragua

    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.

  • πŸ‡ΊπŸ‡ΈUnited States m.stenta

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

  • πŸ‡ΊπŸ‡ΈUnited States joegraduate Arizona, USA

    Doesn't the change record also need to be changed from draft to published?

  • πŸ‡ΊπŸ‡ΈUnited States joegraduate Arizona, USA

    πŸ“Œ Fix PHPCS issues Active 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?

  • heddn Nicaragua

    I've published and updated the change record.

  • πŸ‡¨πŸ‡¦Canada nicoleannevella

    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.

  • πŸ‡ΊπŸ‡ΈUnited States willp24

    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.

  • πŸ‡ΊπŸ‡ΈUnited States willp24
  • heddn Nicaragua

    6.1.2 should be a functioning version.

  • πŸ‡³πŸ‡ΏNew Zealand xurizaemon Ōtepoti, Aotearoa 🏝

    @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 β†’

    This will require an issue in the Migrate Orphans project.

  • πŸ‡ΊπŸ‡ΈUnited States mediabounds

    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.

  • heddn Nicaragua

    Good find. Can you open a follow-up?

  • πŸ‡ΊπŸ‡ΈUnited States willp24

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

Production build 0.71.5 2024