Skip to content

Conversation

Brugman
Copy link

@Brugman Brugman commented Jun 5, 2022

Hiya. I'm a new customer. You've got a few PRs incoming based on my first impressions. If these are welcome I'd love to write more.

Description of the change

In my experience WP_ENV is more often used as a constant than an environment variable. I don't want to get rid of the environment variable, but if it doesn't exist can we check the constant?

Fixes #68

Type of change

  • New feature (non-breaking change that adds functionality)

Related issues

None.

Checklists

I'm not yet familiar with testing, I did test manually of course, and I'm on Windows. I hope you'll take this without testing, or can run tests on my behalf.

@Brugman
Copy link
Author

Brugman commented Jun 5, 2022

There's a bunch of duplicate code in this PR. I couldn't find any example of reusable code in the plugin that I could mimic so I did it like this on purpose. Feel free to modify it.

@danielmorell danielmorell changed the base branch from master to next/2.6.4 June 6, 2022 20:02
Copy link
Collaborator

@danielmorell danielmorell left a comment

Choose a reason for hiding this comment

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

You could probably make Plugin::getEnv() a public static function that is called by both Defaults::environment() and UI::environmentSettingNote(). This would keep us from introducing bugs later by changing only one instance of our WP_ENV lookup.

@Brugman
Copy link
Author

Brugman commented Jun 7, 2022

A public static function it will be.

Renamed the method to getEnvironment() to avoid confusion/conflict with PHP's native getenv().

Manually tested, all 3 locations work well.

@Brugman Brugman requested a review from danielmorell June 7, 2022 02:10
Copy link
Collaborator

@danielmorell danielmorell left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you!

@danielmorell danielmorell merged commit 1d592cc into rollbar:next/2.6.4 Jun 7, 2022
@danielmorell danielmorell added this to the v2.6.4 milestone Jun 8, 2022
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.

WP_ENV is not used

2 participants