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

fix/field-resolve-error: fix error exception for non BackedEnum when n… #4

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

korobkovandrey
Copy link
Contributor

ErrorException when nova update-fields request for simple UnitEnum (not BackedEnum):
Undefined property: App\Enums\Status::$value

@korobkovandrey korobkovandrey changed the title fix/field-resolve-error: fix error exeption for non BackedEnum when n… fix/field-resolve-error: fix error exception for non BackedEnum when n… Nov 9, 2023
if($value instanceof BackedEnum){
return $value->value;
}
return $value instanceof UnitEnum ? $value->name : $value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, can you elaborate why in case of an UnitEnum the property value is no longer returned but name?

Do you have a code sample "before" "after" to highlight the necessity of this change?

Did you make sure it doesn't break the current behavior?

Thanks.

Copy link
Contributor Author

@korobkovandrey korobkovandrey Nov 15, 2023

Choose a reason for hiding this comment

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

Hi, a simple UnitEnum does not value, its value is name. The current version does not support them. This change makes it possible to use them. Also, the JsonSerializable interface needs to be implemented for use in laravel.
Example:

// simple UnitEnum, instance of UnitEnum
enum Status implements \JsonSerializable
{
    use LaravelEnumHelper;

    case ENABLED;
    case DISABLED;

    // need for laravel responses
    public function jsonSerialize(): string
    {
        // this condition is not needed here, the code is for common use (in a trait)
        if($this instanceof BackedEnum){
            return $this->value;
        }
        return $this->name;
    }
}

echo Status::ENABLED->value;

//result: PHP Warning:  Undefined property: Status::$value

// in model:
    protected $casts = [
        'status' => Status::class,
    ];

My change doesn't change anything for BackedEnum.

// instance of StringBackedEnum > BackedEnum > UnitEnum
enum StatusBackedString: string
{
    use LaravelEnumHelper;

    case ENABLED = 'enabled';
    case DISABLED = 'disabled';
}

// instance of IntBackedEnum > BackedEnum > UnitEnum
enum StatusBackedInt: int
{
    use LaravelEnumHelper;

    case ENABLED = 1;
    case DISABLED = 2;
}

@sergiu-petrica
Copy link

sergiu-petrica commented Nov 24, 2023

The readme of this package states that pure enums are supported. This isn't the case as any attempt to use a pure enum results in an Undefined property: $value error as described in this PR.

You guys should either merge this PR, or update the readme so it clearly states pure enums aren't supported.

@trippo trippo merged commit e41ffb7 into datomatic:main Apr 3, 2024
1 check passed
@trippo
Copy link
Contributor

trippo commented Apr 3, 2024

You are right

@trippo
Copy link
Contributor

trippo commented Apr 3, 2024

Sorry for delay

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.

4 participants