Skip to content

Commit

Permalink
Patching around yet another nonsensical BC break caused by abuse of `…
Browse files Browse the repository at this point in the history 
…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
  • Loading branch information
@Ocramius
Ocramius committed Dec 26, 2021
1 parent 0a5bbd4 commit 17850cd
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 5 deletions.
3 changes: 2 additions & 1 deletion src/LocateDependencies/LocateDependenciesViaComposer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

namespace Roave\BackwardCompatibility\LocateDependencies;

use Composer\Filter\PlatformRequirementFilter\IgnoreAllPlatformRequirementFilter;
use Composer\Installer;
use Psl;
use Psl\Env;
Expand Down Expand Up @@ -49,7 +50,7 @@ public function __invoke(string $installationPath, bool $includeDevelopmentDepen
* of composer, as we otherwise need to re-design how an {@see Installer} is constructed.
*/
$installer->setRunScripts(false);
$installer->setIgnorePlatformRequirements(true);
$installer->setPlatformRequirementFilter(new IgnoreAllPlatformRequirementFilter());

$installer->run();
}, $installationPath);
Expand Down
6 changes: 2 additions & 4 deletions test/unit/LocateDependencies/LocateDependenciesViaComposerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,7 @@ public function testWillLocateDependencies(): void
$this
->composerInstaller
->expects(self::atLeastOnce())
->method('setIgnorePlatformRequirements')
->with(true);
->method('setPlatformRequirementFilter');

$this
->composerInstaller
Expand Down Expand Up @@ -143,8 +142,7 @@ public function testDevelopmentDependenciesCanBeOptionallyInstalled(): void
$this
->composerInstaller
->expects(self::atLeastOnce())
->method('setIgnorePlatformRequirements')
->with(true);
->method('setPlatformRequirementFilter');

$this
->composerInstaller
Expand Down

3 comments on commit 17850cd

@herndlm
Copy link

@herndlm herndlm commented on 17850cd Dec 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Ocramius,

thx for letting me know. I'm sorry that this caused you troubles and work. I really am.

Tbh. it's hard to not take that a bit personally after reading your comments, I don't feel offended or anything whatsoever though, so no worries.

I'm missing experience in lib maintenance and BC for sure, just kept to the best practices in the Composer code base / environment. So I'm not going to argue, take sides or start a big discussion. Just so that I can take something away from here though and learn something - after seeing #369 I think I understand your arguments but how would you have handled this in Composer? Just with the deprecated phpdoc so that e.g. tooling can check that as well?

Also - do you think I can do something else to help avoid something like this happening again? E.g. start a discussion over at Composer or open a PR there?

I have the feeling that the deprecation error method is the Symfonish way of handling BC and with phpunit removing the deprecation - exception transformation recently I thought that it even strengthened this approach more. Maybe it makes sense to raise this problem in the community more? Or if that has been done already and I just didn't notice - sorry :)

Sorry for the wall of text also. Wish you nice holidays too and don't do too much OSS work! :)

@Ocramius
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx for letting me know. I'm sorry that this caused you troubles and work. I really am.

Tbh. it's hard to not take that a bit personally after reading your comments, I don't feel offended or anything whatsoever though, so no worries.

@herndlm don't take this as scolding: my intent was mostly to let you know the effects of trigger_error(), hence the "FYI"

Just with the deprecated phpdoc so that e.g. tooling can check that as well?

Correct 👍

Also - do you think I can do something else to help avoid something like this happening again? E.g. start a discussion over at Composer or open a PR there?

I gave up on discussing it with the symfony/*-oriented ecosystem: the amount of regular pain caused by this approach seems to be completely ignored over there 🤷‍♀️

I suggest taking it easy and thinking about it in a few days, of if it occurs again. For this library, I've raised #369, with which I'm going to raise the issue from this end.

@herndlm
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the explanations Marco, appreciate it :)

Please sign in to comment.