8000 [DomCrawler] fixed HTML5 form attribute handling by kepten · Pull Request #8197 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DomCrawler] fixed HTML5 form attribute handling #8197

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

Closed
wants to merge 3 commits into from
Closed

[DomCrawler] fixed HTML5 form attribute handling #8197

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
181cddd
fixed HTML5 form attribute handling and added some tests
kepten Jun 4, 2013
787b4ec
removed an unnecessary condition check
kepten Jun 4, 2013
38bb4d3
changed code to improve performance
kepten Jun 5, 2013
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
Next Next commit
fixed HTML5 form attribute handling and added some tests
  • Loading branch information
@kepten
kepten committed Jun 4, 2013
commit 181cddd7d8c2c9b86a9cc19766fdbefb81cb3e8a
61 changes: 45 additions & 16 deletions src/Symfony/Component/DomCrawler/Form.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,31 +366,61 @@ protected function setNode(\DOMNode $node)
$this->node = $node;
}

/**
* Adds form elements related to this form.
*
* Creates an internal copy of the submitted 'button' element and
* the form node or the entire document depending on whether we need
* to find non-descendant elements through HTML5 'form' attribute.
*/
private function initialize()
{
$this->fields = new FormFieldRegistry();

$document = new \DOMDocument('1.0', 'UTF-8');
$node = $document->importNode($this->node, true);
$xpath = new \DOMXPath($document);
$button = $document->importNode($this->button, true);
$root = $document->appendChild($document->createElement('_root'));
$root->appendChild($node);
$root->appendChild($button);
$xpath = new \DOMXPath($document);

// add descendant elements to the form
$fieldNodes = $xpath->query('descendant::input | descendant::button | descendant::textarea | descendant::select', $root);
foreach ($fieldNodes as $node) {
$this->addField($node, $button);
}

// find form elements corresponding to the current form by the HTML5 form attribute
// find form elements corresponding to the current form
if ($this->node->hasAttribute('id')) {
$formId = Crawler::xpathLiteral($this->node->getAttribute('id'));
$xpath = new \DOMXPath($this->node->ownerDocument);
$fieldNodes = $xpath->query(sprintf('descendant::input[@form=%s] | descendant::button[@form=%s] | descendant::textarea[@form=%s] | descendant::select[@form=%s]', $formId, $formId, $formId, $formId));
// traverse through the whole document
$node = $document->importNode($this->node->ownerDocument->documentElement, true);
$root->appendChild($node);
$formNode = $xpath->query(sprintf("//form[@id=%s]", Crawler::xpathLiteral($this->node->getAttribute('id'))))->item(0);
$fieldNodes = $xpath->query('descendant::input | descendant::button | descendant::textarea | descendant::select', $root);
Copy link
Member

Choose a reason for hiding this comment

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

you are now querying all input elements in the page. Is it really more efficient than the previous code ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. I tried Firefox and it preserved form element order, so I wanted to implement the same way but I am not sure if it really matters. I can change it to a more efficient one if we decide order does not need to be preserved.

Copy link
Member

Choose a reason for hiding this comment

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

is the order of POST params really relevant ?


// corresponding elements are either descendants or have a matching HTML5 form attribute
foreach ($fieldNodes as $node) {
$this->addField($node, $button);
if (($node->hasAttribute('form') && $this->node->getAttribute('id') == $node->getAttribute('form'))) {
// if the element has an HTML5 form attribute, it should match the form id
$this->addField($node, $button);
} elseif($node->isSameNode($button)) {
// this is the button used for submission
$this->addField($node, $button);
} elseif (!$node->hasAttribute('form')) {
// find parent form node to check whether this is a descendant element or not
$parentForms = $xpath->query('ancestor::form', $node);

if ($parentForms->length > 0) {
if ($parentForms->item(0)->isSameNode($formNode)) {
$this->addField($node, $button);
}
}
}
}
} else {
// parent form has no id, add descendant elements only
$node = $document->importNode($this->node, true);
$root->appendChild($node);

$fieldNodes = $xpath->query('descendant::input | descendant::button | descendant::textarea | descendant::select', $root);
foreach ($fieldNodes as $node) {
// if the descendant element has an HTML5 form attribute, it should match the parent form id
if (!$node->hasAttribute('form') || ($this->node->hasAttribute('id') && $this->node->getAttribute('id') == $node->getAttribute('form'))) {
Copy link
Member

Choose a reason for hiding this comment

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

$this->node->hasAttribute('id') will always be false here as you are in the else part.

$this->addField($node, $button);
}
}
}
}
Expand All @@ -402,8 +432,7 @@ private function addField(\DOMNode $node, \DOMNode $button)
}

