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

New require-dev handling #1644

Merged
merged 6 commits into from Mar 4, 2013

Conversation

Projects
None yet
7 participants
Owner

Seldaek commented Mar 3, 2013

fixes #719, #1185, #1330, #789, #640
fixes #1005 (update has now --dev enabled by default, install not, it can be disabled on update with --no-dev)

@stof stof commented on the diff Mar 3, 2013

src/Composer/Command/UpdateCommand.php
@@ -68,7 +69,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
->setVerbose($input->getOption('verbose'))
->setPreferSource($input->getOption('prefer-source'))
->setPreferDist($input->getOption('prefer-dist'))
- ->setDevMode($input->getOption('dev'))
+ ->setDevMode(!$input->getOption('no-dev'))
@stof

stof Mar 3, 2013

Contributor

The option --dev does not seem to be used anymore, right ?

@stof stof commented on the diff Mar 3, 2013

src/Composer/Command/InstallCommand.php
@@ -33,7 +33,8 @@ protected function configure()
new InputOption('prefer-source', null, InputOption::VALUE_NONE, 'Forces installation from package sources when possible, including VCS information.'),
new InputOption('prefer-dist', null, InputOption::VALUE_NONE, 'Forces installation from package dist even for dev versions.'),
new InputOption('dry-run', null, InputOption::VALUE_NONE, 'Outputs the operations but will not execute anything (implicitly enables --verbose).'),
- new InputOption('dev', null, InputOption::VALUE_NONE, 'Enables installation of dev-require packages.'),
+ new InputOption('dev', null, InputOption::VALUE_NONE, 'Enables installation of require-dev packages.'),
+ new InputOption('no-dev', null, InputOption::VALUE_NONE, 'Disables installation of require-dev packages (enabled by default, only present for sanity).'),
@stof

stof Mar 3, 2013

Contributor

This no-dev option is simply ignored, isn't it ?

@Seldaek

Seldaek Mar 3, 2013

Owner

Yeah I just kept them in both commands because if you're used to it, and type --dev even though you don't need to and get an error it's unnecessarily painful.

Contributor

till commented Mar 3, 2013

This is confusing behavior. I prefer --dev to be explicit.

Owner

Seldaek commented Mar 3, 2013

@till the rationale is, when you run update, you should be in dev env (rather you should not ever run update on a prod env). Then when running install you may not be, so there it's needed explicitly.

Owner

naderman commented Mar 3, 2013

Looks good to me +1

Seldaek added a commit that referenced this pull request Mar 4, 2013

@Seldaek Seldaek merged commit c95127b into composer:master Mar 4, 2013

1 check passed

default The Travis build passed
Details

@Seldaek Seldaek deleted the Seldaek:newdevrequires branch Mar 4, 2013

Like @till, i like to explicit installation of dev requirement.
It is a personal preference, but I find that the workflow is not explicit

Owner

Seldaek commented Mar 4, 2013

@mikaelrandy I hope that this feeling will go away in time. I get what you're saying but I think it's mostly a matter of habit.

Instead, I find that this choice implies leaving a natural way to use for a more "practical" one

To understand this, just imagine the documentation:
"Dependencies developments are installed by default with the install command, but not with the update command."

For two similar commands, behavior is different.

It's personal, but I think that leaving the natural for the "pratical" leads to a slippery slope

Owner

Seldaek commented Mar 4, 2013

Well yes and no. Install and update are similar in what they do, but have two very different usage contexts. I can't really see a reason to use update without --dev.

I simple use case i use every day : update dependancies and ensure future deployment will work without dev dependancies.

Owner

Seldaek commented Mar 4, 2013

How do you "ensure" this? If your dev requirements are needed for running tests for example, that sounds like a problem.

IMO a better/safer workflow here would be to run update (with --dev), run the test suite, then run install (which will remove the dev requirements) and "verify" by hand that the project still runs. That way you make sure the stuff tested for prod does not change between your dev update and your non dev install, since the install uses the lock file created in the update. With the way you do it, running two updates means some dependencies can change in the mean time, and perhaps your low-key check that things still run after a non-dev update will fail to spot a bug introduced in a new version of a dependency. I don't know if I got my point across well, but I hope you get it.

I'm not convinced.

I got you the point for the tests tools, but i feel bundle have a better way to do : it believe all commands are running in dev mode, unless it is specified.
I think this much more explicit approach is more intuitive

I really think about people who want to use composer, and will be disapointed by this kind of inconsistency

Owner

Seldaek commented Mar 7, 2013

@mikaelrandy so you're saying install should default to dev too? That would be fine by me I think, but can create quite a mess since many lock files don't contain require-dev information.

I'm saying it's important to be consistent, and do not have different behaviour between similar commands (install / update)

Bundler use a "--deployment" option who mean "i'm installing dependancies on a deployment environment, not a development one"

Maybe a "--no-dev" could be great for composer

Owner

Seldaek commented Mar 7, 2013

You can already use --no-dev for install and update. It's just the
default for install at the moment.

I continue to think the behaviour have to be the same on each command

I don't understand why an exception is thrown here. It's not an error per se. When using composer in automated tools (like a continuous integration server), it would be quite tedious to check if the require-dev section is not empty to conditionally add the --dev option when installing the dependencies. What about just doing nothing here instead?

Owner

Seldaek replied Apr 5, 2013

It's not about having require-dev or not. If you don't have any packages-dev here will be an empty array. This only fails if you use a lock file that was created without dev information (either with update --no-dev, which is a dumb idea, or either with a normal update without --dev before this PR was merged). In this case we can't install the dev packages since we don't have them in the lock file. I'll however make sure that doesn't blow up if the composer.json has no require-dev, because probably in this case people didn't run update with --dev and have an outdated "corrupt" lock file.

The way bundler handles this is that all dependencies are installed by default, and you can manually exclude groups (for example dev).

I think this makes sense because during development you run composer install many times, but for the production server you usually configure this in a deployment script.

digitalkaoz pushed a commit to digitalkaoz/composer that referenced this pull request Nov 22, 2013

@chillu chillu referenced this pull request in silverstripe/silverstripe-framework Apr 28, 2014

Closed

Allow PHPUnit installations with composer (and drop PEAR dependency) #3049

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