Skip to content

Conversation

@kukko
Copy link

@kukko kukko commented Oct 18, 2018

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.

@stof
Copy link
Member

stof commented Oct 18, 2018

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.

@kukko
Copy link
Author

kukko commented Oct 18, 2018

Yeah you are right with this. I will rewrite it to don't change the value of the select.

*/
public function hasOption($option){
if ('select' !== $this->getTagName()) {
return;
Copy link
Member

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

Copy link
Author

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?

Copy link
Member

@aik099 aik099 Oct 22, 2018

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.

return;
}

$optionElement = $this->find('named', array('option', $option));
Copy link
Member

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.

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