Throw exception, when doing a "setValue" call on checkbox with a non-boolean value

[aik099]

Fixes a test failure for a test, that is introduced in the minkphp/driver-testsuite#102 .

P.S.
The static analysis failures are unrelated.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.03%. Comparing base (7a74a76) to head (0fbf050).
Report is 2 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #183      +/-   ##
============================================
+ Coverage     98.01%   98.03%   +0.01%     
- Complexity      127      129       +2     
============================================
  Files             1        1              
  Lines           303      305       +2     
============================================
+ Hits            297      299       +2     
  Misses            6        6              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[uuf6429]

Just wondering: since there may be other cases, could it make more sense to catch (specific) exceptions when calling $field->setValue and rethrowing them as DriverException?

[aik099]

The $field->setValue(...) (method of BrowserKit) doesn't throw any exceptions.

[uuf6429]

That doesn't seem right - the failure message stated that an unexpected exception was thrown:

 ✘ Set invalid value in field with checkbox·field·with·string
   ┐
   ├ Failed asserting that exception of type "InvalidArgumentException" matches expected exception "Behat\Mink\Exception\DriverException". Message was: "Input "agreement" cannot take "updated" as a value (possible values: "yes")." at
   ├ /home/runner/work/driver-testsuite/driver-testsuite/vendor/symfony/dom-crawler/Field/ChoiceFormField.php:140                                                                                                                        
   ├ /home/runner/work/driver-testsuite/driver-testsuite/src/BrowserKitDriver.php:420                                                                                                                                                    
   ├ /home/runner/work/driver-testsuite/driver-testsuite/vendor/behat/mink/src/Element/NodeElement.php:118                                                                                                                               
   ├ /home/runner/work/driver-testsuite/driver-testsuite/vendor/mink/driver-testsuite/tests/Form/GeneralTest.php:366                                                                                                                     
   ├ phpvfscomposer:///home/runner/work/driver-testsuite/driver-testsuite/vendor/phpunit/phpunit/phpunit:97          

and the Symfony code definitely throws exceptions: https://github.com/symfony/dom-crawler/blob/7.2/Field/ChoiceFormField.php#L102

(e.g. looking at that code, I realised we are neither testing the setValue(null) case in driver-testsuite, nor handling it here in BrowserKitDriver)

[aik099]

Ah. I've looked at the wrong setValue method. Not of the ChoiceFormField class at least.

The ChoiceFormField::setValue method is tricky, because it handles 3 control types (checkboxes, radio button, selects). As you can see from the code if it's not true/false, but a checkbox control was used it goes into select branch and I doubt that we should even allow that, because error message would be super non-obvious.

@uuf6429 , knowing that would it still be possible to implement your original proposal (catch BrowserKit exception and convert it to something we want; better with a useful exception message)?

[uuf6429]

@aik099 given that the original exceptions already have a decent message (I think they do, e.g. one case mentions a list of possible option values, whereas we do not do that), the BrowserKit setValue method could as well be defined simply as:

    public function setValue(string $xpath, $value)
    {
        $field = $this->getFormField($xpath);

        try {
            $field->setValue($value);
        } catch (\InvalidArgumentException $e) {
            throw new DriverException("Cannot set value: {$e->getMessage()}", 0, $e);
        }
    }

