-
Notifications
You must be signed in to change notification settings - Fork 172
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
Issue #4036: Allow for an empty option in select fields via schema pr… #4037
Conversation
See my comment on the issue @stefan-korn, I think the proper behavior here is just to add an empty option in the select for any |
@@ -77,6 +77,9 @@ public function handleStringElement($definition, $data, $object_schema = FALSE) | |||
// Add options if element type is select. | |||
if ($element['#type'] === 'select') { | |||
$element['#options'] = $this->getSelectOptions($property); | |||
if ($property->empty_value) { | |||
$element['#empty_value'] = ''; |
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.
Looks like we're going a different direction anyway, but not sure where $property->empty_value
is supposed to be getting set?
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 supposed it to be set in the schema .json file, just like "type", "format", etc. But yes now we are going a different and better way.
@stefan-korn do you feel up to updating the tests? I think you should be able to see the circleCI results and that the tests are all broken now... fairly trivial to update them to expect this and possibly to confirm the right result for both required and non-required fields, but if you're not familiar with PHPUnit let me know and we can help with that part. |
I fixed the tests. The html output itself is not checked I suppose? |
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.
Sorry I let this languish for so long @stefan-korn - just tested and it works perfectly.
fixes #4036
QA Steps