Skip to content

Composer v2 compatibility #107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Composer v2 compatibility #107

wants to merge 1 commit into from

Conversation

thojou
Copy link
Contributor

@thojou thojou commented Nov 9, 2020

Hi,

Today I encountered an issue with composer v2, which has been already reported in #106.
Here is the PR that should fix this issue.

I'm not sure, how to handle the new introduced installed-path inside a package definition. May that be a replacement for target-dir. If you think so, I can update this part as well.

What is your thought on the new PHPUnit-Test? Currently, it is duplicated code from testBundlePackageWithNoVendorReturnsEmptyBundle. Should we use a dataProvider instead?

@PurHur PurHur mentioned this pull request Nov 11, 2020
Closed
nargotik
nargotik suggested changes Nov 11, 2020
View reviewed changes
Copy link

@nargotik nargotik left a comment

Choose a reason for hiding this comment

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

The solution could be only this line, not needing to pass an entire array to another method verify the same array.

$installed = empty($installed['packages']) ? $installed : $installed['packages'];

@soyuka
Copy link

soyuka commented Nov 20, 2020 •
edited
Loading

change works 👍

$installed = $installed['packages'] ?? $installed;

@nargotik
Copy link

change works +1

$installed = $installed['packages'] ?? $installed;

Yes but this would make incompatible with php5.3 i think

@wimleers
Copy link

wimleers commented Dec 3, 2020

Yes but this would make incompatible with php5.3 i think

Which is probably why most builds are failing.

Could the PR be updated to use syntax that is more backwards compatible? 🙏 😊

@nargotik nargotik mentioned this pull request Dec 3, 2020
Closed
@nargotik
Copy link

nargotik commented Dec 3, 2020

@wimleers the PR is already backwards even using this aproach or aproach on #108 that I have already closed due duplication of issues solved.

@clue clue mentioned this pull request Dec 11, 2020
Merged
@clue
Copy link
Owner

clue commented Dec 11, 2020

@thojou Thanks for looking into this and providing this high-quality PR with some additional tests!

I've just applied your changes with #114 after introducing some additional test changes now that #112 is in.

Now let's get this shipped! :shipit:

Keep it up!

@clue clue closed this Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@nargotik nargotik nargotik requested changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thojou @soyuka @nargotik @wimleers @clue