yiisoft / yii2-bootstrap4

Yii 2 Bootstrap 4 Extension
https://www.yiiframework.com/
BSD 3-Clause "New" or "Revised" License
216 stars 106 forks source link

No error displayed for checkbox #173

Closed koxu1996 closed 3 years ago

koxu1996 commented 5 years ago

What steps will reproduce the problem?

Create form that require accepting checkbox. Example:

use yii\base\DynamicModel;
use yii\bootstrap4\ActiveForm;
$model = new DynamicModel(['text_field' => '', 'checkbox_field' => '']);
$model->addRule('text_field', 'required', ['requiredValue' => '1'])
        ->addRule('checkbox_field', 'required', ['requiredValue' => '1'])
        ->validate();
print_r($model->getErrors());
$form = ActiveForm::begin();
echo $form->field($model, 'text_field');
echo $form->field($model, 'checkbox_field')->checkBox();
ActiveForm::end();

What's expected?

Error message for required checkbox should be displayed.

What do you get instead?

There is no error for checkbox field: issue

However client-side validation works, so after filling form you get: cs-ok After un-filling form: cs-error

Additional info

Name Version
yiisoft/yii2 2.0.25
yiisoft/yii2-bootstrap4 2.0.7
PHP 7.3.9
ruskid commented 4 years ago

Looks like is-invalid class doesn't apply to the checkbox

ActiveField's checkbox and radio function should look like this:

public function checkbox($options = [], $enclosedByLabel = true)
    {
        if ($this->form->validationStateOn === ActiveForm::VALIDATION_STATE_ON_INPUT) {
            $this->addErrorClassIfNeeded($options);
        }

        $this->addAriaAttributes($options);
        $this->adjustLabelFor($options);

        if ($enclosedByLabel) {
            $this->parts['{input}'] = Html::activeCheckbox($this->model, $this->attribute, $options);
            $this->parts['{label}'] = '';
        } else {
            if (isset($options['label']) && !isset($this->parts['{label}'])) {
                $this->parts['{label}'] = $options['label'];
                if (!empty($options['labelOptions'])) {
                    $this->labelOptions = $options['labelOptions'];
                }
            }
            unset($options['labelOptions']);
            $options['label'] = null;
            $this->parts['{input}'] = Html::activeCheckbox($this->model, $this->attribute, $options);
        }

        return $this;
    }

Currently addErrorClassIfNeeded is called after printing the checkbox.

smigles commented 4 years ago

Also no error is for checkboxList with a custom validation rule.

The part of the model:

public function rules()
{
    return [
        ['categories', 'validateCategoryCount'],
    ];
}

public function validateCategoryCount($attribute, $params)
{
    if (!is_array($this->$attribute)) {
        $this->addError($attribute, 'Некорректное значение.');
    } elseif (count($this->$attribute) > 3) {
        $this->addError($attribute, 'Должно быть выбрано не более 3 категорий.');
    }
}

The part of the view:

<?= $form->field($model, 'categories')->checkboxList($categories) ?>

The screenshot from Firefox Page Inspector:

Gray color is for hidden elements.

alexgivi commented 4 years ago

I've fixed this bug such way:

  1. create AcriveField class, inherit from \yii\bootstrap4\ActiveField.
  2. implement checkbox method such way:

    public function checkbox($options = [], $enclosedByLabel = false)
    {
        Html::addCssClass($options, 'custom-control-input');
        if ($this->form->validationStateOn === ActiveForm::VALIDATION_STATE_ON_INPUT) {
            $this->addErrorClassIfNeeded($options);
        }
    
        return parent::checkbox($options, $enclosedByLabel); // TODO: Change the autogenerated stub
    }
  3. when create ActiveForm instance - pass 'fieldClass' config option as your new ActiveField class name.
    $form = ActiveForm::begin([
        'fieldClass' => \app\widgets\ActiveField::class,
        // ...
    ]);
samdark commented 4 years ago

@alexgivi would you like to submit a pull request?

egrekov commented 3 years ago

Yesterday I spent a day to understand why the error message was not shown until I came across this problem. Already it turns out for a year and a half people suffer and make crutches, but the problem has not been solved. Hmm, very sad.

samdark commented 3 years ago

@egrekov that's OpenSource. If there's a pull request fixing it that has unit tests, we merge it right away after a short code review.

egrekov commented 3 years ago

@samdark Well, it's fixed :)

bizley commented 3 years ago

