Fix for typed attributes in BaseHtml::activeListInput()

[WinterSilence]

Q	A
Is bugfix?	✔️
New feature?	❌
Breaks BC?	❌
Fixed issues	#19446

[yii-bot]

Thank you for putting effort in the improvement of the Yii framework.
We have reviewed your pull request.

In order for the framework and your solution to remain stable in the future, we have a unit test requirement in place. Therefore we can only accept your pull request if it is covered by unit tests.

Could you add these please?

Thanks!

P.S. If you have any questions about the creation of unit tests? Don't hesitate to ask for support. More information about unit tests

This is an automated comment, triggered by adding the label pr:request for unit tests.

[WinterSilence]

@samdark test added

[rob006]

This does not fix this error, it hides it (unselect no longer works).

I don't think it is worth to fix #19446, since by design you should not use typed propertied (or setters) for form models. In Yii 2 validation has 2 steps:

1. assign untrusted data to model
2. validate model properties

Therefore, your model must accept any data type for validation to work.

[WinterSilence]

@rob006

This does not fix this error, it hides it (unselect no longer works).

"unselect" not works when user don't want it and strictly declare attribute/property as array. this method should have created hidden input for each checkbox to keep data type.

since by design you should not use typed propertied (or setters) for form models

Nonsense, by your logic, adaptation to PHP8 is also not needed. 10 years ago typing was just beginning to be introduced into PHP - it's not "desing", just wasn't any other solutions to data typing.

[MarkoNV]

@WinterSilence maybe it would be more appropriate to handle this by load() method. $_POST doesn't have any mean to send empty array, so to be able to set variable to empty array we need to send empty string. load() could handle this either by reflection (if string -> set empty string, if non-nullable array -> set empty array, if nullable non-string set null); or by manualy defined casting rules (e.g. filters() method which would behave similarly to rules() but supporting only filters, no errors, and executing before setting value to property).

[WinterSilence]

@MarkoNV Yii 2 is freezed i.e. just fixes are allowed

[MarkoNV]

@WinterSilence I think we had disagreement about cause of #19446.

Your view is that setting $options['unselect'] = ''; is issue. But I disagree, I have to agree with @rob006 here. Any property which is intended to be populated from $_POST or $_GET should accept any value that can be sent by $_POST and/or $_GET method without exception. If value isn't valid, that should be solved by filters or rejected by validators.

Solution would be:

• casting on load on framework level; or
• documenting fact that typed attributes are not appropriate for $_POST or $_GET data without casting/filtering on model (project) level

If we conclude that first solution would be new feature, we end up on what @rob006 said.

Also, your solution is BC break. You are changing behavior of BaseHtml::activeListInput() for models which implement casting on load or use proxy properties for casting.

Example casting/filtering on load:
Model:

class Form extends \yii\base\Model
{
    public array $values = [];

    public function setAttributes($values, $safeOnly = true)
    {
        if (empty($values['values']) && array_key_exists('values', $values)) {
            $values['values'] = [];
        }
        parent::setAttributes($values, $safeOnly);
    }

    public function rules()
    {
       return [
        [['values'], 'safe']
      ];
    }
}

This example is valid project code, works with current implementation and allows unsetting properties with default BaseHtml::activeListInput() options. After your bugfix, $options['unselect'] = ''; would need to be set explicitly to get same behavior.

Example proxy property:
Model:

class Form extends \yii\base\Model
{
    public array $values = [];

    public function setFormValues($values)
    {
        if (empty($values)) {
            $this->values = [];
        }
        elseif (is_array($values)) {
            $this->values = [$values];
        }
        // additional code if needed
    }

    public function rules()
    {
       return [
        [['formValues'], 'safe']
      ];
    }
}

View:

use \yii\widgets\ActiveForm;
$form = ActiveForm::begin();
echo $form->field($model, 'values[]')->checkboxList([1 => 'foo', 2 => 'bar'], [
   'name' => Html::getInputName($model, 'formValues')
]);
echo \yii\widgets\Html::submitButton();
ActiveForm::end();

This is also valid project code. FormField allows to set name and to submit form value to other attribute or setter. Value is loaded from attribute $values, but it's posted as 'formValues' which is set only proxy property. Your PR would try to set $model->values = '', that would fail and default behavior would be changed.

As I said, that should be solved by casting on loading data. Solving it on framework level would be nice, but if you consider it new feature, then only documentation should be changed explaining limitations, consequences and possible solutions for typed attributes.

[WinterSilence]

@MarkoNV

Also, your solution is BC break.

nope, in this case the old version throws an exception

empty($values['values']) && array_key_exists('values', $values) no reasons to run array_key_exists() after empty()

[MarkoNV]

@WinterSilence

nope, in this case the old version throws an exception

Example from bug report throws exception because property doesn't accepts all possible values from user.

In my two examples, there is no exception because model implementation handles accepting values from user and by default there is possibility to unselect, as is per documentation. After your PR, ability to unselect is by default disabled unless manually enabled, which is BC break.

empty($values['values']) && array_key_exists('values', $values) no reasons to run array_key_exists() after empty()

This submission should set property to empty value:

$data = [
    'id' => '1',
    'values' => '',
];

but this submission shouldn't change property $values since it isn't submitted:

$data = [
    'id' => '1',
];

Both submissions match empty(), but only first one matches array_key_exists().