Run composer install after adding a patch

[szeidler]

Pull request for #8

[esolitos]

Great work! This works, but it seems it's not updating the composer.lock, is this a composer-patches issue or composer-patches-cli ?

[szeidler]

I think it's a general problem with composer patches. Isn't it the case, that when running composer install with an existing composer.lock file, is not updating that file at all.
So the same issue should also exists without composer-patches-cli and just modifying the composer.json by hand.

cweagans/composer-patches#12

[esolitos]

Actually yes the install should not do anything in this case, because it should only look in the lockfile (the fact that the patch gets applied anyway it's definitely bug).

A composer update pkg/package should be triggered, so the patch gets applied and added to the lockfile.

[szeidler]

composer update pkg/package could lead to an undesired update of the package, depending on the version constraints. Maybe we could solve it with a parameter on the composer-patches-cli to make it a non-default behavior?

I'm a bit confused, because all those issues ideally would need to be addressed in cweagans library.

[esolitos]

composer update pkg/package could lead to an undesired update of the package

While this is true and I though about it: I would assume that if you're patching a package you have probably either already tested (and running) the most recent version or have a strict version requirement.
So overall it should be fine, and if not fine could indicate that one is doing something "wrong", if you see what I mean.

Maybe we could solve it with a parameter on the composer-patches-cli to make it a non-default behavior?

I like the idea of having it configurable (via parameter or in composer.json), but I would argue that applying a patch should be the default behaviour for the reasons expressed above.

all those issues ideally would need to be addressed in cweagans library.

Not really or not until they provide a CLI command IMHO.

The fact that patches are installed running composer install it's definitely a bug as pointed out in cweagans/composer-patches#12 , because the patches at that point are not in the composer.lock, which should be there to prevent inconsistent states.

[szeidler]

but I would argue that applying a patch should be the default behaviour for the reasons expressed above.

Maybe a negative parameter would make sense here, then, like --no-update. I just updated the pull request, to run an update command (without update the dependencies of the patched package).

[esolitos]

From a quick test the latest change seems to be triggering a full composer update, disregarding the patched module.
The patch is indeed applied and the composer.lock updated, but also all dependencies are updated.

Looking at the UpdateCommand command it seems would make sense to use the $composer->setUpdateWhitelist([$patched_module]).

I've attached a patch of the (tested) changes against this branch to get this behaviour.
In the patch I added also the --no-update and a couple of extra parameters to pass to Installer.

composer-patches-cli--pr9--setUpdateWhitelist.patch

[szeidler]

Thanks for the input @esolitos . I will update the pull request.

One question concerning

// Patches are always considered to be applied in "dev mode".
// This is also required to prevent composer from removing all installed
// dev dependencies.
// TODO: Maybe consider adding a 'no-dev' option to the command?
->setDevMode(TRUE)

Could you elaborate on this? Providing it as an option is quite straight forward, but I like to understand your comment and the consequences of it.

[esolitos]

So, actually that it probably not a great comment, the issue is that without the ->setDevMode(TRUE) the composer install call will remove all dev-dependencies, because it considers the install as "prod" (or non-dev).

Looking at composer install it defaults to --dev to be true, so I thought that would make sense to follow this behavior.