Skip to content

Set the environment of the event to production if it has not been overridden explicitly - #1116

Merged
ste93cry merged 5 commits into
getsentry:developfrom
ste93cry:feature/environment-field-always-required
Feb 22, 2021
Merged

ste93cry merged 5 commits into
getsentry:developfrom
ste93cry:feature/environment-field-always-required

Conversation

@ste93cry

Copy link
Copy Markdown
Contributor

According to getsentry/develop#169 the environment field is now always required. Since we have to maintain the compatibility we cannot just make the property non-nullable, so I added our very first deprecation of the 3.x series that will suggest users to always pass a string. I also changed the default of the option as per documentation

@stayallive stayallive left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@stayallive stayallive left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Soooo actually there is some discussion about the actual value maybe needing to be production instead of default, see discussion in #php on discord. Let's hold of a little bit before merging.

@stayallive stayallive left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that was quick: getsentry/develop@676ab2b

We should change it to use production not default by default and allow the user to pass null if they want to (https://discordapp.com/channels/621778831602221064/621964286952210432/766052439660560384):

server is not going to enforce a default and a user can explicitly set to null if they really want to (though that's probably not a super great idea)

I guess that postpones our first deprecation for a bit 😉

We should also not default to that value in the options but only when creating the event or preparing it for sending (whatever makes more sense) and leave it null in the options itself because: https://discordapp.com/channels/621778831602221064/621964286952210432/766053912247271484.

a reason not to set it by default in the options object is that later on a framework you wouldn't be able to tell if the user set something explicitly programatically and you'd be overriding in the integration. So maybe best is to leave this in the client when preparing the event

Although those 2 conflict a bit in a sense, but considering we should always try to send one I would go for null in options and default to production in the Event when the environment is null.

@ste93cry ste93cry modified the milestones: 3.1, 3.2 Dec 1, 2020
Comment thread src/Serializer/PayloadSerializer.php
@ste93cry ste93cry changed the title Make the environment always required Set the environment of the event to production if it has not been overridden explicitly Feb 22, 2021
@ste93cry
ste93cry merged commit f251023 into getsentry:develop Feb 22, 2021
@ste93cry
ste93cry deleted the feature/environment-field-always-required branch February 22, 2021 19:59
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.

3 participants