Support passing configuration arguments to CLI::run()

[martin-rueegg]

@theseer This is a request for comment!

I've noticed you have added the $env parameter for the run() method.

It is not clear to me, why you would pass a super-global (i.e. $_SERVER) as a parameter, unless the parameter could also be different. Now, in the current code, there is no other use. Has that been to allow external scripts to provide the environment?

Similar to the PR regarding exit codes, it would be awesome to have the possibility to pass the options as a parameter to run(), rather than have to mess with the $_SERVER['argv'] variable. That's why I had added the $options a while ago (unfortunately I forgot to create a PR).

I'm happy to the flip the $options and $env parameters. However, unless there is a known use-case, I find that the $env parameter us probably less likely to be overwritten, given that it's only used to evaluate the home directory.

What is your view on this? Would you see my PR as useful, as it is (but officially breaking backwards-compatibility) or shall I flip the parameters? Or do you have yet another idea?

PS: also speaking German, if that would be an issue at all! :-)

[theseer]

Sorry, I don't understand the motivation of this PR at all.

I'm passing in $_SERVER as I need some environment variables to be available during CLI::configure - in particular the HOME. As this is in the CLI calling wrapper script, it uses the array that is available. Could probably also have been $_ENV.

Anyway, your PR would break that. It creates a global dependency into CLI for no reason and takes away the option to arbitrarily pass in values. So, it would worsen things, wouldn't it?

Secondly, I don't get your point on arguments. The argument parsing is already available. I don't understand what you are trying to accomplish here?

Also, your code uses the short-array-syntax. That won't work on PHP 5.3.

[theseer]

Okay, I get it. The ezcConsoleInput could use that. But we do not need a new Parameter for that. We just need to pass it from the $env we already have.

[theseer]

Changed: https://github.com/theseer/Autoload/blob/master/src/CLI.php#L82

[martin-rueegg]

@theseer thank you for reviewing this PR and your comments!

Sorry, I don't understand the motivation of this PR at all.

I am using your library from another script. Hence, I'm not calling e.g. phpab.php -- some command options, but rather like so:

$factory = new \TheSeer\Autoload\Factory();
$rc = $factory->getCLI()->run(array('--', 'some', 'command', 'options'));

I'm passing in $_SERVER as I need some environment variables to be available during CLI::configure - in particular the HOME. As this is in the CLI calling wrapper script, it uses the array that is available. Could probably also have been $_ENV.

Yes, and this is still possible (as the second parameter to run(), though). And it is now optional and will default to $_SERVER in CLI::configure():

Autoload/src/CLI.php

Lines 274 to 276 in 02030e6

	if ($env === null) {
	$env = $_SERVER;
	}

That's why I have removed it in the calling scripts. However, I can also re-add them like so:

$rc = $factory->getCLI()->run(null, $_SERVER);

Anyway, your PR would break that. It creates a global dependency into CLI for no reason and takes away the option to arbitrarily pass in values. So, it would worsen things, wouldn't it?

I'm not quite sure what you mean by a "global dependency". But as laid out above, arbitrary values can still be passed.

Also, your code uses the short-array-syntax. That won't work on PHP 5.3.

Happy to fix those.

[martin-rueegg]

Changed: https://github.com/theseer/Autoload/blob/master/src/CLI.php#L82

I see.

Well, but that would mean that I need to do something like this, right?

$factory = new \TheSeer\Autoload\Factory();
$env = [
  'HOME' => $_SERVER['HOME'] ?? null,
  'HOMEDRIVE' => $_SERVER['HOMEDRIVE'] ?? null,
  'HOMEPATH' => $_SERVER['HOMEPATH'] ?? null,
  'argv' => ['--', 'some', 'command', 'options',]
]
$rc = $factory->getCLI()->run($env);

This is of course much more complicated, than just adding my options:

$factory = new \TheSeer\Autoload\Factory();
$rc = $factory->getCLI()->run(['--', 'some', 'command', 'options',]);

Also, I'm not sure that with your solution the $env['argv]-key needs to exist. I suspect the following would break the code:

$factory = new \TheSeer\Autoload\Factory();
$env = [
  'HOME' => $_SERVER['HOME'] ?? null,
]
$rc = $factory->getCLI()->run($env);

As ?: does not work on non-existent keys, does it?

[theseer]

I'm not quite sure what you mean by a "global dependency". But as laid out above, arbitrary values can still be passed.

The very concept of a "super globals" was a design mistake and should never have made it into PHP as it leaks global state. Which is bad. Hence me passing in the $_SERVER array. You are free to provide any type of array you feel like when using it. It should contain the required environment variables that can be expected in CLI context.

[martin-rueegg]

Furthermore I believe that you now pass an array (even just an empty one) to $input->process(). However, this would prevent \ezcConsoleInput::process() form actually using $_SERVER['argv'], as isset() will always return true.

[theseer]

That's correct and intended. I despise access to global state.

[martin-rueegg]

That's correct and intended. I despise access to global state.

I see. So you prevent doing it by doing it yourself. 😉

Anyway. Thanks for change. 🙏

[theseer]

Well, but that would mean that I need to do something like this, right?

$factory = new \TheSeer\Autoload\Factory();
$env = [
  'HOME' => $_SERVER['HOME'] ?? null,
  'HOMEDRIVE' => $_SERVER['HOMEDRIVE'] ?? null,
  'HOMEPATH' => $_SERVER['HOMEPATH'] ?? null,
  'argv' => ['--', 'some', 'command', 'options',]
];
$rc = $factory->getCLI()->run($env);

Yes and No.

$rc = $factory->getCLI()->run(array_merge($_SERVER, array('argv' => ....));

Would be enough if you're in CLI context yourself.
If not, you have to get the values from somewhere.

As ?: does not work on non-existent keys, does it?

Valid point. Fixed.

[theseer]

That's correct and intended. I despise access to global state.

I see. So you prevent doing it by doing it yourself. 😉

Not sure how you mean this. Of course one has to get the Information from somewhere, but the only time I access it, is in the global state of the bootstrap script. The actual application never sees the global state, as it should be.