Skip to content
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

Is this expected behaviour on unsafe attributes validation? #20110

Open
erdosam opened this issue Jan 30, 2024 · 5 comments
Open

Is this expected behaviour on unsafe attributes validation? #20110

erdosam opened this issue Jan 30, 2024 · 5 comments
Labels

Comments

@erdosam
Copy link

erdosam commented Jan 30, 2024

What steps will reproduce the problem?

Define an unsafe attribute with multiple rules in a model.

class SimpleModel extends Model
{
    public $account_id;
    private $count = 0;

    public function rules()
    {
        return [
            ['!account_id', 'required'],
            ['account_id', function () {
                Yii::info('The first message ' .$this->count++);
            }],
            ['account_id', function () {
                Yii::info('The second message ' .$this->count++);
            }]
        ];
    }
}

Prepare the unit test using Codeception.

    public function testSimpleModel()
    {
        $model = Yii::createObject([
            'class' => SimpleModel::class,
            'account_id' => 'TheAccount',
        ]);
        $model->load([], '');
        $model->validate();
    }

Run Codeception.

vendor/bin/codecept -c common run unit SimpleModelTest::testSimpleModel --debug

What's expected?

The validation should occur once for each rule on account_id.

  [application] 'The first message 0'
  [application] 'The second message 1'

What do you get instead?

  [application] 'The first message 0'
  [application] 'The first message 1'
  [application] 'The second message 2'
  [application] 'The second message 3'

Additional info

If all instances of account_id in the rules are marked with "!", the expected behavior is produced.

...
            ['!account_id', 'required'],
            ['!account_id', function () {
                Yii::info('The first message ' .$this->count++);
            }],
            ['!account_id', function () {
                Yii::info('The second message ' .$this->count++);
            }]
...
Q A
Yii version 2.0.15.1
PHP version 5.6.40
Operating system Debian GNU/Linux 9 (stretch)
@bizley
Copy link
Member

bizley commented Jan 30, 2024

Please use latest version of Yii 2 to see if this is still a problem.

@erdosam
Copy link
Author

erdosam commented Feb 1, 2024

Sorry for late reply.
Yes. Using the latest version and this still happen. Tried with fresh installation too, and the behavior still the same.

@bizley bizley added status:to be verified Needs to be reproduced and validated. and removed status:need more info labels Feb 1, 2024
@bizley bizley transferred this issue from yiisoft/yii2-app-advanced Feb 1, 2024
@bizley bizley added type:bug Bug and removed status:to be verified Needs to be reproduced and validated. labels Feb 4, 2024
@bizley
Copy link
Member

bizley commented Feb 4, 2024

I can confirm this, although it is an effect of abusing the way the framework works (but still needs to be fixed).

Unsafe attributes (not suitable for mass loading, prefixed with !) are not meant to be defined in the rules, you should do it in the scenario definition. The list there defines all attributes available for the scenario with optional unsafe marking so you would rather not do the scenario like:

'scenario1' => ['attribute1', '!attribute1'],

because it doesn't make sense (but it's still possible).

The situation you have here is exactly this case although moved to a different place.

Since Yii is gathering all the attributes for validation, it treats attribute and !attribute as two separate attributes (with cleaning the name later on, so removing the !), and this is the reason why the validation process is called twice.

Quick fix for you is it to move the marking the attributes unsafe to scenarios() method.

As for fixing it in the framework itself I would change this line to:

$attributeNames = array_unique((array)$attributeNames);

but I'm not sure how it would affect the rest of the code (probably it would not), needs some testing.

@erdosam
Copy link
Author

erdosam commented Feb 6, 2024

"an effect of abusing the way the framework works"
"Unsafe attributes (not suitable for mass loading, prefixed with !) are not meant to be defined in the rules"

Agree. Thanks for the quick fix.
Rather than categorizing it as a bug, would it be advantageous to emphasize it in the documentation?

@bizley
Copy link
Member

bizley commented Feb 6, 2024

This is still an error. And the doc never mention it can be done in the rules ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants