Feature request: create new transform method that both maps and validates values

[kael-shipman]

Instead of Issue #86, perhaps it makes more sense to implement a transform method that handles both validation and value mapping.

I have certain options that accept simple json strings as values, for example. These will require both a transformation from string to php array, and also certain object-specific validations.

To handle this well, I'll need the ability to deliver fine-grained error messages and also the ability to parse the value only once, rather than parsing the value for validation, then parsing again for mapping.

Here's how I see this working:

$cmd->option("t")
    ->require()
    ->transform(function($val) {
        $val = json_decode($val, true);
        if (!$val) {
            throw new OptionValidationException("You must provide a valid json-encoded object for option 't'");
        }

        $errors = [];
        if (empty($val['prop1']) || !is_string($val['prop1'])) {
            $errors[] = "`prop1` of option 't' must be a string";
        }
        if (empty($val['prop2']) || !preg_match("/^[a-fA-F0-9-]{32,36}$/", $val['prop2'])) {
            $errors[] = "`prop2` of option 't' must be a valid GUID or UUID"
        }

        if (!empty($errors)) {
            $e = new OptionValidationException("");
            $e->setOptionValidationErrors($errors);
            throw $e;
        }

        if (empty($val['prop3'])) {
            $val['prop3'] = 0;
        } elseif (!is_int($val['prop3'])) {
            $val['prop3'] = (int)$val['prop3'];
        }

        // Return the transformed value for the option
        return $val;
    });

[NeoVance]

Can this not be done using map or cast and must?

[kael-shipman]

Ah! I didn't know about map. I think that probably does fit the bill. Thanks!

[kael-shipman]

Actually, scratch that, there are a few essential features that are missing:

1. must is called before map, meaning you have to transform your value twice if you want to validate on the mapped value.
2. You can't communicate more than one error (my solution involves a modified exception type that accepts an array of errors that can then be consumed when the exception is caught).

I think if we were to implement this new exception type (10 lines of code + a few for catching in the right spot), then I could use it from within map and just forget about must. In other words, my new proposal is that we implement \Commando\OptionValidationException and catch it, if thrown, on parse. This would allow us to read the errors attached to the exception and display them to the user, or just display the message if no errors are attached.

[NeoVance]

See #93, where if using reduce map is called on the values before being passed to reduce, and before any validation, and should fix the issue you were having with must.

There are a few changes that need to be made around validation I feel as well. Can you create a PR for us?

[kael-shipman]

Sure. Are the approved issues tagged or anything to make them recognizable?

Just started this branch to track the work.

[NeoVance]

Probably, but Nate still has not made me a maintainer, so I don't manage tags, so I just keep track in my head.

[kael-shipman]

Ah, ok. Well I guess there aren't a ton. I'll just read through the open issues and see if I can assemble a good solution for the ones that speak to validation. Will see if I can dig in this weekend.