A small problem I noticed. For a better OOP design, method/constructor signatures should stay the same, or extend. This would require any extension of MigrationBase, Migration or DynamicMigration to include at least:

public function __construct($group = null)
{
    ...
}

(Additionally, I think PHP 5.4 will give strict errors for omitting $group.)

But this causes problems in client code, because a MigrateGroup object is expected, but an array is actually being passed in. Later, calling $foo->getGroup()->getName() fails.

Obviously an easy workaround for now is to not pass the $group variable to the parent constructor. Possible to use type-hinting and also fix where the array is being passed in?

Comments

mikeryan’s picture

Status: Active » Postponed

Yes, this is a known issue, but there's nothing we can do about it in Migrate 2.x without breaking existing migration code. Migrate 3.x will certainly make sure to keep the constructor signatures consistent.

mikeryan’s picture

Version: 7.x-2.5 » 7.x-2.x-dev
Status: Postponed » Active
Issue tags: +Migrate 2.6

Rethinking this - I'm still trying to clean up some ugly chicken-and-egg issues that are a legacy of the now-abandoned auto-registration, and the $group argument in the base classes is in the way - $arguments should be the standard parameter here. I think we can deal with it in an upwardly-compatible way simply by detecting the type of that parameter - if it's an object of type MigrateGroup, handle it the old way (and maybe even issue a deprecated warning suggested that the calling code be updated), otherwise treat it as an argument array.

mikeryan’s picture

As I dig in here, I think I see some possible scenarios that might trigger #1977174: "Default" job created when no migration belongs to default - the work here will, I hope, prevent that.

mikeryan’s picture

Title: Constructor signature mismatch » Straighten out constructor parameters/arguments
mikeryan’s picture

Status: Active » Fixed

OK, I've committed this. Going forward, migration classes should accept a $arguments array in their constructor and pass it to the parent constructor. The base class will for at least Migrate 2.6 continue to accept a MigrateGroup object, or nothing, as the first constructor parameter but will warn that this is deprecated (the warnings can be disabled in configuration). While I was at it, I similarly deprecated the DynamicMigration class, which no longer serves any useful purpose (all migration classes are now "dynamic" in the sense that DynamicMigration was).

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