[DomCrawler] added support for HTML5 'form' attribute #7500
Conversation
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #7486 |
| License | MIT |
| Doc PR |
| } while ('form' != $node->nodeName); | ||
| $this->node = $form; | ||
| return; | ||
| } else { |
There was a problem hiding this comment.
You don't need to use else as the if returns.
|
This is only implementing half of the support of the |
| $this->assertTrue(true, '__construct() throws a \\LogicException if the form attribute is invalid'); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I would make 2 tests, one to test form="bar" is okay, one to test form="nonexistent" throws exception.
|
@stof you were right, I extended it. is it ok now? |
|
|
||
| $nodeName = $node->nodeName; | ||
| private function addField(\DOMNode $node, \DOMNode $button) { |
There was a problem hiding this comment.
the curly brace should be on its own line
| @@ -331,7 +331,7 @@ public function offsetUnset($name) | |||
| } | |||
|
|
|||
| /** | |||
| * Sets current \DOMNode instance. | |||
| * Expects a 'submit' button and finds the corresponding form element. | |||
There was a problem hiding this comment.
That's not how we document function. The first line must say what it does (something like Sets the node for the form). And then,after a blank line, you can explain what it expects (what you've written here).
|
Can you add a note in the component CHANGELOG as this is a new feature? |
|
@fabpot Done. |
This PR was merged into the master branch. Discussion ---------- [DomCrawler] added support for HTML5 'form' attribute | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #7486 | License | MIT | Doc PR | Commits ------- f8178dd [DomCrawler] added support for HTML5 'form' attribute