Remove unnecessary methods from trait, adds Composer 2.3 compatibility

[Seldaek]

I believe this should fix things reported in #18

[GuySartorelli]

Tha is for the contribution @Seldaek. We can't remove these methods without making it a major release since RecipeCommandBehaviour is part of the public API. It's unlikely to be used in other projects but we can't rule it out.

[dhensby]

@GuySartorelli - as @Seldaek points out in #18, these methods exist on the base command, so removing them from the trait shouldn't be too big a deal? (unless the signatures are different, I suppose)

But I guess if we need to release a major version to support 2.3, then so be it?

[GuySartorelli]

The BC concern is if someone else is using the trait for some class that doesn't extend BaseCommand. Unlikely, but something that bears considering and pointing out as a possibility. Though I guess since these declarations are abstract, they'll have some concrete implementation in their class anyway, so maybe that doesn't matter? 🤔

[dhensby]

Yep, if they are abstract they will have had to implement them to get them working now.

In terms of major release, what are the blockers to v2 being tagged?

---

edit: to answer my own question:

basically nothing (there are no divergences in master: https://github.com/silverstripe/recipe-plugin/compare/master

[Seldaek]

Though I guess since these declarations are abstract, they'll have some concrete implementation in their class anyway, so maybe that doesn't matter?

Yeah IMO this seems really fine to do even as a minor release, but up to you.

[GuySartorelli]

I'll ask Max or Steve to review this since there are a few additional considerations here.
Things to consider:

• This is, in the strictest sense, a breaking change. If someone has an abstract class that uses the trait, and they're relying on the trait's abstract method declarations to ensure any concrete implementations have implemented those methods, then someone could implement the class later without implementing those methods.
• The likelyhood of the above scenario is extremely remote, given where the trait is and what it does.
• If we want to adhere strictly to semver, this should be a major release
  • This would mean updating constraints for this package anywhere it's used (installer, core recipes, most projects, third party recipes)
  • We would need to have very clear comms to ensure the community know about the change and know to update their constraints where necessary.
• If we are okay with introducing this change in a minor or patch release, then I think it's good to go as is.
• If we want to adhere strictly to semver but are uncomfortable releasing a major version at this time, we could do what was done to allow SaphireTest to work with two different versions of PHPunit.
  • The Composer\PartialComposer class was introduced in composer 2.3.0 and can be used for feature detection - we could then declare the trait as is if the new class doesn't exist (composer < 2.3.0), and add the typehints if the class does exist (composer >= 2.3.0).
  • There may also be other solutions available.

[emteknetnz]

I think pull-request is correct, it's removing code that should never have been there in the first place

All these methods exists in composer/composer BaseCommand.php. We only use this trait on 2 classes (RequireRecipeCommand, UpdateRecipeCommand), that both extend BaseCommand.

Blockers for releasing as a major are real world implications. There's several modules/recipes that require "silverstripe/recipe-plugin": "^1", and if we don't release this as a minor, then we're making it difficult for those users to receive this patch, so if they upgrade to composer 2.3.0+ now they're going to be in trouble. Yes we could update all the deps for the upcoming release to "^1 || ^2", though what happens when we need to backport a critical security patch to say framework 4.9?

I think semver is worth bending a little here. The code removed here was essentially defining an interface for an upstream dependency, which was just weird. We don't release new majors when other upstream dependencies change their APIs, so I don't think we should here either. This should be released as a minor IMO.

[maxime-rainville]

I tested this PR locally. It fixes the error when using composer 2.3.2. It also doesn't break anything when using composer 2.2.6 or 1.10.25. I'll try doing a deployment to our platform to make sure things work there as well. Presuming that works well, I'm inclined to merge this PR and tag a release to ship it.

[maxime-rainville]

Confirmed this fixes the issue and doesn't break anything anywhere else.

I'll merge this and tag version 1.6.0