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

Tests bootstrap - remove require InstalledVersions #11014

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

ondrejmirtes
Copy link
Contributor

This causes issues for PHPStan in the latest version. Because PHPStan itself loads its own InstalledVersions from inside the PHAR, this bootstrap line makes it crash.

If we find out that loading this class is indeed necessary for Composer's test suite to run, I'll create a separate PHPStan bootstrap next to this one.

@ondrejmirtes
Copy link
Contributor Author

Looks like the test suite is green even without this line, so I'd appreciate if this could be merged as-is. Thank you!

(The one failure is already present on main.)

@Seldaek
Copy link
Member

Seldaek commented Aug 20, 2022

IIRC I did add this because otherwise it got the one loaded from vendor and or from phpstan and I ended up not catching/reporting errors in the actual file. Any chance you can conditionally load yours only if it doesn't exist yet?

@Seldaek Seldaek added this to the 2.4 milestone Aug 20, 2022
@ondrejmirtes
Copy link
Contributor Author

Just changed it, thanks.

@Seldaek
Copy link
Member

Seldaek commented Aug 22, 2022

Sorry I meant can't you do that conditional loading in the PHPStan repo? :D I guess you should even delay it until the project bootstrap has been loaded.. But I can see this is maybe too painful to fix just this one use case.

@ondrejmirtes
Copy link
Contributor Author

I'm sorry, I can't, Symfony Console needs the version provided really early before running PHPStan itself: https://github.com/phpstan/phpstan-src/blob/62ac4d9e35dc8b9e9993c0f15ca7abaa8fdf9400/bin/phpstan#L157

@Seldaek
Copy link
Member

Seldaek commented Aug 22, 2022

Yeah this sucks, it's not the first time I see this application version need cause problems. You could easily hardcode it in the phar at build time tho.. But anyway maybe I should submit a PR to symfony/console to allow passing a version callback or something lazy..

@Seldaek Seldaek merged commit b602895 into composer:main Aug 22, 2022
@Seldaek
Copy link
Member

Seldaek commented Aug 22, 2022

Anyway merging this for now as workaround, thanks

@ondrejmirtes
Copy link
Contributor Author

I don't think it's worth to bother symfony/console with this :) We could read the JSON with the version directly, or just read the information from Git, if we come to another blocker because of this.

@ondrejmirtes
Copy link
Contributor Author

Thank you!

@ondrejmirtes ondrejmirtes deleted the patch-4 branch August 22, 2022 13:00
@Seldaek
Copy link
Member

Seldaek commented Aug 22, 2022

You could call setVersion with your ComposerHelper still, but after the bootstrap was executed? Or is that difficult?

@Seldaek
Copy link
Member

Seldaek commented Aug 22, 2022

Or overriding getLongVersion (and perhaps getVersion for completeness) in your own Application extending symfony's would also be an option to lazily evaluate this only when it's needed (which is only on --version I guess)

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.

None yet

2 participants