Fixes #67: Adds ability to disable all or certain installers #376
Conversation
|
Fixes #67 |
# Conflicts: # src/Composer/Installers/Installer.php
|
After I merged in master to resolve the conflict, the build now fails. The test created in master uses PHP5.4 specific code (short array syntax) which fails the PHP5.3 build. https://github.com/composer/installers/pull/378/files#diff-6d9d0d14aa0035388175f3e27359ae67R68 What is the preferred method of fixing this?
|
|
Thanks for report. I fixed problem. Composer Installer is compatibility with PHP 5.3. |
…disable-installers
|
Thanks for the fix. This is now back to passing and ready for review 😄 |
|
@shama you could have a review? |
|
Is there anything else I can or should do to help with this? Thanks |
|
@shama and @niksamokhvalov, This is up-to-date again with the changes recently merged into master |
| { | ||
| $extra = $this->composer->getPackage()->getExtra(); | ||
|
|
||
| if (!isset($extra['installer-disable'])) { |
There was a problem hiding this comment.
It may be worth to consider options [] and false?
There was a problem hiding this comment.
Given that false is the equivalent of not adding this option, I hadn't considered it before. Now that you bring it up I can see where it would be useful in a situation where wikimedia/composer-merge-plugin is being used. I'll add it as an option.
Is your thought that this would be a global, "all installers are enabled"?
Regarding an options array, what did you have in mind? I'm not at all opposed to having available options, but I'm not sure what options to include or what they would do? Or are you suggesting to have it available for future ideas?
There was a problem hiding this comment.
@niksamokhvalov
Just wondering if you had a chance to review the latest commit updating this code. I've added false as an option, but am unsure what options[] you had in mind. I'm open to the idea, but am just unsure of what or how to implement.
| * @param \Composer\Installer\BinaryInstaller|null $binaryInstaller | ||
| */ | ||
| public function __construct( | ||
| \Composer\IO\IOInterface $io, |
There was a problem hiding this comment.
Since these are already imported, can you just use their short classname? Same for docblocks please.
|
Aside from some minor nitpicks that I can sort out after merging as well, is there anything blocking this from being merged at the moment? |
|
Fully qualified namespace issue resolved, and I just pulled in the latest changes from upstream. Should not be any blockers preventing this from being merged. I'm happy to resolve any "nitpicks" you have prior to merge, or you can do it afterwards. It's your choice. |
Adds ability to disable one, multiple, or all installers.