Skip to content
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.

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

Deprecate InstalledVersions::getRawData #39

Merged
merged 7 commits into from
May 26, 2021

Conversation

BramRoets
Copy link
Contributor

Composer released an update that deprecates the getRawData() method.

This PR uses the new recommended getAllRawData() method.

This package is used by https://github.com/getsentry/sentry-php. After a recent deployment, Sentry stopped working because of the change made by composer. I hope to get this PR merged and then update the requirement in Sentry.

Related commit in Composer: https://github.com/composer/composer/pull/9816/files

Copy link
Owner

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Thanks for the heads up, it's nice to tackle this head on.

I'm sorry but this PR is not feasible in this form. getAllRawData is present only since the commit you listed, and hence it would break under any Composer between 2.0.0 and 2.0.13.

The only way around this that I see is to check for method existence.

@Jean85 Jean85 added this to the 2.0 milestone May 24, 2021
@BramRoets
Copy link
Contributor Author

@Jean85 Thanks, that makes sense! Let me know if it needs any additional changes.

Copy link
Owner

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

The code looks fine, thanks!

I was going to propose an optimization, but I just only noticed that you're dealing with N possible entries with this change, so no optimization is doable...

I would like to cover this with tests but I don't think it will be doable :( Damn

@Jean85
Copy link
Owner

Jean85 commented May 24, 2021

I've approved the CI, it's complaining because it doesn't recognize the method... I don't understand why, probably composer in CI is older than the requested patch?

Maybe we can downgrade Composer in the --prefer-lowest job to cover both cases?

@BramRoets
Copy link
Contributor Author

I've added some additional steps to the tests. Not sure if this will fix them. Can you approve the GitHub Action?

@BramRoets BramRoets requested a review from Jean85 May 25, 2021 06:27
@Jean85
Copy link
Owner

Jean85 commented May 25, 2021

@BramRoets I reworked the CI flow, since it had complications from when 8.0 wasn't stable. I also found a way to solve the Composer version constraint using setup-php.

I'll also check the PHPStan issue.

Copy link
Owner

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

PHPStan doesn't budge, I don't know why it doesn't recognize the presence of that method, neither the existence check.

Please put that in the baseline to fix the CI, and add a changelog entry for this PR, then it's good to merge.

@BramRoets
Copy link
Contributor Author

Hi Jean, I'm not sure. Do you need any input from my side?

@umbarov
Copy link

umbarov commented May 26, 2021

Hi,
will this PR be merged? Composer broke half of the web with it's deprecation. Do you need some help? And thank you for your work 👍

@Jean85
Copy link
Owner

Jean85 commented May 26, 2021

I had to manually merge this since I couldn't push this from local, don't know why. Thanks anyway @BramRoets!

This has been released as 2.0.4: https://github.com/Jean85/pretty-package-versions/releases/tag/2.0.4

@filakhtov
Copy link

Thanks a lot for quick turnaround 🙇🏻‍♂️ What are the chances that this will be backported to 1.6.x branch?

@Jean85
Copy link
Owner

Jean85 commented May 26, 2021

@filakhtov this issue does not affect 1.6 because I do not use the Composer API directly there, but I use composer/package-versions-deprecated there. So in that case the issue is fixed by composer/package-versions-deprecated#25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants