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

Refactor getenv access using constructor #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bcremer
Copy link
Contributor

@bcremer bcremer commented Oct 24, 2014

Another alternative to PR #6 and #7 including the idea of @bobthecow.

@bcremer bcremer changed the title Refactor getenv access Refactor getenv access using constructor Oct 24, 2014
public function __construct(array $env = [])
{
foreach (self::$envKeys as $key) {
$this->env[$key] = array_key_exists($key, $env) ? $env[$key] : getenv($key);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Possibly add an explicit string cast to $env[$key] to mirror the values we'll be getting back from getenv?

Copy link
Collaborator

Choose a reason for hiding this comment

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

$this->env is never declared, so it's a public property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@bobthecow
Copy link
Collaborator

👍 (of course, I'm probably biased)

@bcremer bcremer force-pushed the refactor-environment-access-alternative branch from 2fdc5f5 to 84deaaf Compare October 24, 2014 07:11
@bcremer
Copy link
Contributor Author

bcremer commented Oct 24, 2014

Included CR feedback from @bobthecow

@GrahamCampbell
Copy link
Contributor

Any news on this @dnoegel, @bobthecow, @bcremer?

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.

3 participants