Skip to content

Conversation

@Tyrael
Copy link
Contributor

@Tyrael Tyrael commented Apr 2, 2015

No description provided.

Copy link
Member

Choose a reason for hiding this comment

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

maybe you should check wether PG(http_globals)[TRACK_VARS_ENV] is available before calling import_environment.. right?

@laruence
Copy link
Member

laruence commented Apr 3, 2015

or a normal way is to trigger global env's armor handler, then copy from PG(http_globals)[TRACK_VARS_ENV] as result..

@Tyrael
Copy link
Contributor Author

Tyrael commented Apr 3, 2015

for the record, the travis failure is the same(ext/pdo_mysql/tests/pdo_mysql_pconnect.phpt) that we also have in php-src master, so not related to this change.

@Tyrael
Copy link
Contributor Author

Tyrael commented Apr 3, 2015

@laruence I didn't want to use the http_globals as I didn't want to create any side effects from calling this function nor allow any caching.

if you call environ() then add/update an environment var with putenv(), then you should see the changes after you call environ() again.

@Tyrael
Copy link
Contributor Author

Tyrael commented Apr 5, 2015

pasting the discussion with @laruence from irc:
13:35:08 laruence | [12:42:25] Tyrael, in generally, I don't have objections on that.. I just could not see a usage.. actually, I think getenv is enough... and maybe more security...
13:38:16 Tyrael | laruence: you could use it for debugging (the environments set can affect your code or the underlying libraries, etc.) and also for proc_open you can specify the list of environment vars, but if you want to pass the current environment plus an additional env var, you either have to putenv() then proc_open without environment (as it will copy the current), or you can array_merge($_ENV, array('MY_ENV' => 'foobar'))
13:38:16 Tyrael | but if $_ENV is not populated you can't do that
13:39:48 Tyrael | and I can imagine that some people would use it to debug what environment vars did their code touch upon execution. (as environment changes via putenv() won't be reflected in $_ENV)
13:41:15 Tyrael | for the record I got this idea when I bumped into https://bugs.php.net/bug.php?id=60981 in 2012
13:42:58 Tyrael | run-tests.php uses $_ENV to fetch the current env vars, then sets/overwrites a couple of them based on the different environment vars or shell arguments passed
13:43:08 Tyrael | then passes it to proc_open
13:44:26 Tyrael | and I've seen 4 people bump into the issue that on ubuntu by default $_ENV is not populated because variables_order not containing E
13:45:11 Tyrael | which will cause the whole run-tests.php to not work if not called through make test (which many people do for the record) and not setting variables_order explicitly to contain E
13:53:38 laruence | Tyrael, fair enough, I think it's a good add now
13:54:00 laruence | but maybe still drop a mail to internal@

@bwoebi
Copy link
Member

bwoebi commented Oct 7, 2015

environ() is a weird abbreviation… Either env() or environment().

In general, I'd suggest to just alter getenv() to also accept just zero arguments and return the array in this case.

@jerrygrey
Copy link

@bwoebi's idea of getenv() accepting zero arguments is much simpler implementation imo, and will follow similar behaviour in other functions like error_reporting(), plus no need for yet another function.

@Tyrael
Copy link
Contributor Author

Tyrael commented Oct 7, 2015

@bwoebi was trying to follow suite: http://man7.org/linux/man-pages/man7/environ.7.html but yeah, it feels a bit weird. let's see what others think.

@weltling
Copy link
Contributor

weltling commented Oct 7, 2015

Well, PHP often mirrors C APIs and that is good. A function consisting on 3 lines is fine anyway, so far about simplicity :)

Thanks.

@bishopb
Copy link
Contributor

bishopb commented Oct 7, 2015

+1 on @bwoebi suggestion of changing getenv to accept no arguments and return array of all environment.

@jerrygrey
Copy link

@Tyrael I can see what you are trying to do, but I still think the zero arguments idea is much better though, but if environ() goes ahead, can the change to getenv() also be made? As it makes sense for that function to have that behaviour imo, especially since it isn't much of a change.

@marcioAlmada
Copy link
Contributor

I like the idea of getenv() without arguments returning an array.

@smalyshev
Copy link
Contributor

I agree with getenv() without parameters being a better idea.

@flaupretre
Copy link
Contributor

+1 for getenv([NULL])

@Tyrael
Copy link
Contributor Author

Tyrael commented Oct 12, 2015

closed in favor of #1567

@Tyrael Tyrael closed this Oct 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants