Skip to content

Remove jean85/pretty-package-versions dependency#1607

Merged
ondrejmirtes merged 1 commit intophpstan:1.8.xfrom
herndlm:remove-jean85-pretty-package-versions-dep
Aug 17, 2022
Merged

Remove jean85/pretty-package-versions dependency#1607
ondrejmirtes merged 1 commit intophpstan:1.8.xfrom
herndlm:remove-jean85-pretty-package-versions-dep

Conversation

@herndlm
Copy link
Copy Markdown
Contributor

@herndlm herndlm commented Aug 10, 2022 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Member

ondrejmirtes commented Aug 10, 2022

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
Copy Markdown
Member

ondrejmirtes commented Aug 10, 2022

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
Copy Markdown
Contributor Author

herndlm commented Aug 10, 2022 •
edited
Loading

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
ondrejmirtes
ondrejmirtes requested changes Aug 10, 2022
View reviewed changes
@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
Copy Markdown
Contributor Author

herndlm commented Aug 15, 2022

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

@herndlm
Copy link
Copy Markdown
Contributor Author

herndlm commented Aug 15, 2022 •
edited
Loading

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
@herndlm herndlm requested a review from ondrejmirtes August 16, 2022 06:51
@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
Copy Markdown
Member

ondrejmirtes commented Aug 17, 2022

Thank you!

@herndlm herndlm deleted the remove-jean85-pretty-package-versions-dep branch August 17, 2022 17:57
@herndlm
Copy link
Copy Markdown
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
Copy Markdown
Member

ondrejmirtes commented Aug 17, 2022

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

ondrejmirtes commented Aug 20, 2022

See composer/composer#11014

@herndlm
Copy link
Copy Markdown
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

Reviewers

@staabm staabm staabm left review comments

@ondrejmirtes ondrejmirtes ondrejmirtes approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@herndlm @ondrejmirtes @staabm