IMO keeping the current validation logic (on top of Symfony's) is a bit redundant, but let's say we do that to have custom exception messages, then I would keep it but still wrap $field->setValue() in case some other exception goes through:

    public function setValue(string $xpath, $value)
    {
        $field = $this->getFormField($xpath);

        ...

        if (\is_array($value) || \is_bool($value)) {
            throw new DriverException('Textual and file form fields don\'t support array or boolean values.');
        }

        try {                                                                          👈
            $field->setValue($value);
        } catch (\InvalidArgumentException $e) {                                       👈
            throw new DriverException("Cannot set value: {$e->getMessage()}", 0, $e);  👈
        }                                                                              👈
    }

AFAIK our driver implementations are supposed to throw DriverException or its children.

[aik099]

Correct exception class makes the test pass - the computer is happy. I want to make sure, that humans are happy as well by providing useful exception messages.

@uuf6429 , please create a comparison table showing each of the currently thrown exceptions (from the MinkBrowserKitDriver::setValue method):

• what is the condition for exception throwing?
• current exception message
• proposed exception message (when our custom exception messages, that we throw before calling BrowserKit setValue method, removed)
  .

I just want to make sure, that reducing exception handling code won't reduce exception message clarity.

[uuf6429]

Correct exception class makes the test pass

My main point is not about passing tests, it's that we defined a contract (the DriverInterface) that tells driver users what kind of exceptions to expect and tells driver developers what kind of exceptions they can throw.

I want to make sure, that humans are happy as well by providing useful exception messages.

Sure, but sticking to the contract is also quite important. As a mink user, I've had this happening once or twice - it can be quite disruptive if you have a retry mechanism that is based on catching specific exceptions.

---

I'll do the comparison later.

[uuf6429]

condition: input-type == radio && value-type == array
driver	Only string values can be used for a radio input.
symfony	The value for "<name of input>" cannot be an array.

condition: input-type == radio && value-type != array && value-type != string
driver	Only string values can be used for a radio input.
symfony	Input "<name of input>" cannot take "<stringified value>" as a value (possible values: <comma-separated list of quoted possible values>).

condition: input-type == checkbox && value-type != bool
driver	Only boolean values can be used for a checkbox input.
symfony	Input "<name of input>" cannot take "<stringified value>" as a value (possible values: <not sure what goes here for checkbox but I guess "1", "">).

condition: input-type == select && value-type == bool
driver	Boolean values cannot be used for a select element.
symfony	if there are options with a value of "1" or "", I think symfony can select them when given boolean values (while we show an exception) - otherwise same message as (2)

condition: value-type == array || value-type == bool
driver	Textual and file form fields don\'t support array or boolean values.
symfony	same logic as (4)

condition: value-type == array || value-type == string
driver	none
symfony	logic from (4), in the sense that it checks if the element is multi-selectable and if the specified option(s) exist

condition: value-type == string && value-format != input-format (e.g. non-numeric characters in an <input type="number"/>)
driver	none
symfony	no idea if symfony does anything about this

[aik099]

I've performed almost the same analytics, but from a different angle:

• commented-out custom exception throwing code for radio/checkbox/select in the \Behat\Mink\Driver\BrowserKitDriver::setValue method
• compared what of the messages (Mink custom or BrowserKit/Symfony native) looked what would be more useful
  .

Here are the results:

• GeneralTest::testSetInvalidValueInField (checkbox with array and string):
  • array given:
    • symfony error: The value for "agreement" cannot be an array.
    • mink error: Only boolean values can be used for a checkbox input.
  • string given:
    • symfony error: Input "agreement" cannot take "updated" as a value (possible values: "yes").
    • mink error: Only boolean values can be used for a checkbox input.
• RadioTest::testSetArrayValue (setting array into radio button):
  • symfony error: The value for "group1" cannot be an array.
  • mink error: Only string values can be used for a radio input.
• RadioTest::testSetBooleanValue:
  • setting true into radio button:
    • symfony error: Input "group1" cannot take "1" as a value (possible values: "set", "updated").
    • mink error: Only string values can be used for a radio input.
  • setting false into radio button:
    • symfony error: Input "group1" cannot take "" as a value (possible values: "set", "updated").
    • mink error: Only string values can be used for a radio input.
• SelectTest::testSetBooleanValue:
  • setting true value:
    • symfony error: Input "select_number" cannot take "1" as a value (possible values: "10", "20", "30").
    • mink error: Boolean values cannot be used for a select element.
  • setting false value:
    • symfony error: Input "select_number" cannot take "" as a value (possible values: "10", "20", "30").
    • mink error: Boolean values cannot be used for a select element.

The Symfony error messages were 100% cases more useful, than Mink provided ones. That is an unexpected result.

If we decide to go with @uuf6429 approach (for ChoiceFormField field wrap exception provided by the BrowserKit into DriverException instead of throwing our own exceptions), then error messages of this driver related to checkbox/radio/select would look radically different.

@stof , what you think?

[stof]

I'll fix the phpstan job separately to account for the new generic types in BrowserKit.