And yet, without unit tests ;) Could you please add some?

egrekov commented 3 years ago

@bizley, some problem in the assembly of tests


# docker login
Authenticating with existing credentials...
WARNING! Your password will be stored unencrypted in /root/.docker/config.json.
Configure a credential helper to remove this warning. See
https://docs.docker.com/engine/reference/commandline/login/#credentials-store

Login Succeeded

# make test
test -d tests/docker || git clone https://github.com/cebe/jenkins-test-docker tests/docker
cd tests/docker && git checkout -- . && git pull
Уже обновлено.
mkdir -p tests/dockerids
cd tests/docker/php && sh build.sh
building base docker image for php.
Sending build context to Docker daemon  6.144kB
Step 1/10 : FROM debian:wheezy
 ---> 10fcec6d95c4
Step 2/10 : RUN   DEBIAN_FRONTEND=noninteractive apt-get update &&   DEBIAN_FRONTEND=noninteractive apt-get upgrade -y &&   DEBIAN_FRONTEND=noninteractive apt-get install -y git curl vim build-essential pkg-config autoconf bison re2c libxml2-dev
 ---> Running in 20c0c5134467
Ign http://security.debian.org wheezy/updates Release.gpg
Ign http://deb.debian.org wheezy Release.gpg
Ign http://security.debian.org wheezy/updates Release
Ign http://deb.debian.org wheezy-updates Release.gpg
Err http://security.debian.org wheezy/updates/main amd64 Packages

Err http://security.debian.org wheezy/updates/main amd64 Packages

Ign http://deb.debian.org wheezy Release
Err http://security.debian.org wheezy/updates/main amd64 Packages

Ign http://deb.debian.org wheezy-updates Release
Err http://security.debian.org wheezy/updates/main amd64 Packages

Err http://security.debian.org wheezy/updates/main amd64 Packages
  404  Not Found [IP: 151.101.2.132 80]
Err http://deb.debian.org wheezy/main amd64 Packages
  404  Not Found
Err http://deb.debian.org wheezy-updates/main amd64 Packages
  404  Not Found
W: Failed to fetch http://security.debian.org/debian-security/dists/wheezy/updates/main/binary-amd64/Packages  404  Not Found [IP: 151.101.2.132 80]

W: Failed to fetch http://deb.debian.org/debian/dists/wheezy/main/binary-amd64/Packages  404  Not Found

W: Failed to fetch http://deb.debian.org/debian/dists/wheezy-updates/main/binary-amd64/Packages  404  Not Found

E: Some index files failed to download. They have been ignored, or old ones used instead.
The command '/bin/sh -c DEBIAN_FRONTEND=noninteractive apt-get update &&   DEBIAN_FRONTEND=noninteractive apt-get upgrade -y &&   DEBIAN_FRONTEND=noninteractive apt-get install -y git curl vim build-essential pkg-config autoconf bison re2c libxml2-dev' returned a non-zero code: 100
building docker for php php-5.6.8.
Sending build context to Docker daemon  12.29kB
Step 1/8 : FROM yiitest/php-base:latest
pull access denied for yiitest/php-base, repository does not exist or may require 'docker login'
Makefile:20: ошибка выполнения рецепта для цели «docker-php»
make: *** [docker-php] Ошибка 1```
bizley commented 3 years ago

Is this local docker? I'm not sure how to help you here. This part of code should be ok to test with php only, without any external services anyway.

egrekov commented 3 years ago

@bizley Is this local docker?

Yes local docker, looked 7 debians are used in tests, it is already deprecated. You tried to run tests on your own, it seems to me that they won't start either.

egrekov commented 3 years ago

@bizley Once again I looked at why the test is not being collected, it turns out that the repository is missing yiitest/php-base

egrekov commented 3 years ago

Are the tests passed? image

bizley commented 3 years ago

Yes, tests are passing which means nothing is broken but is there a test checking what you just added?

egrekov commented 3 years ago

Yes, tests are passing which means nothing is broken but is there a test checking what you just added?

root@516debc04af8:/opt/app# vendor/phpunit/phpunit/phpunit --filter ActiveFieldTest
PHPUnit 4.8.34 by Sebastian Bergmann and contributors.

Runtime:        PHP 7.3.27
Configuration:  /opt/app/phpunit.xml.dist

.............

Time: 2.62 seconds, Memory: 8.00MB

OK (13 tests, 13 assertions)