[DomCrawler] Skip disabled fields processing in Form

[sbogx]

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #28179
License	MIT

[nicolas-grekas]

Can you give us more details as to why this is needed?
Looking at the changes, this raises a red flag on my side: that's a clear behavior change, thus has the potential to break existing apps.

[sbogx]

It is indeed a behaviour change.
The issue with the current implementation is that if you have two fields with the same name, and the 2nd one is disabled, it is considered in the logic and it affects the way the first field is getting processed.

Normally if a field is disabled in a form, it is completely ignored and that's what I intended to fix with the changes.

[sbogx]

@nicolas-grekas , I created the following mini-repo for demoing the issue https://github.com/sbogx/DomCrawler-disabled

[fabpot]

Thank you @sbogx.

[alexpott]

In light of #35923 I think we should consider reverting this. Maybe to fix the original poster's problem we need to detect when multiple fields have the same name and enact some logic?

[sbogx]

Hi @alexpott, the point of the DomCrawler is to simulate a normal functioning form, this change ensures this behaviour when processing disabled fields. I recommend checking the example repo.

[alexpott]

@sbogx well then this fix requires changes to a lot of things that use domcrawler for example \Behat\Mink\Driver\BrowserKitDriver::isChecked(). I'm not sure that I agree that sole purpose of the

DomCrawler is to simulate a normal functioning form

To quote the readme it's purpose is to

The DomCrawler component eases DOM navigation for HTML and XML documents.

This change has broken the ability to navigate the DOM for disabled fields and check their values.

[nicolas-grekas]

We should revert the PR for sure. Changing behavior and affecting legit use cases should not happen in a bugfix release.

[pfrenssen]

The issue with the current implementation is that if you have two fields with the same name, and the 2nd one is disabled, it is considered in the logic and it affects the way the first field is getting processed.

Disabled form fields are perfectly normal and should be supported by dom-crawler. I checked the demo repo https://github.com/sbogx/DomCrawler-disabled and dom-crawler already has support for selecting those fields.

Selecting the enabled element

1. Using XPath:

$crawler->filterXPath('descendant-or-self:://input[@type="text" and not(@disabled)]');

2. Using CssSelector

$crawler->filter('input[name="field1"]:enabled');

Selecting the disabled element

1. Using XPath:

$crawler->filterXPath('descendant-or-self:://input[@type="text" and @disabled="true"]');

2. Using CssSelector

$crawler->filter('input[name="field1"]:disabled');

The test case is doing the following:

$crawlerDisabled->filter('form')->form()->getValues();

but since getValues() is returning items indexed by the name attribute, you probably shouldn't use this method to filter elements that have identical names. The current behavior in this case is undefined. I would suggest that we at least update documentation for the Form::getValues() method.

Note that it is allowed in HTML to have identical names for different form elements, regardless whether or not some of those fields are disabled. This is also valid HTML:

<form action="" method="GET">
    <input type="text" name="field1" value="1"/>
    <input type="text" name="field1" value="2"/>
    <input type="text" name="field1" value="3"/>
    <input type="text" name="field1" value="4"/>
    <input type="submit"/>
</form>

In the above example you also don't get the expected results back from Form::getValues(). But you can still get to those individual elements by using a selector such as 'input[name="field1"]:nth-of-type(1).

[sbogx]

@pfrenssen My issue was not whether DomCrawler should or should not support disabled fields. I know how the implementation works and how to get a specific disabled input.

The issue that this PR is trying to fix is to make the \Symfony\Component\DomCrawler\Form, which in its DocBlock says Form represents an HTML form., to function as one. When you submit an html form with disabled fields, the fields don't get sent. That was the idea behind the demo.

The problem originates from \Codeception\Module\PhpBrowser which uses the DomCrawler and its implementation of the Form for getting the values required for posting the data.

In this case, maybe it would be better to have a new, separate collection of values that will be used by \Symfony\Component\DomCrawler\Form::getPhpValues (used in \Symfony\Component\BrowserKit\Client::submit) to fix the bad behaviour.

[pfrenssen]

The Form object is a representation of a form as close as possible to the actual data in the DOM, and its main purpose is to facilitate DOM crawling.

Submitting of forms is not handled by this object but by client side code (e.g. Codeception). The ::getValues() method is provided for this purpose, as it will filter out disabled fields. So this means that the problem that is demonstrated in the demo repo should be fixed by changing only the ::getValues() method, not by changing the internal data model in a way that it no longer aligns with the DOM.

In real life if a form is submitted containing multiple fields with the same name, then the value of the last defined field is submitted, and all others are discarded. So a proper fix could be something like this:

    /**
     * Gets the field values.
     *
     * The returned array does not include file fields (@see getFiles).
     *
     * @return array An array of field values
     */
    public function getValues()
    {
        $values = [];

        $fields = array_filter($this->fields->all(), function (FormField $field) {
          return !$field->isDisabled();
        });

        foreach ($fields as $name => $field) {
            if (!$field instanceof Field\FileFormField && $field->hasValue()) {
                $values[$name] = $field->getValue();
            }
        }

        return $values;
    }

[pfrenssen]

Hmm I tried it and it has the same result. It seems Form::addField() only considers pre-existing fields with the same name. We need a better way to index the fields inside FormFieldRegistry, and not rely on the field name. A simple fix could be to suffix duplicate field names with e.g. .0, .1 etc, but this would likely introduce other B/C problems.