Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Add empty option before empty check #2837

Closed
wants to merge 4 commits into from

Conversation

vseager
Copy link
Contributor

@vseager vseager commented Oct 25, 2012

Add empty option to options array before checking if is empty, to allow for an empty select in cases where a user may not have added options into database require to populate field.

@davidwindell
Copy link
Contributor

👍 this would save us a lot of code checks

@davidwindell
Copy link
Contributor

Actually, I'd prefer if it didn't throw a hard exception at all, I don't see the need for it here as the validator would check for an empty value. Can you update the PR?

@weierophinney
Copy link
Member

Please update the tests as well, as they are testing for an exception thrown when no options are passed, and thus failing.

@ghost ghost assigned weierophinney Oct 30, 2012
@minlare
Copy link

minlare commented Oct 31, 2012

I have a select which will be populated by javascript. Please can we get this merged asap.

@@ -337,11 +337,9 @@ public function testRenderElementWithNoNameRaisesException()
$this->helper->render($element);
}

public function testRenderElementWithNoValueOptionsRaisesException()
public function testRenderElementWithNoValueOptionsDoesNotRaiseException()
Copy link
Member

Choose a reason for hiding this comment

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

In fact the test should be removed due the almost the test of the file already test this

@davidwindell
Copy link
Contributor

@Maks3w doesn't this serve as a regression test?

@Maks3w
Copy link
Member

Maks3w commented Nov 2, 2012

@davidwindell Do a regression with this is really very very hard, it's not as simple as change a conditional operator or something. This involve throw an exception.

@davidwindell
Copy link
Contributor

@vseager can you remove the test?

@vseager vseager closed this Nov 8, 2012
@vseager vseager reopened this Nov 8, 2012
@vseager
Copy link
Contributor Author

vseager commented Nov 8, 2012

@weierophinney Travis is failing on PHP 5.3.3 but not on newer versions. Can you pass this PR?

weierophinney added a commit that referenced this pull request Nov 13, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants