Skip to content

Conversation

@SOHELAHMED7
Copy link
Contributor

@SOHELAHMED7 SOHELAHMED7 commented Apr 9, 2021

Current status

When a form have following inputs:

<input type="hidden" name="coffee" value="0">
<input type="checkbox" name="coffee" value="1" id="coffee-id" checked>

and when checkbox is unchecked and then if the form is submitted, value '0' of coffee is not sent.

Ideally it should.

This PR fix that issue.

Failing test: d56484c

@SamMousa

Have any questions, feel free to ask

@Naktibalda
Copy link
Member

Wouldn't it be better to check if Checkbox field is unchecked and then skip it.

Browsers process form fields in a linear fashion (that's the point of putting hidden field before checkbox).
Your patch introduces non-linear behaviour which is nothing like browser's and it will introduce a lot new bugs.

Could you check if skipping unchecked fields work?

Before:

foreach ($fields as $field) {
	if ($field instanceof FileFormField || $field->isDisabled() || !$field->hasValue()) {
	    continue;
	}
	// ...
}

After:

foreach ($fields as $field) {
	if ($field instanceof FileFormField || $field->isDisabled() || !$field->hasValue()) {
	    continue;
	}
	
	if ($field instanceof CheckboxField && $field->isChecked() === false) {
	    continue;
	}
	// ...
}

@SamMousa
Copy link
Contributor

The issue is that the symfony component does not add all fields to the form... It essentially deduplicates based on name too early

@Naktibalda
Copy link
Member

Ah, it is a WONTFIX issue: symfony/symfony#11689

@SamMousa
Copy link
Contributor

Yeah, so the question is do we want to work around it via the solution proposed by @SOHELAHMED7 ?

@Naktibalda
Copy link
Member

It seems that a better solution would be to stop using Symfony\Component\DomCrawler\Form, extract all fields using Crawler and do all processing in InnerBrowser code, but it is a lot of work.

So a simple fix like this will have to do for now.

@SOHELAHMED7
Copy link
Contributor Author

@Naktibalda

Do you mean to say like this 0d03718 ?

@Naktibalda Naktibalda merged commit d5b87df into Codeception:master Apr 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants