spatie / laravel-enum

Laravel support for spatie/enum
https://spatie.be/open-source
MIT License
344 stars 37 forks source link

BadMethodCallException with numerical enum values #57

Closed djam90 closed 3 years ago

djam90 commented 4 years ago

I have a UserTypeEnum with 2 values, Provider and Client. I had already created the database schema with tiny integers for this field so I wanted to set the values to be numerical.

I could not see an example in the documentation for this package but I saw an example of using numbers in the base enum package.

My class is below. When I call the request transformation function, it throws a BadMethodCallException which I think is because of the strict comparison between values. My form is a simple POST form which is submitting a string "1" which does not match the numerical 1 in the values array.

The error is as follows:

BadMethodCallException There's no value 1 defined for enum App\Enums\UserTypeEnum, consider adding it in the docblock definition.

Enum example:

/**
 * @method static self PROVIDER()
 * @method static self CLIENT()
 */
final class UserTypeEnum extends Enum
{
    protected static function values(): array
    {
        return [
            'PROVIDER' => 1,
            'CLIENT' => 2,
        ];
    }
}

This is how I am calling the transformEnums method.

$request->transformEnums($this->enumCastRules());

Below is the enum cast rules I am using.

private function enumCastRules()
{
    return [
        'type' => UserTypeEnum::class,
    ];
}

My form field:

<input type="radio" name="type" id="type_provider" value="{{ \App\Enums\UserTypeEnum::PROVIDER() }}">
Gummibeer commented 4 years ago

Interesting and good point! πŸ€” I'm thinking about a solution as dropping the strict comparison in the base package is no solution. So we have to find a way to transform the value before it's passed to the base package.

My favorite would be "magic" that transforms the request value based on the validation rule.

Some questions:

Right now I think/fear that this will only be automatically fixed for FormRequest as that's the only place in which we have access to validation rules, request data and enum casts and can also manipulate the data. I wonder if there should be a general Laravel solution that transforms request data to the corresponding scalar type if rules like integer are applied. πŸ€”

djam90 commented 4 years ago

I hadn't used a FormRequest because this is a field I have added to the standard Laravel Registration controller.

I added the "type" field to the validation rules in the RegistrationController:

    protected function validator(array $data)
    {
        return Validator::make($data, [
            'name' => ['required', 'string', 'max:255'],
            'email' => ['required', 'string', 'email', 'max:255', 'unique:users'],
            'password' => ['required', 'string', 'min:8', 'confirmed'],
            'username' => ['required', 'string', 'max:255', 'unique:users'],
            'type' => ['required', 'enum:' . UserTypeEnum::class],
        ]);
    }

I also added the field to the create method:

    protected function create(array $data)
    {
        return User::create([
            'name' => $data['name'],
            'email' => $data['email'],
            'password' => Hash::make($data['password']),
            'username' => $data['username'],
            'type' => $data['type']
        ]);
    }

And I wasn't sure what else needed adding, so I copied the register function from the RegistersUsers trait, and added $request->transformEnums($this->enumCastRules()); to it.

    public function register(Request $request)
    {
        $request->transformEnums($this->enumCastRules());

        $this->validator($request->all())->validate();

        event(new Registered($user = $this->create($request->all())));

        $this->guard()->login($user);

        if ($response = $this->registered($request, $user)) {
            return $response;
        }

        return $request->wantsJson()
            ? new JsonResponse([], 201)
            : redirect($this->redirectPath());
    }

Thanks

djam90 commented 4 years ago

Also even without that $request->transformEnums($this->enumCastRules()); it gives me an error with the validation rule, presumably for the same reason, that "1" is not 1 which causes it to fail. The validation message I get is:

validation.enum

Even if I remove the validation rule, it fails when trying to save the user due to the cast added on the User model:

    protected $casts = [
        'email_verified_at' => 'datetime',
        'type' => UserTypeEnum::class,
    ];

So it looks like at the moment I can only use the enum with string values.

Gummibeer commented 4 years ago

You can use integers - but have to properly cast the raw value before it's passed to the enum. That's the step I think about how to solve but I fear that there's not an easy solution.

djam90 commented 4 years ago

Ah you are correct, I cast the raw value on the request to be an integer first and it worked now.

Gummibeer commented 4 years ago

Glad the quick fix/workaround works. πŸŽ‰ The package provided one will take some more time.

