Migrate does a lot of if (method_exists($this|$migration, 'someMethod')) { but this has serious drawbacks:

  • Code is not self-documented (no docblock for those ghost/dynamic methods).
  • Method signature is not documented (method is not explicitely declared anywhere): it leaves the developer clueless about the arguments they should take, when they are called, and what he must do into those.
  • Because method signature doesn't exists anywhere, there is no code consistency check at compilation time (for example, when signature diverge in which case the code is definitely broken) and errors remain silent.
  • Runtime checks are bad when then can be done at compile time (it's always faster at compile time).
  • Modern IDE's (I use Eclipse) cannot find those methods, therefore cannot neither outline them in code navigation pane, neither autocomplete on them.
  • Code navigation is broken.
  • method_exists() sounds like introspection to me, introspection is obscure and can cost a lot (I don't know in this particular case thought); Still is obscure.

There are three different ways to fix this from the hardest to the easiest:

  • Rewrite the full module design around interfaces and use "instanceof": this method is the most invasive so I won't ask you to do it.
  • Make those methods abstract in every class they are being called, forcing the implementor to implement them: this method is awful because it forces developers to implement void methods in most cases.
  • Implement empty stubs everywhere they could live, returning a null value for those supposed to return something: easiest way because you don't have to fix the already implemented code (and no API break) but also the ugliest when those must return something (for example the Migration::createStub method) because you have then a polymorphic return.

Comments

mikeryan’s picture

Status: Active » Postponed

Definitely, we want to refactor and make use of interfaces where possible. There's nothing we can do for 2.x - the API would break for existing migration code. The plan is for at least the most basic parts of the Migrate API to go into core for Drupal 8 (#1052692: New import API for major version upgrades (was migrate module)) - that's where the opportunity is to refactor the API.

mikeryan’s picture

Issue tags: +Migrate 3
pounard’s picture

The third solution is doable for 2.x without any API break, I may end up with a patch someday.

pifagor’s picture

Status: Postponed » Closed (outdated)