Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
The migrate stubbing service (\Drupal\migrate\MigrateStub) implements \Drupal\migrate\MigrateStubInterface, but overrides the method signature and description.
As the result, the following line in the MigrationLookup.php file is highlighted because the method call doesn't match the method signature (3 parameters vs 4 parameters).
Proposed resolution
1) Add missing parameter to the interface method signature;
2) Move the method description from the service to the interface and use inheritance.
Issue fork drupal-3184552
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
MatroskeenComment #4
MatroskeenComment #5
MatroskeenComment #6
benjifisherAll of this code was added in #3004927: Create Migration Lookup and Stub services. If you search that page for "createStub", then you will see that this additional parameter is discussed in #3004927-36: Create Migration Lookup and Stub services(4) and #3004927-37: Create Migration Lookup and Stub services. The difference between the interface and the implementation is intentional.
In general, it is not a problem if an implementation adds an optional parameter to the function signature.
What is a problem would be changing the function signature in the interface. There may be non-core modules that implement
MigrateStubInterface, and they will break if we do that.Perhaps we should change the type hint in
MigrationLookupfromMigrateStubInterfacetoMigrateStub. If we do that, then we should add a code comment explaining why.Comment #7
mikelutzYeah, this is intentional, and we can't add the parameter to the interface without breaking BC. The correct path forward is to deprecate and replace the migration_lookup plugin with a new version that doesn't need the 4th parameter (migration_lookup does because of backwards compatibility reasons). Once that's done the fourth parameter can be deprecated and removed.
Comment #8
MatroskeenHow about this? We keep MigrateStubInterface in the constructor (because it might be swapped) but changing a type hint in the property doc block. In this case, IDE doesn't complain anymore and it makes me happy.
Comment #10
mikelutzI don't completely object to this, but at minimum for now it needs at least a comment explaining why we aren't typehinting on the interface. Changing the phpdoc won't break anything, but it's still wrong. The constructor takes an interface, the create method pulls the service, which is only guaranteed to be the interface. Essentially we are assuming that no one has replaced the stub service in custom code.
In retrospec, Maybe I should have had type checking to make sure that the MigrateStub service is actually the Core MigrateStub class, and errored out if we got something different, but this whole plugin really defeats the purpose of having that as an injectable service. Ultimately, the migrate_lookup plugin needs to be deprecated and replaced with a cleaner version.
I created #3246666: Deprecate migrate_lookup and replace with cleaner process plugin to do that, but with an extra comment in there, I don't object to changing the docblock for the class property.