Page MenuHomePhabricator

Build $_ENV from CLI scripts
Needs RevisionPublic

Authored by epriestley on Jul 15 2014, 9:06 PM.
Tags
None
Referenced Files
F14020581: D9938.diff
Wed, Nov 6, 12:51 AM
Unknown Object (File)
Wed, Oct 9, 4:23 PM
Unknown Object (File)
Sep 28 2024, 5:35 PM
Unknown Object (File)
Sep 6 2024, 11:51 AM
Unknown Object (File)
Sep 6 2024, 11:51 AM
Unknown Object (File)
Sep 5 2024, 10:40 PM
Unknown Object (File)
Sep 2 2024, 10:45 AM
Unknown Object (File)
Aug 29 2024, 5:34 PM
Subscribers

Details

Summary

Ref T5554. Currently, when a system is in a non-English language, some things sometimes do not work because we parse output for English error messages but the messages are actually in French (or whatever).

To remedy this, D9922 attempts to pass LC_ALL or LANG or some mixture of things explicitly (I haven't figured out exactly what we have to pass, yet). However, a side effect of doing this is that we wipe out the rest of the environment, because $_ENV is not (always? ever?) populated from the CLI.

Since $_ENV is empty, we end up generating a subprocess with only the overridden variables when building the subprocess enviroment with $overrides + $_ENV. For example, some of my recent commits have a bogus author string because HOME (I think?) gets wiped, so git doesn't know where to look for ~/.gitconfig.

Instead, make sure $_ENV is always populated. The only way I could see to do this is capture phpinfo(), which is very gross, but should work OK.

Test Plan

Added a var_dump($_ENV) and saw it populated where previously it was not populated.

Diff Detail

Repository
rPHU libphutil
Branch
buildenv
Lint
Lint Errors
SeverityLocationCodeMessage
Errorscripts/__init_script__.php:106XHP45PHP Compatibility
Unit
No Test Coverage
Build Status
Buildable 1701
Build 1702: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley retitled this revision from to Build $_ENV from CLI scripts.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: joshuaspence, btrahan.
btrahan edited edge metadata.
btrahan added inline comments.
scripts/__init_script__.php
75

so no environment variable can be false eh? (Seems like the right code looking at getenv; just checking. :D )

This revision is now accepted and ready to land.Jul 17 2014, 10:26 PM
scripts/__init_script__.php
75

Yeah -- as far as I know, they always have to be strings.

joshuaspence edited edge metadata.

I'm not convinced that this is a good idea. FWIW, I was looking at the PHP document on $_ENV and found this comment.

I haven't got a better idea at the moment, but wanted to raise my concerns before this gets landed.

This revision now requires changes to proceed.Jul 20 2014, 9:50 PM

I'm not sure that this would work in all cases, but (on my machine) $_ENV is empty whereas $_SERVER contains all of my environment variables.

Another possibility here is to whitelist a list of environment variables that we do care about, and retrieve them via getenv():

<?php

$env = $_ENV;

if (!$env) {
  // PHP `variables_order` does not contain "E"
  $env_vars = array(
    'HOME',
    ...
  );

  foreach ($env_vars as $env_var) {
    $env[$env_var] = getenv($env_var);
  }
}

For getenv() to work, php.ini variables_order must contain 'E'.

This doesn't hold on my system:

$ php -r 'echo ini_get("variables_order")."\n"; echo getenv("HOME")."\n";'
GPCS
/Users/epriestley

I think the author may be confused between $_ENV (which will not be populated if "E" is missing) and getenv() (which, AFAIK, does not care).

For getenv() to work, php.ini variables_order must contain 'E'.

This doesn't hold on my system:

$ php -r 'echo ini_get("variables_order")."\n"; echo getenv("HOME")."\n";'
GPCS
/Users/epriestley

I think the author may be confused between $_ENV (which will not be populated if "E" is missing) and getenv() (which, AFAIK, does not care).

Yeah, I agree.