Feature dist (bis) #1024

Closed
wants to merge 13 commits into from

6 participants

@MattKetmo

This is a refacto of #804 from @till.

It adds the ability to create zip/tar archives from sources.

The main differences with the initial PR:

  • It uses a separate directory, named Archiver instead of Dumper, because as @Seldaek said, it has nothing to do with ArrayDumper
  • It uses a manager which have the main function archive($package, $format), which will use its own archivers, so no code is duplicated
  • We first try to use VCS archivers (git & hg archive commands). If the VCS is not able to archive a package (for instance svn or any future VCS which could be added to Composer), then it uses standard tar/zip archiver (which archive a directory no matter the source type).

I preferred to start from master because the initial PR started to have to much dummy commits, but I credited @till in the sources ;)

@travisbot

This pull request passes (merged 8750a5fc into f08c748).

@stof

It would have been better to squash @till's commits together to keep his authorship (assuming you don't want to keep the whole history of the implementation). Thus, it would have allowed seeing the differences from the previous PR more easily.

@stof stof commented on an outdated diff Aug 24, 2012
src/Composer/Package/Archiver/BaseArchiver.php
+abstract class BaseArchiver implements ArchiverInterface
+{
+ /**
+ * Create a PHAR archive.
+ *
+ * @param string $sources Path of the directory to archive
+ * @param string $target Path of the file archive to create
+ * @param int $format Format of the archive
+ */
+ protected function createPharArchive($sources, $target, $format)
+ {
+ try {
+ $phar = new \PharData($target, null, null, $format);
+ $phar->buildFromDirectory($sources);
+ } catch (\UnexpectedValueException $e) {
+ throw new \RuntimeException(
@stof
stof added a note Aug 24, 2012

you should add the original exception as previous exception

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Aug 24, 2012
src/Composer/Package/Archiver/GitArchiver.php
+ /**
+ * {@inheritdoc}
+ */
+ public function archive($source, $target)
+ {
+ $format = $this->format ?: 'zip';
+ $sourceRef = $this->sourceRef ?: 'HEAD';
+
+ $command = sprintf(
+ 'git archive --format %s --output %s %s',
+ $format,
+ escapeshellarg($target),
+ $sourceRef
+ );
+
+ $this->process->execute($command, $output, $source);
@stof
stof added a note Aug 24, 2012

you should check the exit code of the process to provide some error management if building the archive failed

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

Yeah, it would be nice if authorship was preserved in the history. A lot of blood, sweat and tears went into this.

@Seldaek
Composer member

@MattKetmo if you can fix authorship please do it, if you can't well leave it and I'll fix things up when merging.

@Seldaek
Composer member

As for the PR, I haven't looked at the code yet but from your description I see one problem, using git/hg to archive means we can't really have composer-specific exclude rules for files. I don't think we can just rely on .gitattributes for excluding files from the archive, I'm pretty sure some people will want to exclude files only from packages but not from github downloads for example. What I'm talking about is #57, although I think it should rather be in the composer.json as {"archive":{"exclude":["path", "file", "..."]}}

@stof

@Seldaek but this would mean that if you use the same github repo through the VCS driver (so using github downloads for dists) or through Satis (assuming it generates archives, which is the goal of adding this feature in composer), you end with different dists.
thus, if you have {"archive":{"exclude":["path", "file", "..."]}}, people might expect packages coming from packagist to respect it. Do you plan to generate archives for every version registered on packagist instead of using github downloads ?

@Seldaek
Composer member
@MattKetmo

@stof @till : I've rebased & squashed initial code to keep authorship. Here is the result: https://github.com/MattKetmo/composer/compare/till-feat-dist

If it's ok for you I will rebase this branch into the squashed one.

@stof

@Seldaek but then, what if people have a .gitattributes in their package ? do they need to duplicate it in the composer.json archive config or should the archiver take it into account (in the first case, it is likely to break things as you can have 2 totally different set of ignored files, not one being a subset of the other, meaning you have to ensure both ways keep working)

@Seldaek
Composer member
@stof

@MattKetmo If you revert everything, it does not keep the history either. Your improvements should be built from @till's version, having a commit moving renaming the namespace (so that git can link the history of the moved file) and then refactoring it.

@Seldaek
Composer member

You don't need the move commit I think, just rm dead files, add yours on top and git should take care of it all.

@stof

@Seldaek If they look close enough, yes. But as the code has been splitted into several archives, I fear git may not consider it as moved file if you do all in once but as removed the old version totally and added an independant one

@MattKetmo

Well, I've never started from @till's commits so I've never moved his files to the directory I created. I just took the global idea starting again from master.

I could changed my git history like if I did it, but it may looks weird. Or I could git rm and git add (the "Created the archiver package" commit) in the same commit, as @Seldaek said. It won't be a file rename, but the old and the new files will be in the same commit.

@travisbot

This pull request fails (merged bc8bcbe7 into f4f14b6).

@travisbot

This pull request passes (merged 757651bf into f4f14b6).

@MattKetmo

I've changed my commits history so that we can see the code has been initiated by @till then refactored into the archiver package.

There are some improvements I want to add to the ArchiveManager to customize the created archive. For information, it's quite easy to add some options, for instance to exclude some files (we are not obliged to use VCS archivers), change the archive path, etc... The PR is a base, those improvements could be add later.

@travisbot

This pull request passes (merged 59a3c196 into f4f14b6).

@stof stof and 1 other commented on an outdated diff Aug 27, 2012
src/Composer/Package/Archiver/ArchiveManager.php
+
+ if (null !== $downloadManager) {
+ $this->downloadManager = $downloadManager;
+ } else {
+ $factory = new Factory();
+ $this->downloadManager = $factory->createDownloadManager(new NullIO());
+ }
+ }
+
+ /**
+ * @param ArchiverInterface $archiver
+ */
+ public function addArchiver(ArchiverInterface $archiver)
+ {
+ if ($archiver instanceof VcsArchiver) {
+ $this->vcsArchivers[$archiver->getSourceType()] = $archiver;
@stof
stof added a note Aug 27, 2012

do you really need 2 different lists of archivers ? composer does not need it for downloaders and installers

When you download package there is one way to do it :

  • zip|tar|whateverarchive = copy the file
  • git = use git clone
  • hg = use hg clone
  • etc.

When you want to archive a package, you have the choice between using the vcs command if available (git/hg archive), or standard method (zip or tar a directory or files). VCS archivers are optionals, there are just here to "optimize" the archiving method depending on the source type.

Btw, instead of creating the phar file with buildFromDirectory(), I could use the Symfony Finder to ignore VCS files.

@stof
stof added a note Aug 27, 2012

but they should simply be registered first, so that their support method is called first. I don't think handlign them separately is needed

VcsArchivers have a format support method (zip, tar, etc.) but also a source type support (plus a sourceRef property). So we use them differently.

If you want to homogenize all archivers, I can had those properties to the interface (sourceRef and sourceType), but it will have no meaning for basic tar/zip archivers.

In the case I change the ArchiverInterface, what do you think of:

public function archive($sources, $target, $format, $sourceRef = null);
public function supports($format, $sourceType = null);
@stof
stof added a note Aug 27, 2012

I would not make $sourceType an optional argument in the interface. You will always pass it anyway

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

The Tar and Zip archivers will not exclude the VCS metadata from the archive (the .git folder for git), which may break things

@stof stof referenced this pull request in composer/satis Aug 28, 2012
Closed

Create dist packages #12

@travisbot

This pull request fails (merged bd3ae10f into f4f14b6).

@travisbot

This pull request passes (merged b3b430d2 into e2f8098).

@stof stof and 1 other commented on an outdated diff Aug 28, 2012
src/Composer/Package/Archiver/ArchiveManager.php
+ * @param string $format The format of the archive (zip, tar, ...)
+ *
+ * @return string The path of the created archive
+ */
+ public function archive(PackageInterface $package, $format)
+ {
+ if (empty($format)) {
+ throw new \InvalidArgumentException('Format must be specified');
+ }
+
+ $usableArchiver = null;
+ $sourceType = $package->getSourceType();
+
+ foreach ($this->archivers as $archiver) {
+ if ($archiver->supports($format, $package->getSourceType())) {
+ $usableArchiver = $archiver;
@stof
stof added a note Aug 28, 2012

you need to break the loop, otherwise you will use the last found, not the first one

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Aug 28, 2012
src/Composer/Package/Archiver/ArchiveManager.php
+ foreach ($this->archivers as $archiver) {
+ if ($archiver->supports($format, $package->getSourceType())) {
+ $usableArchiver = $archiver;
+ }
+ }
+
+ // Checks the format/source type are supported before downloading the package
+ if (null === $usableArchiver) {
+ throw new \RuntimeException(sprintf('No archiver found to support %s format', $format));
+ }
+
+ $filesystem = new Filesystem();
+ $packageName = str_replace('/', DIRECTORY_SEPARATOR, $package->getUniqueName());
+
+ // Directory used to download the sources
+ $sources = sys_get_temp_dir().DIRECTORY_SEPARATOR.$packageName;
@stof
stof added a note Aug 28, 2012

don't bother about using DIRECTORY_SEPARATOR everywhere. It is useless in PHP 5.3 as it accepts / even on Windows.

The only case where it is important is when you need to do some string manipulation with the path

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Aug 28, 2012
src/Composer/Package/Archiver/BaseArchiver.php
@@ -0,0 +1,46 @@
+<?php
+
+/*
+ * This file is part of Composer.
+ *
+ * (c) Nils Adermann <naderman@naderman.de>
+ * Jordi Boggiano <j.boggiano@seld.be>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Composer\Package\Archiver;
+
+use Composer\Package\BasePackage;
@stof
stof added a note Aug 28, 2012

unused use statement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Aug 28, 2012
src/Composer/Package/Archiver/GitArchiver.php
+ if (null === $sourceRef || preg_match('/^[0-9a-f]{40}$/i',$sourceRef)) {
+ $sourceRef = 'HEAD';
+ }
+
+ $command = sprintf(
+ 'git archive --format %s --output %s %s',
+ $format,
+ escapeshellarg($target),
+ $sourceRef
+ );
+
+ $exitCode = $this->process->execute($command, $output, $sources);
+
+ if (0 !== $exitCode) {
+ throw new \RuntimeException(
+ sprintf('The command `%s` returned %s', $command, $exitCode)
@stof
stof added a note Aug 28, 2012

Please use a better exception message (Impossible to build the archive ...), otherwise you will have to catch it and wrap it in the ArchiverManager

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Aug 28, 2012
src/Composer/Package/Archiver/GitArchiver.php
+ if (0 !== $exitCode) {
+ throw new \RuntimeException(
+ sprintf('The command `%s` returned %s', $command, $exitCode)
+ );
+ }
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function supports($format, $sourceType)
+ {
+ return 'git' === $sourceType && in_array($format, array(
+ 'zip',
+ 'tar',
+ 'tgz',
@stof
stof added a note Aug 28, 2012

you could keep the array inline. It does not make the line too long and is more readable as the main part of the statement is not about defining the array but about doing the condition

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Aug 28, 2012
src/Composer/Package/Archiver/MercurialArchiver.php
+ if (null === $sourceRef) {
+ $sourceRef = 'default';
+ }
+
+ $command = sprintf(
+ 'hg archive --rev %s --type %s %s',
+ $sourceRef,
+ $format,
+ escapeshellarg($target)
+ );
+
+ $exitCode = $this->process->execute($command, $output, $sources);
+
+ if (0 !== $exitCode) {
+ throw new \RuntimeException(
+ sprintf('The command `%s` returned %s', $command, $exitCode)
@stof
stof added a note Aug 28, 2012

same issue here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Aug 28, 2012
src/Composer/Package/Archiver/PharArchiver.php
+ * @author Till Klampaeckel <till@php.net>
+ * @author Matthieu Moquet <matthieu@moquet.net>
+ */
+class PharArchiver extends BaseArchiver
+{
+ static public $formats = array(
+ 'zip' => \Phar::ZIP,
+ 'tar' => \Phar::TAR,
+ );
+
+ /**
+ * {@inheritdoc}
+ */
+ public function archive($sources, $target, $format, $sourceRef = null)
+ {
+ // source reference is useless for this archiver
@stof
stof added a note Aug 28, 2012

this comment is also useless :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Aug 28, 2012
src/Composer/Package/Archiver/PharArchiver.php
+
+ /**
+ * {@inheritdoc}
+ */
+ public function archive($sources, $target, $format, $sourceRef = null)
+ {
+ // source reference is useless for this archiver
+ $this->createPharArchive($sources, $target, static::$formats[$format]);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function supports($format, $sourceType)
+ {
+ return in_array($format, array_keys(static::$formats));
@stof
stof added a note Aug 28, 2012

return isset(static::$formats[$format]) will be faster

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Aug 28, 2012
src/Composer/Package/Archiver/PharArchiver.php
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Composer\Package\Archiver;
+
+use Composer\Package\BasePackage;
+use Composer\Package\PackageInterface;
+
+/**
+ * @author Till Klampaeckel <till@php.net>
+ * @author Matthieu Moquet <matthieu@moquet.net>
+ */
+class PharArchiver extends BaseArchiver
+{
+ static public $formats = array(
@stof
stof added a note Aug 28, 2012

does it really need to be public ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Aug 28, 2012
src/Composer/Package/Archiver/BaseArchiver.php
+ * Jordi Boggiano <j.boggiano@seld.be>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Composer\Package\Archiver;
+
+use Composer\Package\BasePackage;
+use Composer\Package\PackageInterface;
+
+/**
+ * @author Till Klampaeckel <till@php.net>
+ * @author Matthieu Moquet <matthieu@moquet.net>
+ */
+abstract class BaseArchiver implements ArchiverInterface
@stof
stof added a note Aug 28, 2012

A base class used by only 1 of the 4 implementations looks useless. You should define the method directly in the PharArchiver

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

you should also add a method in the factory to create an ArchiveManager.

And btw, I would make the DownloadManager required in the ArchiveManager, as it is not its role to build a factory and to create a DownloadManager. However, you can make this argument optional for the method of the factory

@travisbot

This pull request passes (merged 410c7c71 into e2f8098).

@stof stof commented on an outdated diff Aug 28, 2012
src/Composer/Factory.php
@@ -234,6 +235,31 @@ public function createDownloadManager(IOInterface $io)
}
/**
+ * @param string $workDir Directory used to download sources
+ * @param DownloadManager $dm Manager use to download sources
+ *
+ * @return ArchiveManager
@stof
stof added a note Aug 28, 2012

this phpdoc is wrong as there is no use statement for the class, meaning it will be resolved to the wrong class name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Aug 28, 2012
src/Composer/Factory.php
@@ -234,6 +235,31 @@ public function createDownloadManager(IOInterface $io)
}
/**
+ * @param string $workDir Directory used to download sources
+ * @param DownloadManager $dm Manager use to download sources
+ *
+ * @return ArchiveManager
+ */
+ public function createArchiveManager($workDir = null, DownloadManager $dm = null)
+ {
+ if (null === $dm) {
+ $factory = new Factory();
@stof
stof added a note Aug 28, 2012

don't create a new factory. use $this

huhu, that's true! ^^

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

This pull request passes (merged b64714de into e2f8098).

@MattKetmo

I've cleaned &rebased the PR on master. It can be merged if there is nothing else to add.

@till

Can haz merge? /cc @Seldaek, @naderman

@till till referenced this pull request Sep 11, 2012
Closed

Feature dist #804

@stof stof commented on an outdated diff Sep 24, 2012
src/Composer/Package/Archiver/ArchiveManager.php
+ foreach ($this->archivers as $archiver) {
+ if ($archiver->supports($format, $package->getSourceType())) {
+ $usableArchiver = $archiver;
+ break;
+ }
+ }
+
+ // Checks the format/source type are supported before downloading the package
+ if (null === $usableArchiver) {
+ throw new \RuntimeException(sprintf('No archiver found to support %s format', $format));
+ }
+
+ // Directory used to download the sources
+ $filesystem = new Filesystem();
+ $packageName = $package->getUniqueName();
+ $sources = sys_get_temp_dir().'/'.$packageName;
@stof
stof added a note Sep 24, 2012

shouldn't you be using the working dir of the ArchiveManager here ?

@stof
stof added a note Sep 24, 2012

btw, the variable name is weird. $sourcePath would be better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff Sep 24, 2012
src/Composer/Package/Archiver/GitArchiver.php
+
+ public function __construct($process = null)
+ {
+ $this->process = $process ?: new ProcessExecutor();
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function archive($sources, $target, $format, $sourceRef = null)
+ {
+ // Since git-archive no longer works with a commit ID in git 1.7.10,
+ // use by default the HEAD reference instead of the commit sha1
+ if (null === $sourceRef || preg_match('/^[0-9a-f]{40}$/i', $sourceRef)) {
+ $sourceRef = 'HEAD';
+ }
@stof
stof added a note Sep 24, 2012

This is wrong IMO. It should checkout the appropriate reference first to be sure it builds the right archive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Sep 24, 2012
src/Composer/Package/Archiver/GitArchiver.php
+ /**
+ * {@inheritdoc}
+ */
+ public function archive($sources, $target, $format, $sourceRef = null)
+ {
+ // Since git-archive no longer works with a commit ID in git 1.7.10,
+ // use by default the HEAD reference instead of the commit sha1
+ if (null === $sourceRef || preg_match('/^[0-9a-f]{40}$/i', $sourceRef)) {
+ $sourceRef = 'HEAD';
+ }
+
+ $command = sprintf(
+ 'git archive --format %s --output %s %s',
+ $format,
+ escapeshellarg($target),
+ $sourceRef
@stof
stof added a note Sep 24, 2012

you need to use escapeshellarg here too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Sep 24, 2012
src/Composer/Package/Archiver/MercurialArchiver.php
+ {
+ $this->process = $process ?: new ProcessExecutor();
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function archive($sources, $target, $format, $sourceRef = null)
+ {
+ if (null === $sourceRef) {
+ $sourceRef = 'default';
+ }
+
+ $command = sprintf(
+ 'hg archive --rev %s --type %s %s',
+ $sourceRef,
@stof
stof added a note Sep 24, 2012

you need to use escapeshellarg here too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Sep 24, 2012
src/Composer/Package/Archiver/PharArchiver.php
+ * @author Till Klampaeckel <till@php.net>
+ * @author Matthieu Moquet <matthieu@moquet.net>
+ */
+class PharArchiver implements ArchiverInterface
+{
+ static protected $formats = array(
+ 'zip' => \Phar::ZIP,
+ 'tar' => \Phar::TAR,
+ );
+
+ /**
+ * {@inheritdoc}
+ */
+ public function archive($sources, $target, $format, $sourceRef = null)
+ {
+ $this->createPharArchive($sources, $target, static::$formats[$format]);
@stof
stof added a note Sep 24, 2012

do you really need a separate method here ? Other archivers are putting the logic in archive directly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Sep 24, 2012
tests/Composer/Test/Package/Archiver/ArchiverTest.php
+ $result = $this->process->execute('git init -q');
+ if ($result > 0) {
+ throw new \RuntimeException('Could not init: '.$this->process->getErrorOutput());
+ }
+
+ $result = file_put_contents('b', 'a');
+ if (false === $result) {
+ throw new \RuntimeException('Could not save file.');
+ }
+
+ $result = $this->process->execute('git add b && git commit -m "commit b" -q');
+ if ($result > 0) {
+ throw new \RuntimeException('Could not commit: '.$this->process->getErrorOutput());
+ }
+
+ chdir($currentWorkDir);
@stof
stof added a note Sep 24, 2012

this should also be done before throwing an exception

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Sep 24, 2012
tests/Composer/Test/Package/Archiver/ArchiverTest.php
+ /**
+ * @var string
+ */
+ protected $testDir;
+
+ public function setUp()
+ {
+ $this->filesystem = new Filesystem();
+ $this->process = new ProcessExecutor();
+ $this->testDir = sys_get_temp_dir().'/composer_archivertest_git_repository'.mt_rand();
+ }
+
+ /**
+ * Create local git repository to run tests against!
+ */
+ protected function setupGitRepo()
@stof
stof added a note Sep 24, 2012

Why putting this method here ? It is only useful for the GitArchiverTest and so should go in this class

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

@MattKetmo Any update on this? Do you need help to clean this up?

MattKetmo and others added some commits Aug 23, 2012
@MattKetmo MattKetmo Refactored the archiver package 7be3944
@till till Initial feature-dist
 * extends BaseDumper, implements interface
 * put $keys into BaseDumper

 * WIP WIP WIP WIP
 * BaseDumper for utilities
 * interface to enforce 'dump()'
 * feature:
   * supports git
   * supports zip output
   * basic test to cover feature

 * add @todo for later
 * add vendor namespace to package name

 * add extension to getFilename() so we don't need to switch in there (HT, @naderman)

 * add extension (obviously 'zip' in ZipDumper)

 * create archive in destination dir (provided by __construct())

 * condensed ZipDumper
 * moved code to BaseDumper (hopefully easier re-use)

 * use ProcessExecutor from BaseDumper

 * fix assignments in __construct()
 * allow injection of ProcessExecutor

 * fix parameters

 * fix regex

 * write in 'system temp dir'
 * update test case (oh look, a duplicate regex)

 * move working directory related to BaseDumper

 * add quotes

 * place holder for these methods

 * use PharData to create zip/tar when necessary

 * add placeholder calls
 * add call to package() using PharData

 * finish downloadHg(), downloadSvn()

 * put to use

 * make BaseDumper abstract (to force extension)
 * make BaseDumper implement Interface (makes for less code in the implementation)

new functionality for dumping as .tar.gz

tar instead of tar.gz, new abstract dumpertest class

creates a local git repo instead of fetching a remote one

more oo-ish version of it

no constructor

 * refactor tests to be less linux-specific (used Composer\Util to wrap calls)

 * make filename only the version

 * various cs fixes (idention, tabs/spaces, doc blocks, etc.)
 * fixed a typo'd exception name

 * refactored downloading:
   * removed download*() methods
   * added dep on Composer\Factory to setup a DownloadManager instance

 * update CS with feedback from @stof

 * ArrayDumper doesn't extend BaseDumper anymore (hence no conflict on the interface)
 * move keys from BaseDumper back to ArrayDumper
 * interface now declares dump() to always return void

Apparently I had to update the lock.

CS fixes (tabs for spaces)
Bugfix: sprintf() was missing.

Fix docblock for @stof. ;)

Pull in lock from master.

Update lock one more time (hope it still merges).

whitespace

Revert ArrayDumper static keys
ad15b25
@MattKetmo MattKetmo Moved archive Dumpers into its own Archiver package 3718e03
@MattKetmo MattKetmo Changed Package class due to upstream changes cbe85a9
@MattKetmo MattKetmo Checks process execution a179e55
@MattKetmo MattKetmo Update interface to merge vcs with basic archivers ffc0acc
@MattKetmo MattKetmo Checks support before downloading the package 2a5699a
@MattKetmo MattKetmo Merged zip & tar archivers 5c03eb8
@MattKetmo MattKetmo Fixed several typos
- break at first archiver supports
- use standard directory separator
- change exception message
- remove the BaseArchiver since tar & zip archivers have been merged
- plus coding style
cdf2965
@MattKetmo MattKetmo Create ArchiveManager with the Factory fc137d7
@MattKetmo MattKetmo Fix how download manager is constructed
This fixes tests due to upstream changes.
The createDownloadManager in the Factory now takes the config as extra
parameter.
d1d4b5b
@MattKetmo MattKetmo Fix workflow & typos 9abec22
@MattKetmo MattKetmo Cleaned archiver tests a4c6d96
@MattKetmo

Sorry i didn't have much time those days to spend on this PR.

I've rebased my branch to make it up to date with upstream & cleaned some methods as pointed out by @stof.

@till sure, I'll appreciate your help. I add you as collaborator of MattKetmo/composer so you can commit in the branch without me ;)

@stof

@Seldaek anything missing here ?

@stof stof commented on the diff Jan 23, 2013
src/Composer/Factory.php
@@ -243,6 +244,26 @@ public function createDownloadManager(IOInterface $io, Config $config)
}
/**
+ * @param Downloader\DownloadManager $dm Manager use to download sources
+ * @param Config $config The configuration
+ *
+ * @return Archiver\ArchiveManager
+ */
+ public function createArchiveManager(DownloadManager $dm = null, Config $config)
@stof
stof added a note Jan 23, 2013

Optional arguments should be placed after mandatory ones (otherwise they cannot be optional)

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

Ping!

@naderman naderman referenced this pull request Feb 7, 2013
Merged

Distributions/Archives #1567

@naderman
Composer member

Opened a new PR to expand on this: #1567

@naderman naderman closed this Feb 7, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment