AutoloadGenerator refactoring - strategy talk

[donquixote]

I still would like to refactor the AutoloadGenerator. It is so messy and huge. But I want to wait until PSR-4 lands, #2459

Previous attempts:

• AutoloadGenerator: Refactoring, part 1. #2142
• More refactoring for AutoloadGenerator #2298

Now, before I give this another try, I would like to discuss some strategy.

I made some strategical decisions in the previous attempts, and would like to know which of these you agree with and which you don't.

1. Introduce separate classes for different parts of the generated autoload stuff.

One for PSR-0, one for PSR-4, one for classmap, one for include paths, etc. In the second PR I called this "aspects", but discussion showed this was a stupid and misleading choice of word. Maybe "Plugin" is better?

Each of these classes can do two things: Dump a file, and return a php snippet for the ComposerAutoloaderInit::getLoader() method. This can be defined in an interface they all implement.

In the second PR this used to be in the same method, but we can easily split this up to be two separate methods.

One question is which data to provide to those "plugins". The main algorithm should just iterate over all registered plugins and give each the chance to dump a file and add a php snippet. Here they should all receive the same parameters. However, they could have different constructor parameters.

2. Turn some variables into protected object properties

Some of the variables in AutoloadGenerator are passed around all over the place. E.g. $basePath, $vendorPath, $targetDir, $appDirBaseCode, stuff like that.

I generally believe in reducing the visibility of state and variables as much as possible. An object or method that does not need a given variable, should not get its hand on it. However in this case I don't think that all the passing around really helps anyone.

It would be easier if at least some of the variables were somehow bundled in an object. This can either be the main object where all the work happens, so you would do $this->vendorPath.

Or it could be a service object that is passed around, to provide everyone with the paths. This could also take care of the calculation of relative paths from one base to another.

3. Have an object that represents the process

Atm the AutoloadGenerator itself does not remember anything about the generation process. All the information about the generation process is passed in to the dump() method, and will be forgotten afterwards.

I am suggesting a different approach: An object that receives all the information on construction time, and that only lives as long as the generation process.

This object could either contain all the generation logic, or it could just be a helper to be passed around.

We should not change the interface of AutoloadGenerator, so this new object would be something else, instantiated from AutoloadGenerator->dump().

4. LazyEvaluationContainer similar to a DIC

This was an idea I used in the second PR, that I also like to use "at home" in other projects.

It is meant for a situation where you want to calculate a number of values with a dependency graph. You want to make sure that each value is only calculated once, and you want to make sure that values are calculated in the correct order so that if B depends on A, then A will be calculated before B. The values are all part of the same instance of the problem, they will all be recalculated if the instance parameters change.

Technically the solution I use works very much like a compiled DIC like in e.g. Symfony2.

I am still not sure this is an idea that can be generally recommended, e.g. @igorw had some valid concerns. I also asked on stackoverflow to get some useful feedback, but not sure I asked the right question.

[Seldaek]

I closed the other two PR to clean up the state. Let's discuss here it's a better idea to get a clear plan of action first.

1. I agree here, though I would call them Dumpers maybe, in Composer\Autoload\Dumper\{Psr0,Psr4,...}.
2. I also agree with this, and I think a helper object that can resolve paths probably makes most sense, hoping there is a way to find clear boundaries that will enable all autoload dumpers.
3. I agree about not changing the interface of the AutoloadGenerator in a first step, because I think a first pass of refactoring should be done without touching any test at all. It'll allow to review and merge with much more confidence :) After that's done, maybe minor adjustments to the interface and tests can be done to clean up further, but we should really be careful given the amount of edge cases in this entire thing. About the process object, I'm not sure it changes much. If we implement 2 I think it becomes less necessary.
4. Not sure what to think of that. Sounds overly complex for that task at hand given that the package dependencies can be sorted very well before running all the dumpers. Also we have circular dependencies to take into account.

[donquixote]

A new idea about the dumpers / plugins:
(I am calling it "plugin" because it does not really dump anything yet)

namespace Composer\Autoload\Generator\Plugin;

class Psr4 {
  function generate($build) {
    $snippet = <<<EOT
\$loader->addPsr4(..);
EOT;
    $build->addAutoloaderInitSnippet($snippet);
    $php = <<<EOT
return array(..);
EOT;
    $build->addFile('autoload_psr4.php', $php);
  }
}

Putting it together:

$build = new AutoloadGeneratorBuild();
// Collect the information.
foreach ($plugins as $plugin) {
  $plugin->generate($build);
}
// Create the files.
$build->dump();

We could even extend this for package parsing:

namespace Composer\Autoload\Generator\Plugin;

class Psr4 {

  protected $psr4 = array();

  function addPackage($dir, $package) {
    $this->psr4[..] = ..;
  }

  function generate($build) {

    // Generate the snippet for AutoloaderInit.
    $snippet = <<<EOT
\$loader->addPsr4(..);
EOT;
    $build->addAutoloaderInitSnippet($snippet);

    // Generate the autoload_psr4.php file.
    $export = var_export($this->psr4, TRUE);
    $php = <<<EOT
return $export;
EOT;
    $build->addFile('autoload_psr4.php', $php);
  }
}

