CHtml::listBox() to include an empty value similar to CHtml::activeCheckBoxList() #676

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@maximcherny
Contributor

Bug #235: Fixed CHtml::listBox() to include an empty value via hidden field similar to CHtml::activeCheckBoxList()

@mdomba
Member
mdomba commented May 4, 2012

as you mentioned on the issue this functionality works on activeCheckBoxList but not in checkBoxList, same for the activeListBox and listBox.

so if we add this to listBox then it should be added to checkBoxList too.

opinions?

@maximcherny
Contributor

I agree that for the sake of consistency all the relevant methods should be behaving in a similar manner in the context of this issue.

Therefore, I am happy to amend the remaining methods in question.

However, personally, I think it is the developer's responsibility to ensure that things don't break when expected attributes are not present in the request - nothing still stops one from modifying the DOM and removing those hidden fields and any other fields shall they so desire.

Going through the CHANGELOG, the "uncheckValue" addition was in fact previously marked as enhancement meaning that there was a legitimate use case behind this, unfortunately I can't figure out where something like this may actually be used...

@mdomba
Member
mdomba commented May 4, 2012

I don't understand what you meant with modifying the DOM...

You need to understand the difference of the "normal" and "active" CHtml methods...

The "active" methods are intended to work with a model class, the normal methods are intended to work with classic PHP variables...

When using a model we have the massive assignment option by using $model->attributes=$_POST['modelName']

And because of that the empty/unchek values are added to the "active" methods so that proper attributes gets some value to be processed further. That's because (as you know) HTML in those situations do not send anything if nothing is selected or checked.

That is the reason of adding the empty/uncheck value...

So now we need to see what is the reson or usage for adding those to the "normal" CHtml methods?
Do we need those at all for the normal methods?

@maximcherny
Contributor

Apologies for obscure language, I do understand what the difference is :)

By DOM modification I mean some bad person (potential attacker) firing up a web inspector (i.e. Firebug, WebKit Dev Panel etc) and removing those form elements from the form and submitting your form.

So if your model expects attribue $model->foo to always contain a value when it is populated from the request and I go and remove it from the DOM in my browser and submit the form I may get an "Undefined whatever..." error, whereas one should really check if the attribute exists and is set before trying to access it, or so I think for some reason...

@mdomba
Member
mdomba commented May 4, 2012

You are right... that's why after the assignment fase comes the model validation that checks the validation rules...

So if you set an attribute displayed with a checkbox as required and the user do not check anything... the validation would fail all the time because that attribute do not get any value... it's exactly for this reason why uncheck/empty properties has been added.

And again the question... is that needed for normal methods like listBox()

IMO thisis not needed at all for normal methods as then it's up to the developer to check the POST and act accordingly.

@mdomba mdomba was assigned May 4, 2012
@maximcherny
Contributor

I see - well, AFAIK, the request is NEVER guaranteed to contain the attributes that you want because one can easily craft a form that has no fields whatsoever. In reality, adding a hidden field to mimic an empty value for a checkbox list IMO is not a solution.

Talking about validation:

CRequiredValidator:52
    $value=$object->$attribute;

This is what will cause en error, wheres what I think should happen is a check that explicitly sets the attribute to NULL if it discovers that it has not been assigned any value whatsoever.

To answer your question - I don't think this is needed in either of them since it is easily by-passable, but we can't remove this for BC reasons now, can we? :)

This pull request is merely to address the reported bug in its context, so if the core team feels that this is not needed I am happy to close this request, but can you please close the bug as "invalid" also?

@mdomba
Member
mdomba commented May 5, 2012

you are not right for the required validator because you are not seeing the whole picture with models... but that is not the point of this issue...

Closing this pull request and issue.

@mdomba mdomba closed this May 5, 2012
@maximcherny
Contributor

I like it how your response is simply based on the assumption that someone is "not seeing the whole picture" and therefore is not right, I am sure a more constructive discussion around this would lead to a better outcome.

@mdomba
Member
mdomba commented May 5, 2012

Of course it would... but that is not the point of this github issues... for this please use the Yii forum...

Beside this, you can make a test for your assumptions with the actual code, then you would see how all this works :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment