Adding warning when inexact version constraints are used.

[grasmash]

No description provided.

[grasmash]

Example output:

Gathering patches for root package.
Loading composer repositories with package information
Updating dependencies (including require-dev)
Gathering patches for root package.
Gathering patches for dependencies. This might take a minute.
  - Installing drupal/core (8.2.3)
    Loading from cache

  - Applying patches for drupal/core
    drupal/core has inexact version constraint. This may cause a patch failure now or in the future when the package is changed.
    https://www.drupal.org/files/issues/ignore_front_end_vendor-2329453-116.patch (Ignore front end vendor folders to improve directory search performance)
    https://www.drupal.org/files/issues/2652138-28.patch (2652138 - ImageFormatter does not support SVGs)

[cweagans]

This won't work for dependency patches, right? Also, it may be more efficient to use the Locker if it's available.

[grasmash]

This will not work for dependency patches, although I think that's good because the person running composer install won't have been the person to actually define the dependency patches.

I'm not familiar with "the Locker". Can you explain?

[cweagans]

By the time the plugin executes, Composer will have resolved all package operations to be run (i.e. it has figured out what will be installed). If we're going to add this (and I'm on the fence about it, but I'll come back to this), I think it should warn of potential patch failures that are coming from dependencies as well. A dependency could very easily define a patch for an inexact version constraint.

The Locker is created from composer.lock. It's basically the cached version of all of the package resolution stuff that Composer does, but it has the version constraint that was given for each packages and what specific version will be installed, as well as any patches that will be applied to that package. It's a pretty convenient one-stop-shop for all of this information. It is unfortunately not available if composer.lock doesn't exist (i.e. if you're installing for the first time).

The reason that I'm on the fence about this is that version constraints should very rarely be pinned to a specific version. I get wanting to have a known-good configuration, but the idea is that a developer would use inexact version constraints in their composer.json, run composer install and let composer figure out the specific versions to use, and then have everything pinned in composer.lock. Realistically, this warning would show up for the majority of packages installed and patched by this plugin. I'm wondering if it would be better to simply include a note in the readme about what to expect when your patches and packages get out of sync and how to resolve the issues.

[grasmash]

It is unfortunately not available if composer.lock doesn't exist

I think this precludes us from using the Locker for this pull request. We do need to know the root package version constraints even when there is no lock file present.

I'm wondering if it would be better to simply include a note in the readme about what to expect

This pull request comes from my experience maintaining https://github.com/acquia/blt, which relies on composer-patches. It is very common for developers to define an inexact version constraint, apply a patch, and then experience a failure in the future when the upstream package changes.

It is rare that the developer identifies the cause. In most cases, they are not aware of the role of composer-patches and would not think to read the README.md for this project. For this reason, I think putting a message in the output is the best approach.

I understand that the best practice for Composer version constraints is to use an inexact version, typically using the caret operator. That makes sense when you need to leave Composer the flexibility to choose a version of the dependency that is interoperable with the rest of your requirements. However, I think that a patched dependency is an exception to this rule. That flexibility is often a liability with a patched package, especially when the patch addresses an issue that is being actively worked on upstream. Patching implies an expectation about the exact state of the source code.

Perhaps it would be best to make this warning optional using configuration in the extras array? I would still suggest that it be opt-out rather than opt-in, but that's clearly up to you. Would you like me to update the pull request with this change?

If not, I'd like to find a way to provide this in a separate package. I might be able to respond to the custom patch events that you've defined, but I'd need to do a bit of experimenting to see if that's possible.

[cweagans]

I have no issues with it if there's an option. I think I'd lean toward opt-in if it's displayed any time there's an inexact version constraint.

It is very common for developers to define an inexact version constraint, apply a patch, and then experience a failure in the future when the upstream package changes.

The upstream package should not just change, though. It's pinned to a specific point in time in composer.lock, so the only time that the package should change is when a developer specifically runs composer update vendor/package, right? Can we show the message at that point rather than any time a package has an inexact version constraint?

[grasmash]

Ok, I've added an "inexact-constraint-warning" option in the extra array. It behaves as an opt-in and must be set to true in order for the warning to be displayed.

The upstream package should not just change, though. It's pinned to a specific point in time in composer.lock, so the only time that the package should change is when a developer specifically runs composer update vendor/package, right?

I think it's valuable to show this warning when there is no composer.lock present, which is an install (rather than update) operation. But, it's a matter of opinion. Let me know if you'd like me to add a condition for $operation->getJobType() == 'update'.

As a side note, I wonder if we should consider nesting all such options under a composer-patches key in the extras array. E.g.,

"extra": {
  "composer-patches": {
    "inexact-version-constraints": true,
    "enable-patching": true
  }
}

You might even remove "enable-patching" and assume that patching is enabled when the "composer-patches" key is present. The main issue with this is backwards compatibility, but it could be useful in the 2.x branch.

[cweagans]

I think it's valuable to show this warning when there is no composer.lock present, which is an install (rather than update) operation. But, it's a matter of opinion. Let me know if you'd like me to add a condition for $operation->getJobType() == 'update'.

Fair. I could see this being useful on the first install as well. The larger point there was that the message shouldn't be displayed all the time because users will learn to ignore it. Let me think about this for a bit - this might be a good thing for the 2.x branch too, so some consistent behavior would be good.

As a side note, I wonder if we should consider nesting all such options under a composer-patches key in the extras array

That's the plan in 2.x. I think the top level keys in extra are patches, patches-file, and patches-config. I thought about using the top level config key, but the way that the composer.json schema is written, it will cause composer validate to fail.

[grasmash]

@cweagans I've fielded a few support requests this week alone that stem from patch failures due to this exact issue. I believe this feature would help to educate users.

How do you feel about merging it?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.