Error Reporting is Atrocious about getopt-php HOT 7 CLOSED

I passed an array to GetOpt::process() which indeed conforms to your annotation of array, however the array was invalid and contained another array due to an error on my part.

1. Of course we could check deeper but every check requires cpu cycles - and it's quite useless. That you can pass an array is documented in Working With Options. I thought it's clear that it's an array of strings like argv in every language is. If not we could adjust the documentation.
2. The correct way to annotate array of strings in php is string[] the most IDEs should understand this.
3. The process method has already a type check with an error message that if you pass an unaccepted value. However with an array of arrays we didn't calculate. This error messages also says you should pass an "array of arguments" that one argument on the command line is a string was clear for me. https://github.com/getopt-php/getopt-php/blob/master/src/GetOpt.php#L142

That was the objective part. The personal part is: I don't know how good is your English but I wouldn't use "atrocious" to describe error messages of a library I have chosen and some developers put a lot of effort into. Sentences like "WTF? Seriously" and "This function is an incomprehensible mess" are just emotional and not constructive criticism. You wrote that "things like continue & break are discouraged" like it is not your opinion but you share this opinion. If you have more constructive criticism how this could be written with less confusion create a PR, thanks.

BTW: I think the method is fairly readable and straight forward. It got a small refactoring to support spaces in command names but it wasn't that big. So it has proven to be maintainable. It is somehow the heart of this library where the arguments getting processed.

from getopt-php.

Before I forget, the issue is that $name is an empty array on line 288, but I don't have a hope or prayer figuring out how/why.

from getopt-php.

"Functions are useless structures. Indeed the goto command could replace every structure like loops, functions, continues and so on."

This or similar sentences you could hear in the 70s. While some guys had this opinion and where known for an important or great algorithm others were just following his opinion. But we are lucky that others ignored them and created such great functionality in programming languages. Just because someone is saying "else can always be replaced" I would still prefer an else to keep things together and improve the readability.

But instead spreading your opinion and that you can't understand the code we should focus on the issue.

You say there is a bug in line src/GetOpt.php:288 where an array is used for array access. Maybe for a developer it is helpful to copy the code where the error appears (instead the code you find confusing):

    /**
     * Get the current or a named command.
     *
     * @param string $name
     * @return Command
     */
    public function getCommand($name = null)
    {
        if ($name !== null) {
            return isset($this->commands[$name]) ? $this->commands[$name] : null;
        }
        return $this->command;
    }

Without debugging it: you mean we should always check that $name is a string (like the annotation suggests) and throw an individual error message? Or should we throw a E_USER_WARNING when somone tries to use something else?

In the future I would prefer to drop php 5.6 support and add type declarations to the parameters where possible. But for now that is not possible and creating a new major release just because of this feature is not necessary.

from getopt-php.

"Functions are useless structures. Indeed the goto command could replace every structure like loops, functions, continues and so on."

I have no idea where you are quoting from and I am not sure anyone said or meant that to be taken seriously. The whole reason to avoid using goto and break and continue etc is precisely to "improve the readability" and comprehension of program flow, so thank you for reiterating my point. Just because you can, doesn't mean you should.

Maybe for a developer it is helpful to copy the code where the error appears

The reason we are looking at Arguments::process() is because that is where the side effect originates from. The issue is that input to new Arguments() from GetOpt L137 is not appropriately checked and eventually shits its pants when running process().

The annotation on getCommand and the function itself is not the issue here... I am not even using getCommand in my code. It is called internally by your code, where you didn't check that it was a string as the annotation suggests. I passed an array to GetOpt::process() which indeed conforms to your annotation of array, however the array was invalid and contained another array due to an error on my part. I am just pointing out that the software fails in an obtuse way on invalid input and taking a peek under the hood to see why it failed was not as easy or straight forward as it could have been.

I am not sure what the "solution" to this issue should be, maybe a better annotation on process function like array<int,string> in the hopes that a linter will catch the error? Maybe better validation inside of the while loop of Arguments::process() particularly, but not limited to, isValue() would be the better option as empty($arg) doesn't really ensure that $arg is a string? Maybe it is as simple as $arg === "" instead of an empty() check? I don't know how you prefer to do sanity checks.

from getopt-php.

Of course we could check deeper but every check requires cpu cycles

You should use a profiler to optimize your code, not guess about how much cpu time something needs.

The correct way to annotate array of strings in php is string[] the most IDEs should understand this.

The string[] is the official PHPDoc type and array<key, values>is the more descriptive phan/phan type. Both are supported by PHPStorm.

The process method has already a type check with an error message that if you pass an unaccepted value.

This is totally irrelevant. This exception was never thrown as part of this issue.

from getopt-php.

"things like continue & break are discouraged" like it is not your opinion but you share this opinion.

BTW, this isn't my opinion. This is the general consensus of many pioneers of computer science.

from getopt-php.

😆

"things like continue & break are discouraged" like it is not your opinion but you share this opinion.

BTW, this isn't my opinion. This is the general consensus of many pioneers of computer science.

how long you want to continue this? I say it is not your opinion - you say this is not your opinion. As you would arguing but in fact you agree that it is not your opinion.

Some years ago singleton was "the shit!" you have to use it. This opinions change every now and then. If I would adept every library because someone thinks this or that is not readable I would work all day just to follow all the guidelines. Of course some of them might be useful because of maintainability or performance but not the generalizations like in php-md "else is discouraged". Maybe they just never saw code where it was better readable with an else branch - I don't know. I'm also open for a better readable code but not for such blaming as you did.

The type check is irrelevant? First you want more deeper type checks and now it's irrelevant...

I never said how much time it costs to check types I said it costs cpu cycles. and no matter how deeply you follow the conversations of which guru: you will never hear a from a performance optimist that cpu cycles don't matter.

from getopt-php.