ArgumentException should not be sent immediately

[Arcesilas]

Throwing exceptions immediately when an argument is unexpected or missing when processing arguments list prevent from using valuable options like --help or --verbose, as they have not been processed and therefore are not available when handling the exception.

To observe, just take the example file provided in the documentation and just make this change:

    try {
        $getOpt->process();
    } catch (unexpected | Missing $exception) {
        // catch missing exceptions if help is requested
        if (!$getOpt->getOption('help')) {
            throw $exception;
        } else {
            echo 'Help option provided'.PHP_EOL;
        }
    }

Now call the script with: php example.php -p --help
The exception will always be thrown and the text Help option provided will not be displayed, despite we provided the help option.

Note that this does not happen if we pass the help option first: php example.php --help -p

To prevent this from happening, exceptions should be stored somewhere (Symfony uses an ExceptionBag if I'm correct). After processing the arguments list is done, the exceptions can then be checked and handled appropriately. This allows to use options to adjust error handling (verbose mode for example).

I may provide with some PR if you're interested, to have something concrete to discuss.

[tflori]

interesting idea. but we would need to define somehow what happens when an option is unknown and what else could someone want to do when an option is unknown except showing the help?

[Arcesilas]

What about having an ArgumentExceptionHandler that user could extend or replace? If none is defined when time comes to handle the ExceptionBag (or whatever we call it or use), then it uses a default one which shows the help text.

Or use a callback (a Closure or whatever the user wants to use) which receives the ExceptionBag as argument. If none, show help.

[Arcesilas]

Actually, there is no need to handle process errors: by default, exceptions are thrown, so that there is no BC break. If the user decides to not throw errors (I'm adding a $throwExceptions = true parameter to method process()), then they have to check whether there have been errors or not, just like they currently use a try/catch block.

As illustrated in the example in the documentation, there are try/catch blocks: this is not part of the library, it's in userland. I know that this way, if exceptions are stored instead of being immediately thrown and they are not checked later, it can cause an unexpected behavior: just like exceptions would be catched with no subsequent action (like showing help text).

So I'm just implementing this in a very simple way: when an error occurs (Unexpected or Missing only), send it to an error() method, which will check whether the script should throw the exception or not. If not, store it.

I'm adding two public methods: hasErrors() and getErrors().

Please, let me know if you think I'm doing the wrong way (I'll submit a PR soon, still need to write tests and adapt doc).

[tflori]

so the process method catches his own exceptions and checks an option if the exception should be thrown or just rememberd? sounds good

[Arcesilas]

That's right.
Could look like this: https://github.com/Arcesilas/getopt-php/commit/ca2baca811cd43b0a95017777fe187607adedfba

[Arcesilas]

It appears that exceptions may be thrown from Arguments::process(), more precisely from Argument::setValue(), when the value does not pass validation.
It would then require the Argument (Operand or Option) to hold an instance of GetOpt. It's easy with a factory, but requires many changes and possibly BC breaks.

Another solution is to use a static property and static public methods on GetOpt. This way, it's possible to call the GetOpt::error() method from within an Argument when assigning the value:

protected static $throwErrors = false;
protected static $processErrors = [];

public static function error(ArgumentException $exception)
{
    // handle the error
}

The GetOpt::getErrors() can remain non-static.

I don't really like static properties, generally, since they are stateless. But in this case, there should not be more than one instance of GetOpt, and no more than one array of arguments is supposed to be processed at the same time.

I also wonder if a setter for $throwExceptions is appropriate, so that both following are possible:

$getOpt = new GetOpt(null, false);

// or:

$getOpt = new GetOpt();
$getOpt->throwErrors(false);

[tflori]

https://github.com/Arcesilas/getopt-php/commit/ca2baca811cd43b0a95017777fe187607adedfba
looks good except some namings.

The comment about static methods I don't understand. There are only 3 static methods in the GetOpt class. There is no way to use the class statically - why we should remember the errors statically (global; over all GetOpt instances)?

Remember: the dev could create multiple getOpt parser that handle different scenarios. We had this in another issue: #142 (comment)

[tflori]

I would recommend to have two settings: SETTING_THROW_EXCEPTIONS and SETTING_STOP_ON_ERROR. When throwing exceptions stop on error is implicit enabled.

The exception from setValue has to be catched the usual way:

try { 
  $options->setValue($value);
  return true;
} catch (ArgumentException $exception) {
  $this->error($exception);
  return false;
}

The setter closures in GetOpt\GetOpt::process() need a boolean return value and the loop in GetOpt\Arguments::process() needs something like this:

$stopOnError = $getOpt->get(GetOpt::SETTING_STOP_ON_ERROR);
$continue = true;
while (($arg = array_shift($this->arguments)) !== null) {
  if ($stopOnError && !$continue) {
    return false;
  }
  // ...
  $continue = $setOption($option);
}

[Arcesilas]

In what I had in mind, THROW_EXCEPTIONS and STOP_ON_ERRORS were exactly the same. I'm not sure it's relevant to stop on errors and not throw exceptions: it's exactly the same as using a try/catch block but with more logic, which makes the code less readable, IMHO.

The problem I pointed out in this issue was that when an exception is thrown (which means "stop on error"), the remaining options and operands are not set. The goal of my proposition is to allow the remaining options and operands to be set, even if there are errors.

Also, I'm thinking about one question: isn't it inappropriate to not throw any exception at all when there are errors? In fact, the problem is not that an exception is raised, but that it's raised to soon.

Actually, I think the behaviour should be as so:

• process the arguments
• if any error happens, always store it
• when finishing process (i.e. the "last" line of GetOpt::process() method), throw an exception with all exceptions (an "aggregate" Exception which would hold all exceptions which have occured during process).

This would prevent the script from running with invalid / missing / unexpected arguments.

If the user wants to do nothing in the catch block, that's their problem. It's not possible to forget something. Also, this way, no setting and the behaviour is not changed at all for older code (the "aggregate" exception would simply return the first exception message and code when using getMessage() and getCode(). We could access all exceptions with a getStack() method, for example.

[Arcesilas]

Actually, we can simply use Exception chaining.

[tflori]

Yes, stop on error without throwing exceptions is the same as catching a thrown exception. But some people might argue that the code looks very different:

$getOpt = createGetOpt();
$getOpt->process($argv);

$options = $getOpt->hasErrors() ? $defaultOptions : $getOpt->getOptions();

// vs.

try {
  $getOpt = createGetOpt();
  $getOpt->process($argv);
  $options = $getOpt->getOptions();
} catch (ArgumentException $exception) {
  $options = $defaultOptions;
}

If an exception is thrown to soon or to late depends on the length of arguments and the complexity to process them (validators that query a database for example). I wouldn't agree to a solution where it is not possible to stop processing when an error occurs.

The idea of not throwing immediately but after the processing with a "aggregate" exception is a 3rd method in my opinion. Maybe an option ERROR_HANDLING with 3 versions: throw, gather, silent.

The "aggregate" exception I would rather develop like this:

class ProcessingErrors extends ArgumentException
{
  /** @var ArgumentException[] */
  protected $errors = [];

  public function __constructor($errors) {
    $this->errors = $errors;
    parent::__constructor('Errors occured during processing the arguments');
  }

  public function getErrors() {
    return $errors;
  }
}