Refactor ignore platform reqs checks #10079

herndlm · 2021-08-26T20:56:55Z

Introduces a PlatformRequirementFilter with methods that help to decide if a requirement is ignored or not as discussed in #10045 but without changing behaviour.
The idea behind this is to be able to adapt simple wildcard or regex ignores in a follow-up.

I changed as much as needed so that the filter and its logic is used everywhere where the bool|string[] config was before, because it felt inconsistent otherwise. But I also tried to change as less as possible which is the reason the argument is nullable in some cases and falls back to the nothing filter then or is defined like that in the constructor.
Initially I only had one filter class, but this became already too complex and mixed regarding types in my opinion, so I split responsibilities into dedicated filter classes, which is of course more code/classes but way simpler to read/understand/test.

herndlm · 2021-08-29T07:29:54Z

Is it safe to do those changes from an API perspective? E.g. could plugins use those classes? And if so - if they extend from them too, can we even change any non-private signatures?

If not - a less invasive alternative I can think of is moving the decision logic into static helper methods or so.

Seldaek

IMO it's mostly looking good, thanks for the work!

The few places I commented on it'd be good to keep the old API functional, but the other places are internal-enough that we shouldn't bother. When restoring setters for BC, we could add trigger_error(E_USER_DEPRECATED...) to those few places so people notice and migrate away.

Note that for the follow-up of adding wildcard support, using this code is probably a good idea to turn wildcards into regexes, we do this in a few places already: https://github.com/composer/composer/blob/master/src/Composer/Package/BasePackage.php#L251-L256

src/Composer/Installer.php

src/Composer/Package/Version/VersionSelector.php

src/Composer/Autoload/AutoloadGenerator.php

herndlm · 2021-08-29T13:49:29Z

IMO it's mostly looking good, thanks for the work!

I'm very glad to hear that, thx!

The few places I commented on it'd be good to keep the old API functional, but the other places are internal-enough that we shouldn't bother. When restoring setters for BC, we could add trigger_error(E_USER_DEPRECATED...) to those few places so people notice and migrate away.

Yes, keeping BC here is luckily as simple as that. I adapted it and added @deprecated phpdoc annotations too.

Note that for the follow-up of adding wildcard support, using this code is probably a good idea to turn wildcards into regexes, we do this in a few places already: https://github.com/composer/composer/blob/master/src/Composer/Package/BasePackage.php#L251-L256

Thx for the hint, I stumbled over that already yesterday and thought of using it (+ adding a few tests). Previously I was looking at fnmatch, which is not available on exotic platforms apparently though.

It's still early but as my commits are ugly fixups - do you squash them automatically on merge or shall I just do that and force-push after approval?

Seldaek · 2021-08-29T14:37:18Z

I can squash on merge. I'll leave this open until master starts targetting 2.2 though. If you want to do a follow-up PR with the pattern matching you could already open a second one that is based on this branch?

herndlm · 2021-08-29T17:39:19Z

Sorry, rebased with auto-squash after all.. I was afraid this might otherwise mess up the new branch / PR on merge and wanted to start clean there.

Seldaek · 2021-08-29T18:01:52Z

No worries :)

herndlm · 2021-09-05T17:14:56Z

rebased to fix a conflict

helhum

Nice cleanup imho

imme-emosol

To move away from mixed data-types such as bool|array as soon as possible in the code, seems like a good idea and along those lines, these changes seem to be a good idea as well.

Why would the specific filters include the abstract class PlatformRequirementFilter.
Is it needed to use the class name of one of the specific filter-classes to use one of the methods provided by the abstract class? Like IgnoreAllPlatformRequirementFilter::ignoreNothing(), is that something to use?

Would it not make more sense to put the abstract public function isReqIgnored($req); in a interface that holds the contract for all RequirementFilter`s ?

The is{All,Nothing}Ignored methods might be useful.. then again.., that instanceof check could be an interface as well (the difference in calling code being if that has a method-call or a call to instanceof, so: $filter->isAllIgnored() vs $filter instanceof IgnoreAllReqsFiltering), although that interface would seem to remain empty/anemic as of now.

herndlm · 2021-10-12T10:28:19Z

Why would the specific filters include the abstract class PlatformRequirementFilter. Is it needed to use the class name of one of the specific filter-classes to use one of the methods provided by the abstract class? Like IgnoreAllPlatformRequirementFilter::ignoreNothing(), is that something to use?

Would it not make more sense to put the abstract public function isReqIgnored($req); in a interface that holds the contract for all RequirementFilter`s ?

The is{All,Nothing}Ignored methods might be useful.. then again.., that instanceof check could be an interface as well (the difference in calling code being if that has a method-call or a call to instanceof, so: $filter->isAllIgnored() vs $filter instanceof IgnoreAllReqsFiltering), although that interface would seem to remain empty/anemic as of now.

