Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

More refactoring for AutoloadGenerator #2298

Closed
wants to merge 10 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

donquixote commented Sep 28, 2013

Follow-up to the refactoring PR for AutoloadGenerator. #2142

Goal:

  • eliminate method parameter madness
  • eliminate local variable madness
  • avoid classes and methods being too long
  • Split the generator into "aspects"
  • Improve documentation
  • Make it look nice in PHPStorm.
Contributor

donquixote commented Sep 28, 2013

@Seldaek: Please don't merge this as-is. I want to clean up the git history (rebase) and we probably want to rename some methods..

Contributor

donquixote commented Sep 28, 2013

And merging with master is going to be fun...

@igorw igorw commented on the diff Sep 28, 2013

...omposer/Autoload/Generator/AutoloadGenerationTask.php
+
+ foreach (new \RecursiveIteratorIterator(new \RecursiveArrayIterator($this->autoloads['classmap'])) as $dir) {
+ $aspect->scanDir($dir);
+ }
+
+ return $aspect;
+ }
+
+ /**
+ * @return Aspect\TargetDirAspect|null
+ */
+ protected function get_targetDirAspect()
+ {
+ // Add custom psr-0 autoloading, if the root package has a target dir.
+ $mainAutoload = $this->mainPackage->getAutoload();
+ if (1
@igorw

igorw Sep 28, 2013

Contributor

what is this for? :)

@donquixote

donquixote Sep 28, 2013

Contributor

to line up the && nicely, and make it easy to rearrange conditions.. similar to the comma after a last element in an array.
i don't insist on it.

@igorw igorw commented on the diff Sep 28, 2013

...Generator/Aspect/AutoloadGeneratorAspectInterface.php
+namespace Composer\Autoload\Generator\Aspect;
+
+use Composer\Autoload\Generator\AutoloadGenerationTask;
+
+interface AutoloadGeneratorAspectInterface {
+
+ /**
+ * Generates the autoload_classmap.php, typically in (project root)/vendor/composer/autoload_classmap.php
+ * Returns the PHP code snippet for AutoloaderInit::getLoader() that will register the class map to the class loader.
+ *
+ * @param AutoloadGenerationTask $task
+ * Information about the current autoload generation task.
+ * @return string
+ * PHP snippet for AutoloaderInit::getLoader().
+ */
+ public function dumpAndGetSnippet(AutoloadGenerationTask $task);
@igorw

igorw Sep 28, 2013

Contributor

Maybe dumping and the snippet should be two separate operations? Especially considering one of them is a command and the other is a query.

@donquixote

donquixote Sep 28, 2013

Contributor

yes they should.
there are "historic" reasons why they are in the same method.
and one practical motivation was that we might reuse some variable.
but none of that is a real reason not to split it up.

@igorw igorw commented on the diff Sep 28, 2013

...omposer/Autoload/Generator/AutoloadGenerationTask.php
+ * @todo verify required keys?
+ * Not doing this allows to use an incomplete version of this, if only one or a few values are needed.
+ * See e.g. AutoloadGenerator::buildPackageMap()
+ */
+ public function __construct(array $data)
+ {
+ $this->data = $data;
+ }
+
+ /**
+ * Gets a lazy value.
+ *
+ * @param $key
+ * @return mixed
+ */
+ public function __get($key)
@igorw

igorw Sep 28, 2013

Contributor

Why this crazy magic getter? I think normal getters would already make this code a lot clearer.

@donquixote

donquixote Sep 28, 2013

Contributor

Because this allows type hints in the IDE via the @property statements in the above docblock.

@igorw

igorw Sep 28, 2013

Contributor

But if you put a @return docblock, you don't need the @property.

@donquixote

donquixote Sep 28, 2013

Contributor

ah, you mean one getter per property, and then if we want a lazy cache, we do that explicitly per method?
I somehow got used to the pattern of a central cache mechanic.. but i might be totally off with that.

@igorw igorw commented on the diff Sep 28, 2013

...omposer/Autoload/Generator/AutoloadGenerationTask.php
+class AutoloadGenerationTask {
+
+ /**
+ * @var array
+ * Lazy-generated data.
+ */
+ protected $data = array();
+
+ /**
+ * @param array $data
+ * Starting values for some data keys.
+ * @todo verify required keys?
+ * Not doing this allows to use an incomplete version of this, if only one or a few values are needed.
+ * See e.g. AutoloadGenerator::buildPackageMap()
+ */
+ public function __construct(array $data)
@igorw

igorw Sep 28, 2013

Contributor

This class has no clearly defined responsibilities. Which is an indication that it is doing way too much. The meaning of this class is completely different depending on what you pass to the constructor. Maybe you pass $config, maybe you pass $mainPackage and $localRepo.

Maybe those distinct responsibilities should be split into separate classes or functions.

@donquixote

donquixote Sep 28, 2013

Contributor

The role is a bit like a compiled DIC..

@igorw

igorw Sep 28, 2013

Contributor

A compiled DIC has a clearly defined set of input parameters. In fact, usually it has no input parameters. What is passed as $data to the constructor here is completely arbitrary.

Most of the time when this class is constructed, it's in an incomplete state. In the sense that some methods will blow up due to missing keys.

@donquixote

donquixote Oct 8, 2013

Contributor

This class has no clearly defined responsibilities. Which is an indication that it is doing way too much.

Well the original AutoloaderGenerator did everything in one class, so it is really hard to be worse than that :)
But I agree, it may still be too much. i can see a number of things that can be removed from this class and put elsewhere.

One argument to have at least some of this in one class is to allow a shared signature for all "aspects" (or better: "partial generators") and then just iterate over the partial generators.

The difference in dependencies for the generators is too arbitrary to justify separate signatures. Maybe their constructors can have separate signatures, but then the getSnippet() and dump() should have a shared signature defined in the interface.

Knowing that one partial dump() needs a project root dir whereas another needs a vendor dir does not really help to separate responsibilities, it is just too arbitrary.

Most of the time when this class is constructed, it's in an incomplete state.

True. E.g. the usage of AutoloadGenerationTask in buildPackageMap() is very cheap and dirty. Better might be to build the package map somewhere outside.
Although.. it feels so convenient!

@stof stof commented on the diff Sep 30, 2013

...omposer/Autoload/Generator/AutoloadGenerationTask.php
+ }
+ return $this->data[$key] = $this->{'get_' . $key}();
+ }
+
+ /**
+ * @return bool
+ */
+ protected function get_useGlobalIncludePath()
+ {
+ return (bool) $this->config->get('use-include-path');
+ }
+
+ /**
+ * @return array
+ */
+ protected function get_packageMap()
@stof

stof Sep 30, 2013

Contributor

Please update the whole PR to follow the PSR-2 coding standards

@donquixote

donquixote Sep 30, 2013

Contributor

Are you talking about the underscore?
Imo it is legitimate to put an underscore in "synthesized" method names, precisely to make them distinguishable from regular user-invented names. I dunno if PSR-2 says anything about that, will check.
Of course, you could disagree with the entire magic stuff from __get()...

@stof stof commented on the diff Sep 30, 2013

...mposer/Autoload/Generator/AutoloadGeneratorHelper.php
+return ComposerAutoloaderInit{$task->suffix}::getLoader();
+
+AUTOLOAD;
+ file_put_contents($task->vendorPath.'/autoload.php', $php);
+ }
+
+ /**
+ * @param AutoloadGenerationTask $task
+ * @param string $loaderSetupCode
+ * @param string $extraMethodsCode
+ * @return bool
+ */
+ public function dumpAutoloadRealFile($task, $loaderSetupCode, $extraMethodsCode)
+ {
+ // TODO the class ComposerAutoloaderInit should be revert to a closure
+ // when APC has been fixed:
@stof

stof Sep 30, 2013

Contributor

It will probably never be fixed as the development of the APC opcode cache has been stopped since the inclusion of Opcache in PHP 5.5.
So the revert to a closure would be possible only once bumping the minimal version to 5.5 (when APC is not considered anymore)

@Seldaek Seldaek closed this Nov 27, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment