-
Notifications
You must be signed in to change notification settings - Fork 280
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.
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
Add hasOption method to NodeElement class. #766
base: master
Are you sure you want to change the base?
Conversation
This will trigger 2 changes of the data of the form, to check whether it has an option or no. that's quite bad as implementation of the check. |
Yeah you are right with this. I will rewrite it to don't change the value of the select. |
src/Element/NodeElement.php
Outdated
*/ | ||
public function hasOption($option){ | ||
if ('select' !== $this->getTagName()) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is wrong. It must return a boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I writed this because if the element is not a select tag, It is not logical to return a boolean. That would be nice if I throw an exception instead of return null? Or editing the documentation of method to @return Boolean|null
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's better to throw an exception.
src/Element/NodeElement.php
Outdated
return; | ||
} | ||
|
||
$optionElement = $this->find('named', array('option', $option)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can safely inline this variable, because it's used only once.
In my case that was a useful method, to check that a select element has an option or not. And I think this will be also useful for others.