Fixes #440: Update drush_get_class() to work with composer autoloader

[greg-1-anderson]

This makes me smile. :)

[weitzman]

Not used?

[weitzman]

Nice work. Lets see if @msonnabaum and others have feedback.

[greg-1-anderson]

As an alternative, I also considered eliminating drush_get_class() altogether with a pattern similar to this:

$class = 'Drush\DrushRole' . drush_drupal_major_version();
$role = new $class($rid);

Then, we would have to catch the class not found exception instead of testing to see if drush_get_class() returned NULL. However, this seemed like a fairly awkward construct. If we could just do something like this:

$role = new Drush\DrushRole($rid);

... and have Drush supply the correct class for the current Drupal version, that would be an improvement. Unfortunately, there is no way to insert a variable expression into the class name without using the awkward two-line syntax shown above. Making our own autoloader that might return Drush\DrushRole7 when Drush\DrushRole was requested also seemed undesirable on two points: (1) it would require a non-standard autoloader with some very "clever" / non-obvious class selection code in it, and (2) when reading the code, there would be no good indication that you might be getting something different than the class that was requested, which seemed far too magic.

So, this is where that stub function get_specific_class_version() comes in; I thought I might try something like this:

$role = new get_specific_class_version('Drush\DrushRole')($rid);

However, that isn't actually valid php syntax -- we're still stuck with the two-line version here, so I decided it wasn't an improvement over drush_get_class(). We could go with the two-line version if you prefer it, but I tend to think that we can leave this about as-is, and add additional improvements at the Drush command level, per #88.

[greg-1-anderson]

Okay, I moved 'classes' to 'lib', and removed the unused empty function.

[greg-1-anderson]

Another thing to comment on here is that it is pretty easy in concept to add additional packages that will autoload from somewhere else. In Drush's composer.json, we have:

  "autoload": {
      "psr-0": {
          "Drush":        "lib/"
      }
  }

If other Drush extensions want to add more classes, it would just need to define another package name, and declare the directory that they are rooted in. The problem with this is that we need to be able to allow the selection of the available packages to be generated at runtime, based on the available extensions. The composer.json file, though, is static, and requires a composer update any time it changes. To make this dynamic, maybe we need Embedded Composer? What does Drupal 8 do? Modules namespace their classes under Drupal\modulename, and their class files are located in the module's lib directory. How does d8 find them? We need to do something similar here.

[greg-1-anderson]

There is a module, xautoload, that provides D8-style autoloading in Drupal 7. It seems probable that modules using the D8 or xautoload autoloader should be able to put classes in the namespace Drush\modulename, and use the from Drush with no code changes to the patch above. I will do an experiment to confirm, but it seems that Embedded Composer should not be necessary.

Some additional code in Drush will probably be necessary to allow Drush extensions that are not modules to autoload classes. I think that the xautoload module sources should serve as a good guide.

[weitzman]

In D8, classes provided by Contrib do get autoloaded. The latest idea is that D8 will switch from PSR-0 to PSR-4. You might want to just read the Summary as the issue has >300 comments.

FYI, Drupal 8 has not decided at all how Composer is supposed download their dependencies.

[weitzman]

I reviewed the diff here with @msonnabaum and we're both happy with this PR. @greg-1-anderson is welcome to commit this when he is satisfied.

[weitzman]

@msonnabaum and I did discus renaming class names DrushRole* to just Role*. No need to prefix everything with Drush IMO.

[greg-1-anderson]

Sounds good. I was also considering renaming the base class to something like RoleBase instead of just Role. The reason for this is that if we start using drush_get_class() to identify classes that are used for command implementations, if there are no Drupal version-specific classes at all, it would be good to fall back to just the classname 'Foo' (if there is no Foo7, Foo8, etc.). Naming the base class 'Foo' would get in the way of this.

Do you agree? Do you think 'FooBase' is the best name for the base class, or do you prefer something else (AbstractFoo, other)?

[weitzman]

Makes sense. I'd go with RoleBase.