Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stof merged 1 commit into from
Jan 9, 2025
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/BrowserKitDriver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
/**
* @var AbstractBrowser
*/
private $client;

Check failure on line 37 in src/BrowserKitDriver.php

View workflow job for this annotation

GitHub Actions / Static analysis 

Property Behat\Mink\Driver\BrowserKitDriver::$client with generic class Symfony\Component\BrowserKit\AbstractBrowser does not specify its types: TRequest, TResponse

/**
* @var array<string, Form>
Expand All @@ -54,7 +54,7 @@
*
* @param string|null $baseUrl Base URL for HttpKernel clients
*/
public function __construct(AbstractBrowser $client, ?string $baseUrl = null)

Check failure on line 57 in src/BrowserKitDriver.php

View workflow job for this annotation

GitHub Actions / Static analysis 

Method Behat\Mink\Driver\BrowserKitDriver::__construct() has parameter $client with generic class Symfony\Component\BrowserKit\AbstractBrowser but does not specify its types: TRequest, TResponse
{
$this->client = $client;
$this->client->followRedirects(true);
Expand All @@ -73,7 +73,7 @@
*
* @return AbstractBrowser
*/
public function getClient()

Check failure on line 76 in src/BrowserKitDriver.php

View workflow job for this annotation

GitHub Actions / Static analysis 

Method Behat\Mink\Driver\BrowserKitDriver::getClient() return type with generic class Symfony\Component\BrowserKit\AbstractBrowser does not specify its types: TRequest, TResponse
{
return $this->client;
}
Expand Down Expand Up @@ -413,6 +413,10 @@
throw new DriverException('Only string values can be used for a radio input.');
}

if (!\is_bool($value) && $field->getType() === 'checkbox') {
throw new DriverException('Only boolean values can be used for a checkbox input.');
Copy link
Member

@uuf6429 uuf6429 Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@aik099 aik099 Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@uuf6429 uuf6429 Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@aik099 aik099 Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member

@uuf6429 uuf6429 Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

@aik099 aik099 Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@uuf6429 uuf6429 Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@uuf6429 uuf6429 Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.
  1. 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>).
  1. 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", "">).
  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)
  1. 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)
  1. 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
  1. 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

Copy link
Member Author

@aik099 aik099 Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

if (\is_bool($value) && $field->getType() === 'select') {
throw new DriverException('Boolean values cannot be used for a select element.');
}
Expand Down
Loading