I agree, actually I prefer a composition only version as well and wanted to use it initially.
To achieve that

the factory methods ignoreAll() and ignoreNothing() could either be moved to create() methods in the specific classes or factory methods in e.g. a PlatformRequirementFilterFactory I guess
isAllIgnored usage would be replaced with instanceof checks as you mentioned
isNothingIgnored is currently not even used IIRC
the factory method fromBoolOrList could be moved to e.g. a PlatformRequirementFilterFactory
PlatformRequirementFilter would then be an interface (maybe renamed to PlatformRequirementFilterInterface depending on the class name style of Composer) with only a isReqIgnored method.

I'm happy to adapt that. Some tests can then be even removed.

What do you think @Seldaek?

UPDATE: I did the adaptions locally, just waiting for confirmation.

imme-emosol · 2021-10-12T21:06:12Z

My comment #9534 (comment) might affect this issue.

herndlm · 2021-10-13T07:14:00Z

My comment #9534 (comment) might affect this issue.

I'm not sure if it does or if I missed something. The intention here was "just" to refactor the platform reqs checks and prepare them to add more functionality like glob like wildcards for which I added another PR. I wasn't even aware of the feature-request regarding upper bounds. Should be independent at least if I'm not mistaken. And would be easier to implement afterwards for sure.

herndlm · 2021-10-13T08:00:12Z

rebased to fix a conflict

imme-emosol · 2021-10-13T08:06:49Z

My comment #9534 (comment) might affect this issue.

I'm not sure if it does or if I missed something. [...] Should be independent at least if I'm not mistaken. And would be easier to implement afterwards for sure.

You are right in that the code changes made here will probably make things easier anyway indeed.

herndlm · 2021-10-18T08:52:35Z

rebased to fix a conflict and added the inheritance -> composition changes in a fixup as I requested a re-review already anyways

Fyi, I also checked the new Filter namespace with phpstan level 6, seems to be fine.

@imme-emosol what do you think? (and btw sorry, I clicked multiple times on the re-request review icon but nothing changed. No idea if you were spammed by that xD)

imme-emosol · 2021-10-18T11:49:20Z

In my perception this is a clearer/easier structure 👍

herndlm · 2021-10-20T10:11:56Z

I'll wait until all type changes are merged before doing another rebase 😅

Seldaek · 2021-10-20T10:37:35Z

Yeah sorry about that :)

Introduces a `PlatformRequirementFilter` with methods that help to decide if a requirement is ignored or not as discussed in composer#10045 but without changing behaviour. The idea behind this is to be able to adapt simple wildcard or regex ignores in a follow-up.

herndlm · 2021-11-03T09:02:21Z

rebased and should be ready again for review

Seldaek

Looks like a great improvement to me, just the method name nitpick and then happy to finally merge this :)

src/Composer/Filter/PlatformRequirementFilter/PlatformRequirementFilterInterface.php

Seldaek · 2021-11-11T11:24:17Z

Thanks!

herndlm · 2021-11-11T11:46:24Z

Great thx, I'll rebase and make the wildcard-follow-up feature ready soon then :)

@herndlm

…trigger_error()` where none was needed Ref: composer/composer@3013674#diff-67d1dfefa9c7b1c7e0b04b07274628d812f82cd82fae635c0aeba643c02e8cd8R145 Ref: composer/composer#10079 Ref: #366 (comment) Ref: composer/composer#10079 (review) /cc @herndlm FYI

@herndlm

…trigger_error()` where none was needed Ref: composer/composer@3013674#diff-67d1dfefa9c7b1c7e0b04b07274628d812f82cd82fae635c0aeba643c02e8cd8R145 Ref: composer/composer#10079 Ref: Roave/BackwardCompatibilityCheck#366 (comment) Ref: composer/composer#10079 (review) /cc @herndlm FYI

herndlm force-pushed the feature/refactor-ignore-platform-reqs branch 8 times, most recently from a38e3bb to 602a1fa Compare August 27, 2021 07:29

herndlm marked this pull request as ready for review August 28, 2021 18:35

Seldaek requested changes Aug 29, 2021

View reviewed changes

src/Composer/Installer.php Show resolved Hide resolved

src/Composer/Package/Version/VersionSelector.php Show resolved Hide resolved

src/Composer/Autoload/AutoloadGenerator.php Show resolved Hide resolved

Seldaek added this to the 2.2 milestone Aug 29, 2021

Seldaek added the Feature label Aug 29, 2021

Seldaek mentioned this pull request Aug 29, 2021

Bump composer-plugin-api to 2.2 #10082

Closed

herndlm requested a review from Seldaek August 29, 2021 13:49

Seldaek approved these changes Aug 29, 2021

