Skip to content

Forms: Selectbox allow item with null caption [ID => null] - #4

Merged
dg merged 1 commit into
nette:masterfrom
simara-esports:master
Apr 18, 2014
Merged

Forms: Selectbox allow item with null caption [ID => null]#4
dg merged 1 commit into
nette:masterfrom
simara-esports:master

Conversation

@simara-esports

Copy link
Copy Markdown
Contributor

No description provided.

@JanTvrdik

Copy link
Copy Markdown
Contributor

Why not just use empty string?

@simara-esports

Copy link
Copy Markdown
Contributor Author

Sorry for the second commit, I'll try to remove it..

Well, it's a bug.
If you'll try to pass data from Selection::fetchPairs(), where one row have an empty name (NULL), you'll see a very strange error message and you'll ask WAT? :-)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

=> not ,
Edit: Actually the second item is irrelevant.

@mishak87

mishak87 commented Apr 4, 2014

Copy link
Copy Markdown
Contributor

It would make more sense to sanitize options in Select. Here null value is a duplicate to empty string.

@simara-esports

Copy link
Copy Markdown
Contributor Author

@mishak87 Typo, it wasn't meant to be 3 items.

@simara-esports

Copy link
Copy Markdown
Contributor Author

Can You please accept or reject this pull request? If you'll reject, please fix the bug described in the test case.

@dg

dg commented Apr 15, 2014

Copy link
Copy Markdown
Member

I'll merge it, but please squash it to single commit and use uppercased NULL in tests.

Forms: test fix, test for ChoiseControl

Forms: Coding standard fix

NULL should be uppercase
@simara-esports

Copy link
Copy Markdown
Contributor Author

Done, thanks

@dg

dg commented Apr 18, 2014

Copy link
Copy Markdown
Member

Thx !

dg added a commit that referenced this pull request Apr 18, 2014
Forms: Selectbox allow item with null caption [ID => null]
@dg
dg merged commit 23be8ee into nette:master Apr 18, 2014
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.

4 participants