Skip to content

Conversation

glpzzz
Copy link

@glpzzz glpzzz commented Jan 27, 2025

Q A
Is bugfix? ? (was failing when the value of the field was an enum)
New feature? ✔️
Breaks BC?
Fixed issues

If the value of the model attribute is an enum,then it fails with Object of class common\enums\Frequency could not be converted to string due to several places where the value is tried to be used directly as an string. E.g checking the selected value against the options to set the "selected" or "checked" one.

Also, the client validation of the RangeValidator was failing when the items in range are the enum cases as in:

public function rules(){
    return [
        ['attribute', 'in', 'range' => MyEnum::cases()],
    ];
}

This is work in progress yet. Trying to find other places which could require changes...

Copy link

codecov bot commented Jan 27, 2025

Codecov Report

Attention: Patch coverage is 10.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 64.83%. Comparing base (d146307) to head (17d7e68).
Report is 30 commits behind head on master.

Files with missing lines Patch % Lines
framework/validators/RangeValidator.php 0.00% 5 Missing ⚠️
framework/helpers/BaseHtml.php 20.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #20318      +/-   ##
============================================
- Coverage     64.84%   64.83%   -0.02%     
- Complexity    11433    11439       +6     
============================================
  Files           431      431              
  Lines         37187    37197      +10     
============================================
+ Hits          24115    24116       +1     
- Misses        13072    13081       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@samdark samdark added this to the 2.0.53 milestone Jan 28, 2025
$value = $value->getPrimaryKey(false);

return is_array($value) ? json_encode($value) : $value;
} elseif (version_compare(PHP_VERSION, '8.1.0') >= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary?

$var instanceof \Undefined\Class\Fqcn – it's correct expression in PHP

Copy link
Author

Choose a reason for hiding this comment

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

I basically reused the same code snippet used on yii\db\Command around line 380 which includes the PHP version check before checking for the enum type. Trying to be consistent with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use PHP_VERSION_ID >= 80100 in both places - it should be more efficient and consistent with other places.

@xepozz
Copy link
Member

xepozz commented Jan 29, 2025

In general, what about creating "enum" rule?

public function rules(){
    return [
        ['attribute', 'in', 'range' => MyEnum::cases()],

        // vs
        ['attribute', 'in', 'enum' => MyEnum::class, 'target' => 'values'], // attribute is one of enum values, default
        ['attribute', 'in', 'enum' => MyEnum::class, 'target' => 'names'], // support enum names as well

        // implement the last keys with the original approach instead
        ['attribute', 'in', 'range' => array_map(fn(MyEnum $case) => $case->name, MyEnum::cases())],
    ];
}

@glpzzz
Copy link
Author

glpzzz commented Jan 29, 2025

I don't think this:

['attribute', 'in', 'enum' => MyEnum::class, 'target' => 'values'], // attribute is one of enum values, default
['attribute', 'in', 'enum' => MyEnum::class, 'target' => 'names'], // support enum names as well

is necessary as it can be achieved with the current RangeValidator just by using an array_map as you did in the last example in your comment. That validating that the value is one of the names (string) or one of the values (string|int) of the enum.

The first one:

['attribute', 'in', 'range' => MyEnum::cases()],

validates that the actual value on the attribute is an instance of the enum, which is the intention.

The current implementation supports the enum range validation on server side. It only fails on client side as the provided values in range must be converted to string and enums can not be directly converted to strings.

@terabytesoftw terabytesoftw modified the milestones: 2.0.53, 2.0.54 Jun 25, 2025
@terabytesoftw
Copy link
Member

terabytesoftw commented Oct 15, 2025

I'm adding tests to this PR:

This works:

<?php

/**
 * @link https://www.yiiframework.com/
 * @copyright Copyright (c) 2008 Yii Software LLC
 * @license https://www.yiiframework.com/license/
 */

namespace yiiunit\data\enums;

enum StatusEnum: int
{
    case ACTIVE = 1;
    case DELETED = 2;
    case INACTIVE = 0;
}
<?php

/**
 * @link https://www.yiiframework.com/
 * @copyright Copyright (c) 2008 Yii Software LLC
 * @license https://www.yiiframework.com/license/
 */

namespace yiiunit\data\enums;

enum ColorEnum
{
    case BLUE;
    case GREEN;
    case RED;
}
/**
 * @requires PHP >= 8.1
 */
public function testGetAttributeValueWithBackedEnum(): void
{
    $model = new HtmlTestModel();
    $model->name = StatusEnum::ACTIVE;

    $this->assertSame(
        1,
        Html::getAttributeValue($model, 'name'),
        "Backed enum value should be returned by 'getAttributeValue()' method.",
    );
}

/**
 * @requires PHP >= 8.1
 */
public function testGetAttributeValueWithUnitEnum(): void
{
    $model = new HtmlTestModel();
    $model->name = ColorEnum::RED;

    $this->assertSame(
        'RED',
        Html::getAttributeValue($model, 'name'),
        "Unit enum name should be returned by 'getAttributeValue()' method.",
    );
}
    
/**
  * @requires PHP >= 8.1
  */
public function testValidateValueWithBackedEnum(): void
{
    $val = new RangeValidator(['range' => StatusEnum::cases()]);

    $this->assertTrue($val->validate(StatusEnum::ACTIVE));
    $this->assertTrue($val->validate(StatusEnum::INACTIVE));
    $this->assertFalse($val->validate(5));
}

Now the big question should the validator validate the scalar of the enum case?

/**
  * @requires PHP >= 8.1
  */
public function testValidateValueWithBackedEnum(): void
{
    $val = new RangeValidator(['range' => StatusEnum::cases()]);

    $this->assertTrue($val->validate(1)); // it should be `true`, currently it doesn't work because it compares with the case?
}

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.

5 participants