Putting it together:

// Parse package data
foreach ($packages as list($dir, $package)) {
  foreach ($plugins as $plugin) {
    $plugin->addPackage($dir, $package);
  }
}

$build = new AutoloadGeneratorBuild();
// Collect the information.
foreach ($plugins as $plugin) {
  $plugin->generate($build);
}
// Create the files.
$build->dump();

[Seldaek]

One thing to remember here that I dont' really see in your above idea is that we need two modes of operation ideally:

• live updating of a classloader to autoload newly installed packages on the fly and/or creating an in memory class loader from scratch
• dumping a config to disk for the app to use

[donquixote]

Does AutoloadGenerator currently do both of that? It does not look like it..

[Seldaek]

It sort of does via createLoader + buildPackageMap + parseAutoloads, but that's exactly the point: right now it's broken and way harder than it should be to achieve full functionality (classmap notably is missing) and it's even too hard to use the parts that work. I'd like the new architecture to take this into account so that we don't fuck up again, now that we know the requirements :)

[donquixote]

Ok, so..
let's say that each plugin/dumper has two methods:

• $plugin->generate($build) will create the PHP snippet for the AutoloaderInit and the to-be-created file, as suggested above.
• $plugin->initClassLoader($loader) will immediately register its stuff to the class loader, instead of generating any PHP code.

So, similar to above:

// Feed package information into the plugins.
foreach ($packages as list($dir, $package)) {
  foreach ($plugins as $plugin) {
    $plugin->addPackage($dir, $package);
  }
}

$loader = new ClassLoader();
// Let plugins register their stuff to the class loader.
foreach ($plugins as $plugin) {
  $plugin->initClassLoader($loader);
}

Or, if we only have one new package:

// We need fresh plugins, to avoid registering the old stuff again.
$plugins = $this->createPlugins();

// Feed package information into the plugins. This time only for the new package.
foreach ($plugins as $plugin) {
  $plugin->addPackage($newPackageDir, $newPackage);
}

$loader = $this->getClassLoader();
// Let plugins register their stuff to the class loader.
foreach ($plugins as $plugin) {
  $plugin->initClassLoader($loader);
}

[Seldaek]

How about having less state and something like:

// Reuse old stateless plugins 
$plugins = $this->createPlugins();

$loader = $this->getClassLoader();
// Let plugins register their stuff to the class loader.
foreach ($plugins as $plugin) {
  $plugin->initClassLoader($loader, $newPackageDir, $newPackage);
}

[donquixote]

This means having more local state in methods in the plugin, and those
methods will grow..
I think it can be a good trade to have some state in the plugins to
simplify the methods themselves.

Also this is about making parts of the plugin easier to reuse.
With your approach, we would need different methods for
iniLoaderForNewPackage, initLoaderForAllPackages, generatePhp.

Maybe we should try both attempts and see which looks better.

2014/1/7 Jordi Boggiano notifications@github.com

How about having less state and something like:

// Reuse old stateless plugins
$plugins = $this->createPlugins();

$loader = $this->getClassLoader();
// Let plugins register their stuff to the class loader.
foreach ($plugins as $plugin) {
$plugin->initClassLoader($loader, $newPackageDir, $newPackage);
}

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/2460#issuecomment-31735847
.

[Seldaek]

Yeah maybe you are right. We could just instantiate the plugins needed
depending on what autoload types the package in question uses anyway, so
that would keep it fairly lightweight.

[donquixote]

I am making quite some progress!
One thing I get stuck with is the sortPackageMap(). This is not documented
at all.

For parseAutoloadsType(), psr-0 and psr-4 use the unsorted package map, but
classmap and files use the sorted package map.
I am sure this has a reason, but it is not properly documented.

Could you explain the sort criterion?

2014/1/7 Jordi Boggiano notifications@github.com

Yeah maybe you are right. We could just instantiate the plugins needed
depending on what autoload types the package in question uses anyway, so
that would keep it fairly lightweight.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/2460#issuecomment-31755136
.

[Seldaek]

The sorting is done to make sure package dependencies work fine, because with files autoload it could be that later packages depend on others to be loaded already. Similarly with classmap packages defined later should be able to override classes or such thing. In any case you should try to preserve that sort algo as is, and maybe you can even use it for psr-4/psr-0 but there we might need to reverse the order in which we go through the array then to keep BC. Anyway I believe there are tests for at least some of this so let's hope they catch anything bad you'd do :)

[donquixote]

The PR is now in a state where I am quite happy with it.
There is still some stuff to be isolated into a separate PR. But this should not stop anyone from reviewing.

I leave the rebase to some later time.
Atm the history represents my thought process and the evolution of the PR.
After the rebase, this history will be lost.

[donquixote]

What do you think about the overall direction?

[donquixote]

I factored out the AutoloadGeneratorTest stuff into a separate PR, which has now been merged.
#2666

The remaining commits are now squashed into one, see
#2563

I'm waiting for reviews!

[Seldaek]

Just not really worth it anymore IMO, the code is pretty stable and changing rarely, no huge benefit in refactoring with possibility of adding regressions.