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

Remove jean85/pretty-package-versions dependency #1607

Merged

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Aug 10, 2022

in favor of direct usage of composer/package-versions-deprecated it was using internally already. See also https://github.com/Jean85/pretty-package-versions/blob/1.6.0/src/PrettyVersions.php#L13 and https://github.com/Jean85/pretty-package-versions/blob/1.6.0/src/Version.php#L25

the even nicer alternative would be to bump the composer API to v2 and use it's features as mentioned by @staabm phpstan/phpstan#7768 (comment)

this is the precondition to update paratest without having to care about jean85/pretty-package-versions which is not compatible with PHP 7 in version 2.

I could somewhat test this by running e.g. bin/phpstan --version and bin/phpstan analyse --fix and partly dumping the version and modifying the package it looks for (because for local phpstan it will always be an empty string as it was before as well).

@herndlm herndlm marked this pull request as ready for review August 10, 2022 10:42
@herndlm
Copy link
Contributor Author

herndlm commented Aug 10, 2022

I grabbed the phar artifact and could verify this which is good I guess

php phpstan.phar --version
PHPStan - PHP Static Analysis Tool 1.8.x-dev

@ondrejmirtes
Copy link
Member

I tried this before and it's not good enough. In current 1.8.x-dev the version says: PHPStan - PHP Static Analysis Tool 1.8.x-dev@bee8f24

So it includes the commit. So I guess the "pretty" part still does something extra besides calling Composer API.

@ondrejmirtes
Copy link
Member

However, we can always copy the few lines of code to make this work without any dependencies, so feel free to get inspired :)

@herndlm
Copy link
Contributor Author

herndlm commented Aug 10, 2022

InstalledVersions::getPrettyVersion('phpstan/phpstan') seems to be doing the trick, I see no commit there.
Did I get you right - should I try that out and use the Composer 2 API instead?
UPDATE: well, looks like I broke something already hehe. but it is related to the phar creation / scoper config or smth like that.
UPDATE2: I rebased it away again, also looks like psalm had problems too and after fixing them it was failing otherwise still vimeo/psalm#7329

ah and now I got you I think. yeah, the pretty part checks if a tagged version is used or not and still shows the commit then or not. I'll check this out.

@herndlm herndlm force-pushed the remove-jean85-pretty-package-versions-dep branch 3 times, most recently from 1ebd2c3 to de2e361 Compare August 10, 2022 21:23
@herndlm herndlm marked this pull request as draft August 10, 2022 21:26
composer.json Outdated Show resolved Hide resolved
@herndlm herndlm force-pushed the remove-jean85-pretty-package-versions-dep branch from c72a8bb to e38fcd9 Compare August 15, 2022 19:09
@herndlm
Copy link
Contributor Author

herndlm commented Aug 15, 2022

hmm this was looking promising, but php phpstan.phar --version shows Version unknown :/

@herndlm
Copy link
Contributor Author

herndlm commented Aug 15, 2022

I think this is looking good now, even better than before this PR since it also works on my local machine :) I hope it also still works on tags 😅
e.g. the phar gives me

php ~/Downloads/phpstan.phar --version
PHPStan - PHP Static Analysis Tool 1.8.x-dev@094b3ac

@herndlm herndlm marked this pull request as ready for review August 15, 2022 21:29
@ondrejmirtes ondrejmirtes force-pushed the remove-jean85-pretty-package-versions-dep branch from e14ed24 to 8350e9c Compare August 17, 2022 11:41
@ondrejmirtes ondrejmirtes merged commit 087141e into phpstan:1.8.x Aug 17, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@herndlm herndlm deleted the remove-jean85-pretty-package-versions-dep branch August 17, 2022 17:57
@herndlm
Copy link
Contributor Author

herndlm commented Aug 17, 2022

hmm weird, did you change the lockfile for a specific reason? there are now some packages still inside that are not required by anything.
but they'd be removed with the next change I guess

@ondrejmirtes
Copy link
Member

I had to change it to rebase the branch. I probably ran the wrong command instead of what you did.

IIRC this was just a prerequisite to be able to update paratest?

@herndlm
Copy link
Contributor Author

herndlm commented Aug 17, 2022

yep, it's not a problem. I'll just open a PR to update paratest and there e.g. composer/package-versions-deprecated will be removed from the lockfile. the new paratest brings back jean85/pretty-package-versions in again funnily but in v2 and as dev dep of course

@herndlm
Copy link
Contributor Author

herndlm commented Aug 18, 2022

Oh no, I found another thing. I was completely ignoring the composer integration test already because it's red and missed that this PR most likely made it worse:

PHP Fatal error:  Cannot declare class Composer\InstalledVersions, because the name is already in use in /home/runner/work/phpstan-src/phpstan-src/e2e/integration/repo/src/Composer/InstalledVersions.php on line 25
Fatal error: Cannot declare class Composer\InstalledVersions, because the name is already in use in /home/runner/work/phpstan-src/phpstan-src/e2e/integration/repo/src/Composer/InstalledVersions.php on line 25
Error: Process completed with exit code 255.

@ondrejmirtes
Copy link
Member

See composer/composer#11014

@herndlm
Copy link
Contributor Author

herndlm commented Aug 20, 2022

Nice, I didn't have time to debug it yet unfortunately

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

Successfully merging this pull request may close these issues.

3 participants