View reviewed changes

herndlm force-pushed the feature/refactor-ignore-platform-reqs branch from 1122a40 to 810a04f Compare August 29, 2021 17:38

herndlm mentioned this pull request Aug 29, 2021

Add wildcard support to ignore platform req #10083

Merged

herndlm force-pushed the feature/refactor-ignore-platform-reqs branch from 810a04f to 0c94018 Compare September 5, 2021 17:14

helhum approved these changes Sep 26, 2021

View reviewed changes

imme-emosol reviewed Oct 12, 2021

View reviewed changes

herndlm force-pushed the feature/refactor-ignore-platform-reqs branch from 0c94018 to eee0052 Compare October 13, 2021 08:00

herndlm requested a review from Seldaek October 14, 2021 09:22

herndlm force-pushed the feature/refactor-ignore-platform-reqs branch from 3b2d7d6 to 0c26e21 Compare October 18, 2021 08:52

herndlm force-pushed the feature/refactor-ignore-platform-reqs branch from 0c26e21 to 9459e67 Compare October 18, 2021 09:04

herndlm force-pushed the feature/refactor-ignore-platform-reqs branch from 9459e67 to 18514c7 Compare October 19, 2021 11:38

herndlm force-pushed the feature/refactor-ignore-platform-reqs branch 4 times, most recently from 0db7efd to 7ac1815 Compare November 3, 2021 08:54

herndlm added 2 commits November 3, 2021 09:57

fixup! Refactor ignore platform reqs checks

49bbad3

herndlm force-pushed the feature/refactor-ignore-platform-reqs branch from 7ac1815 to 49bbad3 Compare November 3, 2021 08:58

Seldaek requested changes Nov 10, 2021

View reviewed changes

src/Composer/Filter/PlatformRequirementFilter/PlatformRequirementFilterInterface.php Outdated Show resolved Hide resolved

Rename isReqIgnored to isIgnored

3416f59

herndlm requested a review from Seldaek November 11, 2021 08:45

Seldaek merged commit 3013674 into composer:main Nov 11, 2021

herndlm deleted the feature/refactor-ignore-platform-reqs branch November 11, 2021 11:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ignore platform reqs checks #10079

Refactor ignore platform reqs checks #10079

herndlm commented Aug 26, 2021 •

edited

Loading

herndlm commented Aug 29, 2021 •

edited

Loading

Seldaek left a comment

herndlm commented Aug 29, 2021 •

edited

Loading

Seldaek commented Aug 29, 2021

herndlm commented Aug 29, 2021

Seldaek commented Aug 29, 2021

herndlm commented Sep 5, 2021

helhum left a comment

imme-emosol left a comment

herndlm commented Oct 12, 2021 •

edited

Loading

imme-emosol commented Oct 12, 2021

herndlm commented Oct 13, 2021

herndlm commented Oct 13, 2021

imme-emosol commented Oct 13, 2021

herndlm commented Oct 18, 2021 •

edited

Loading

imme-emosol commented Oct 18, 2021

herndlm commented Oct 20, 2021

Seldaek commented Oct 20, 2021

herndlm commented Nov 3, 2021

Seldaek left a comment

Seldaek commented Nov 11, 2021

herndlm commented Nov 11, 2021

Refactor ignore platform reqs checks #10079

Refactor ignore platform reqs checks #10079

Conversation

herndlm commented Aug 26, 2021 • edited Loading

herndlm commented Aug 29, 2021 • edited Loading

Seldaek left a comment

Choose a reason for hiding this comment

herndlm commented Aug 29, 2021 • edited Loading

Seldaek commented Aug 29, 2021

herndlm commented Aug 29, 2021

Seldaek commented Aug 29, 2021

herndlm commented Sep 5, 2021

helhum left a comment

Choose a reason for hiding this comment

imme-emosol left a comment

Choose a reason for hiding this comment

herndlm commented Oct 12, 2021 • edited Loading

imme-emosol commented Oct 12, 2021

herndlm commented Oct 13, 2021

herndlm commented Oct 13, 2021

imme-emosol commented Oct 13, 2021

herndlm commented Oct 18, 2021 • edited Loading

imme-emosol commented Oct 18, 2021

herndlm commented Oct 20, 2021

Seldaek commented Oct 20, 2021

herndlm commented Nov 3, 2021

Seldaek left a comment

Choose a reason for hiding this comment

Seldaek commented Nov 11, 2021

herndlm commented Nov 11, 2021

herndlm commented Aug 26, 2021 •

edited

Loading

herndlm commented Aug 29, 2021 •

edited

Loading

herndlm commented Aug 29, 2021 •

edited

Loading

herndlm commented Oct 12, 2021 •

edited

Loading

herndlm commented Oct 18, 2021 •

edited

Loading