Warn if the same package is declared at "require" and "conflict" definitions #12377
Conversation
a5e3a95 to
17124b1
Compare
|
I think I agree with this in principle.. But I am really worried what the impact will be on packagist. I wonder how many packages do this wrong and will suddenly get validation errors. And technically this isn't something that is really wrong, it's merely slightly confusing when reading the requirements. |
|
This could be investigated in Packagist: SELECT count(r.version_id)
FROM require_link r
JOIN conflict_link c ON c.version_id = r.version_id AND c.package_name = r.package_name |
|
Yeah good point.. and that's too many I'd say. 26000 conflicts in 1148 packages found, including 3800 packages that have problems in dev versions (those would start erroring with validation errors). https://packagist.org/packages/ac/web-services-bundle These are just a few examples. So I'd say maybe we rather close this.. Not sure it's worth the hassle to everyone otherwise for a cosmetic "correctness" fix. |
|
|
|
Sure, but this PR is only adding warnings for What I meant by |
|
ah, I misunderstood what you meant with this sentence, indeed. |
|
Thanks for the feedback, I was not aware about the implications at Packagist. Given your concerns, is there something I can do to opt-out this warning in the described scenario? I'd like to identify these cases in my CI pipelines. |
|
I had a closer look and it seems if we gate this behind a new CHECK_* flag (see self::CHECK_STRICT_CONSTRAINTS in the code), then it would be included in the validate command by default (as that uses CHECK_ALL), but NOT on packagist (as that does not pass any flag). So that's a good way to add extra-strict validations IMO. |
|
I'm not sure I even agree with the premise here. I think it may be more accurate to require foo/bar ^2.0 and conflict with 2.4.7 if that contained some weird bug making it incompatible, rather than changing the foo/bar ^2.0 constraint to foo/bar ">= 2.0.0, <= 2.4.6 || ^2.4.8" |
|
FYI: #12384 |
Yup I tend to agree.. this seems too edge-casey. If anything what we could warn on (and what I've seen while exploring this today) is when the conflict matches versions that aren't in the range of the require, because then it's just dead code. But even for that I'd not warn on packagist, just in strict mode when running composer validate. |
17124b1 to
f8eb195
Compare
f8eb195 to
895742d
Compare
|
I just updated the PR by adding the Please, note that my use case is intended to keep the constraint for a single dependency as concise as posible, regardless the boundaries they may have if provided in multiple definitions ( If the check of the "version intersection" for these constraints must be also be considered in this PR, I guess I can give it a try; but I need this extra check to be defined in a different validation flag, as my use case must ignore this detail. Please, let me know what you think. |
Detecting this dead code sounds like a candidate feature request for https://github.com/ergebnis/composer-normalize |
|
Just for clarification about this example:
IMO, the constraint I'd use |
|
Ok I think in the current state I'm ok with the PR, given it won't warn on packagist.org, and users of |
The "require" definition should be updated to avoid the conflicting versions instead of having the constraint split between 2 definitions.
The "conflict" definition should be used only for optional (
require-dev) or transitive dependencies.