kenhagan commented 3 years ago

I've had the BadMethodCallException issue as well, but under a slightly different circumstance.

I store the enum values as integers in db and cast to enum in the model. All works well in mysql, but sqlite/pdo returns my integers as strings when I test, which causes the exception. My solution is to use mysql during tests, but that is obviously less than ideal. This is a clearly a sqlite/pdo limitation, but thought I should add to the conversation.

Gummibeer commented 3 years ago

Thanks for adding! Seems like we need to do some value casting before we pass it to the strict enum code. Potentially even override the make() method for Laravel. πŸ€”

kenhagan commented 3 years ago

I believe enum values can be numbers, so that might break stuff. Am I wrong? Could we set an attribute in the Enum class to force integer casting. What do you think?

Gummibeer commented 3 years ago

@brendt seems like we will have to do/add a weak make() method. πŸ€” The question is if we add it to the base or Laravel wrapper? I think that we should add a re-try like is_numeric() ? intval() : $val.

brendt commented 3 years ago

I think it belongs in the Laravel package, what's your opinion?

goyote commented 3 years ago

I had to do this nasty hack on top of the action controller to get it working:

$request->merge(['status' => (int) request('status')]);

dacopavlov commented 3 years ago

If you are using a Form Request, overload the method validationData like this:

    public function validationData()
    {
        $this->merge([
            'status'=> (int)$this->status,
            ]);

        return $this->all();
    }
thijsvdanker commented 3 years ago

I have used Rule::in(StatusEnum::toValues())

artworkad commented 3 years ago

I've had the BadMethodCallException issue as well, but under a slightly different circumstance.

I store the enum values as integers in db and cast to enum in the model. All works well in mysql, but sqlite/pdo returns my integers as strings when I test, which causes the exception. My solution is to use mysql during tests, but that is obviously less than ideal. This is a clearly a sqlite/pdo limitation, but thought I should add to the conversation.

@kenhagan just came across the same issue. Did you manage to solve this other than having a dedicated mysql instance for testing?

kenhagan commented 3 years ago

I've had the BadMethodCallException issue as well, but under a slightly different circumstance. I store the enum values as integers in db and cast to enum in the model. All works well in mysql, but sqlite/pdo returns my integers as strings when I test, which causes the exception. My solution is to use mysql during tests, but that is obviously less than ideal. This is a clearly a sqlite/pdo limitation, but thought I should add to the conversation.

@kenhagan just came across the same issue. Did you manage to solve this other than having a dedicated mysql instance for testing?

@artjomzab No, I did not solve this. I still use mysql for testing. Running tests in parallel partially solves the problem, but I have to re-seed for manual testing if I run an individual test.

audunru commented 3 years ago

I have the same problem with sqlite which is used during tests, now trying this as a workaround:

/**
 * @method static self Foo()
 * @method static self Bar()
 * @method static self Thing()
 * @method static self OtherThing()
 * @method static self OneMoreThing()
 */
class ExampleEnum extends Enum
{
    protected static function values(): array
    {
        return [
            'Foo'   => 1,
            'Bar'    => 2,
            'Thing'    => 3,
            'OtherThing' => 4,
            'OneMoreThing'  => 5,
        ];
    }

    public function __construct($value)
    {
        // https://github.com/spatie/laravel-enum/issues/57
        if (is_string($value) && in_array((int) $value, $this->toValues(), strict: true)) {
            $value = (int) $value;
        }
        parent::__construct($value);
    }
}
Gummibeer commented 3 years ago

I will take care of this one today. #hacktoberfest πŸŽƒ

Gummibeer commented 3 years ago

Will be solved in the base enum package as it seems that PHP type-juggling applies to enum values as well. Bildschirmfoto 2021-10-20 um 09 49 30

Gummibeer commented 3 years ago

https://github.com/spatie/enum/releases/tag/3.10.0

Gummibeer commented 3 years ago

There won't be a new release in this package as a simple composer update will automatically upgrade the base enum package and if you want to ensure that version you can add a spatie/enum: ^3.10 to your own composer.json file.

audunru commented 3 years ago

Can confirm that after upgrading to spatie/enum 3.10 I no longer need to override the Enum class' constructor. Thanks @Gummibeer !

Gummibeer commented 3 years ago

Happy to hear! πŸŽ‰