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

Prefer the SENTRY_RELEASE environment variable over the package root version #753

Merged

Conversation

ste93cry
Copy link
Collaborator

@ste93cry ste93cry commented Aug 19, 2023

Fixes #623. Since the default value for the release option is hardcoded to be the root package version, the SENTRY_RELEASE environment variable is never considered unless explicitly referenced in the config by the user. With these changes, the environment variable is used as default value. If its value is an empty string or the variable is not set, the root package version is used as a fallback.

This is technically speaking a change in the behaviour of the config, so it may be worth to document it. However, this may be considered a bugfix rather than an improvement since it was never intended to prefer the root package version over the environment variable.

@ste93cry ste93cry force-pushed the fix-release-option-set-precedence-order branch from 0f391d3 to 5ea96fe Compare August 19, 2023 22:17
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.

This env behavior was already documented like this, so I would consider it as a bugfix. But, this wasn't working because the bundle is overriding the default SDK behavior here: https://github.com/getsentry/sentry-php/blob/95d3d3a518010299fc928609ddb05f0067f0b6ac/src/Options.php#L906C1-L906C61

IMHO we should just reverse the priority, and it seems that this PR is doing that... I have a doubt though...

src/DependencyInjection/SentryExtension.php Outdated Show resolved Hide resolved
@cleptric cleptric merged commit beb3ca0 into getsentry:master Sep 5, 2023
28 checks passed
@ste93cry ste93cry deleted the fix-release-option-set-precedence-order branch September 5, 2023 11:20
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.

Precedence order when setting app release
3 participants