$nodeName = $node->nodeName;

if ($node === $button) {
if ($node->isSameNode($button)) {
$this->set(new Field\InputFormField($node));
} elseif ('select' == $nodeName || 'input' == $nodeName && 'checkbox' == $node->getAttribute('type')) {
$this->set(new Field\ChoiceFormField($node));
Expand Down
48 changes: 40 additions & 8 deletions src/Symfony/Component/DomCrawler/Tests/FormTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,21 +90,53 @@ public function testConstructorHandlesFormAttribute()
$dom = new \DOMDocument();
$dom->loadHTML('
<html>
<form id="bar">
<input type="submit" form="bar" />
<form id="form_1" action="" method="POST">
<input type="checkbox" name="apples[]" value="1" checked />
<input form="form_2" type="checkbox" name="oranges[]" value="1" checked />
<input form="form_1" type="hidden" name="form_name" value="form_1" />
<input form="form_1" type="submit" name="button_1" value="Capture fields" />
<button form="form_2" type="submit" name="button_2">Submit form_2</button>
</form>
<input form="form_1" type="checkbox" name="apples[]" value="2" checked />
<form id="form_2" action="" method="POST">
<input type="checkbox" name="oranges[]" value="2" checked />
<input type="checkbox" name="oranges[]" value="3" checked />
<input form="form_2" type="hidden" name="form_name" value="form_2" />
<input form="form_1" type="hidden" name="outer_field" value="success" />
<button form="form_1" type="submit" name="button_3">Submit from outside the form</button>
</form>
<input type="submit" form="bar" />
<button />
</html>
');

$nodes = $dom->getElementsByTagName('input');
$inputElements = $dom->getElementsByTagName('input');
$buttonElements = $dom->getElementsByTagName('button');

$form = new Form($nodes->item(0), 'http://example.com');
$this->assertSame($dom->getElementsByTagName('form')->item(0), $form->getFormNode(), 'HTML5-compliant form attribute handled incorrectly');
// Tests if submit buttons are correctly assigned to forms
$form1 = new Form($buttonElements->item(1), 'http://example.com');
$this->assertSame($dom->getElementsByTagName('form')->item(0), $form1->getFormNode(), 'HTML5-compliant form attribute handled incorrectly');

$form1 = new Form($inputElements->item(3), 'http://example.com');
$this->assertSame($dom->getElementsByTagName('form')->item(0), $form1->getFormNode(), 'HTML5-compliant form attribute handled incorrectly');

$form2 = new Form($buttonElements->item(0), 'http://example.com');
$this->assertSame($dom->getElementsByTagName('form')->item(1), $form2->getFormNode(), 'HTML5-compliant form attribute handled incorrectly');

// Tests if form elements are correctly assigned to forms
Copy link
Member

Choose a reason for hiding this comment

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

If you test different things, you should have different tests instead of more assertions in the same test. It will give a better output in case of failure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point but I think it tests the same thing (form attribute handling) just in two different ways.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kepten I know it's merged already but for future - the very fact you needed to add comments to group your tests implies they're different cases.

$values1 = array(
'apples' => array('1', '2'),
'form_name' => 'form_1',
'button_1' => 'Capture fields',
'outer_field' => 'success'
);
$values2 = array(
'oranges' => array('1', '2', '3'),
'form_name' => 'form_2',
'button_2' => '',
);
$this->assertEquals($values1, $form1->getPhpValues(), '->getPhpValues() should return all form field values');
$this->assertEquals($values2, $form2->getPhpValues(), '->getPhpValues() should return all form field values');

$form = new Form($nodes->item(1), 'http://example.com');
$this->assertSame($dom->getElementsByTagName('form')->item(0), $form->getFormNode(), 'HTML5-compliant form attribute handled incorrectly');
}

public function testMultiValuedFields()
Expand Down
0