search

magento-hackathon / magento-composer-installer

Composer installer for Magento modules
210 stars 154 forks source link

abstract installer in 3.0 #169

Closed davidverholen closed 9 years ago

davidverholen commented 9 years ago

I think the abstract Installer could have way less logic in it.

In the constructor many lines are only about getting config values and set them as class vars in the installer. This could be changed to either have the config class as singleton and always call it directly.

Even better would be, if we implement a DI module to inject any Service classes as we need them. My 2 Favorites for this task are https://github.com/mnapoli/PHP-DI and https://github.com/koriym/Ray.Di

Many other tasks in the Installer, like initializing of the modman and magento root dir should be moved out of the Installer. They could easily be initialized on the pre install or pre package install events.

The path manipulation functions should also be moved in a separate util class. Maybe also in an external library.

The getDeployStrategy function could be moved to an own Factory. Maybe this could also be managed with the DI module to inject the right implementation of the https://github.com/Cotya/Deploy-helper/blob/master/src/Deploy/Strategy/StrategyInterface.php

When this is done, there won't be much left in the Installer. Maybe it's possible then to remove it entirely and just use the underlying libraryInstaller for the basic Installation.

Flyingmana commented 9 years ago

Iam not sure about an DI module here. For configs, a package level config class is maybe better, also I would like to reduce dependencies on the installer class, and even on composer. But that was befor they announced a way to hook scripts as composer commands directly.

For example have a look into DeployAllCommand.php of the 3.0 branch. It does not cover all features yet, but its on the way to work independent from composer.

To Explain, composer is a big big codebase, everything in a single package, and has a lot of customized wrappers and helpers. Its ok for now to still use composer, but I would like to move into a direction, whit less coupling to composer itself, and moving more to the event hooks, with less logic in the installer part.

davidverholen commented 9 years ago

I'm not 100% sure but I think you don't need to require composer in your project. it's enough to have it in require-dev to have the auto completion in your IDE, as it is installed anyway where it is used to deploy. The only Problem could be, that there is an outdated version installed that does not support for example custom commands yet.

The DI module may be too much for such a small project. We could also use Singletons for the service Classes.

But in my opinion one should always prefer using libraries if they can simplify or even reduce/replace your own code - "don't reinvent the wheel".

The installer can be removed completely. We just have to refactor the service classes and then get the rest of the logic in event based classes.

AydinHassan commented 9 years ago

I'm +1 for a DI module, not like there's much code to them anyway, I'd prefer that over statics. Also +1 for dropping composer to a dev requirement. I'd keep it as dev requirement as it's helpful when debugging. I'm pretty sure you can still debug through phars but I've noticed before file formatting can be a bit doggy. I use PHPStorm and my projects always have composer's auto loader configured, so being a dev dependency, composer would always be available for auto completion and debugging purposes.

Also using events in the installer sounds good to me. I only use events in this package.