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

Cleanup the PHP version #604

Merged
merged 2 commits into from
Aug 3, 2018
Merged

Cleanup the PHP version #604

merged 2 commits into from
Aug 3, 2018

Conversation

stayallive
Copy link
Collaborator

@stayallive stayallive commented May 24, 2018

As discussed with the SDK team (Slack) we might want to not include the extra part of the PHP version to cleanup the Sentry interface.

image

(working on the unknown browser)

@stayallive stayallive requested a review from Jean85 May 24, 2018 08:48
Copy link
Collaborator

@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.

LGTM 👍

@ste93cry
Copy link
Collaborator

The same thing should be done for the 2.0 branch

@stayallive
Copy link
Collaborator Author

Yes, however this does remove the alpha-1 if that's being used...

If someone knows an easy way around it I am all ears but otherwise for the sake of simplicity I think we should go for this (you can always override this in your own rumtime context if you really wanted). Because those Ubuntu extra's are terrible.

@ste93cry
Copy link
Collaborator

It should not remove alpha-1 as it's part of the version string and should stay there

@stayallive
Copy link
Collaborator Author

Yes. But. As you can see in the screenshot that also includes other possible jibberish, I would like to keep that as clean as possible.

@ste93cry
Copy link
Collaborator

Maybe I misunderstood, but I thought the string alpha-1 was an example of text present after the PHP version (e.g. RC version which would get reported as 7.2.6RC1) and before the gibberish of the OS build. In that case it's correct to report PHP 7.2.6RC1 as runtime version, but I agree that the rest should be removed (-1+ubuntu14.04.1+deb.sury.org+1 in the case of the screenshot you uploaded). Unfortunately using the constants PHP_*_VERSION is not enough as we lose the stability part of the release (alpha, beta, etc), so I think that the only solution to the problem would be to parse out from the PHP_VERSION constant the portion we need

@stayallive
Copy link
Collaborator Author

Yeah I also, I thought the "extra" version was always behind a "-" however for RC's that's not the case (7.2.0RC6) but dev (7.2.6-dev) is...

PHP-CS-Fixer/PHP-CS-Fixer#2942 (comment)

This is a bit of a deep hole by the looks of it. We could take that snippet ^ and modify it for the nice version.

@nokitakaze
Copy link
Contributor

Are you sure that 7.0.0 and 7.0.0-RC2 are the same versions?

@stayallive stayallive force-pushed the better-runtime-context branch from 500b429 to 3864c53 Compare August 3, 2018 08:38
@stayallive
Copy link
Collaborator Author

@Jean85 I added some processing from the snippet in PHP-CS-Fixer/PHP-CS-Fixer#2942 (comment) so we have the beta/rc version appended to the version. Still okay with it now?

Copy link
Collaborator

@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.

Yep, that seems perfect! 👍
We need to port this to 2.0 though!

@stayallive stayallive merged commit 58172bd into master Aug 3, 2018
@stayallive stayallive deleted the better-runtime-context branch August 3, 2018 09:31
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.

4 participants