Laravel-Backpack / CRUD

Build custom admin panels. Fast!
https://backpackforlaravel.com
MIT License
3.08k stars 886 forks source link

[Bug] Backed Enums break while trying to save #4929

Closed Kovah closed 1 year ago

Kovah commented 1 year ago

Bug report

What I did

I want to add a backed enum with typed values to Crud. The model is already prepared with the corresponding casting for the field.

The enum:

<?php

namespace App\Enums;

enum Priority: int
{
    case Low = 10;
    case Medium = 20;
    case High = 30;
}

The model with casting (irrelevant info was removed):

<?php

namespace App\Models;

use App\Enums\Priority;
use Backpack\CRUD\app\Models\Traits\CrudTrait;

class RoadmapEntry extends Model
{
    use CrudTrait;

    protected $casts = [
        'priority' => Priority::class,
        // ...
    ];
}

And finally, the field for the creation form in setupCreateOperation():

$this->crud->addField([
  'name' => 'priority',
  'label' => 'Priority',
  'type' => 'enum',
  'enum_class' => Priority::class,
]);

What I expected to happen

Crud saves the value of the enum to the database.

What happened

While saving a 0 is not a valid backing value for enum "App\Enums\Priority" error popped up:

<!--
ValueError: 0 is not a valid backing value for enum "App\Enums\Priority" in file /app/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php on line 1174

#0 /app/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php(1174): App\Enums\Priority::from()
#1 /app/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php(1159): Illuminate\Database\Eloquent\Model->getEnumCaseFromValue()
#2 /app/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php(968): Illuminate\Database\Eloquent\Model->setEnumCastableAttribute()
#3 /app/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php(518): Illuminate\Database\Eloquent\Model->setAttribute()
#4 /app/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php(612): Illuminate\Database\Eloquent\Model->fill()
#5 /app/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php(1455): Illuminate\Database\Eloquent\Model->newInstance()
#6 /app/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php(985): Illuminate\Database\Eloquent\Builder->newModelInstance()
#7 /app/vendor/laravel/framework/src/Illuminate/Support/Traits/ForwardsCalls.php(23): Illuminate\Database\Eloquent\Builder->create()
#8 /app/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php(2330): Illuminate\Database\Eloquent\Model->forwardCallTo()
#9 /app/vendor/backpack/crud/src/app/Library/CrudPanel/Traits/Create.php(24): Illuminate\Database\Eloquent\Model->__call()
#10 /app/vendor/backpack/crud/src/app/Http/Controllers/Operations/CreateOperation.php(82): Backpack\CRUD\app\Library\CrudPanel\CrudPanel->create()
#11 /app/vendor/laravel/framework/src/Illuminate/Routing/Controller.php(54): App\Http\Controllers\Admin\RoadmapCrudController->store()
#12 /app/vendor/laravel/framework/src/Illuminate/Routing/ControllerDispatcher.php(43): Illuminate\Routing\Controller->callAction()
#13 /app/vendor/laravel/framework/src/Illuminate/Routing/Route.php(260): Illuminate\Routing\ControllerDispatcher->dispatch()
#14 /app/vendor/laravel/framework/src/Illuminate/Routing/Route.php(205): Illuminate\Routing\Route->runController()
#15 /app/vendor/laravel/framework/src/Illuminate/Routing/Router.php(798): Illuminate\Routing\Route->run()
#16 /app/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(141): Illuminate\Routing\Router->Illuminate\Routing\{closure}()
#17 /app/vendor/backpack/crud/src/app/Http/Controllers/CrudController.php(44): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}()
#18 /app/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(162): Backpack\CRUD\app\Http\Controllers\CrudController->Backpack\CRUD\app\Http\Controllers\{closure}()
...

It seems that Backpack already mistakes the real keys of the enum while creating the field. This is the HTML output for the priority field:

<select name="priority" class="form-control">
  <option value="0">Low</option>
  <option value="1">Medium</option>
  <option value="2">High</option>
</select>

I would assume that something like this would be correct:

<select name="priority" class="form-control">
  <option value="10">Low</option>
  <option value="20">Medium</option>
  <option value="30">High</option>
</select>

What I've already tried to fix it

I created a very simply enum like used in the Backpack documentation:

<?php

namespace App\Enums;

enum SimplePriority
{
    case LOW;
    case MEDIUM;
    case HIGH;
}

With this enum, I am able to save the entry to the database.

Is it a bug in the latest version of Backpack?

After I run composer update backpack/crud the bug is it still there. Just updated to 5.5.0.

Backpack, Laravel, PHP, DB version

When I run php artisan backpack:version the output is:

PHP 8.1.15 (cli) (built: Feb  4 2023 12:39:08) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.15, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.15, Copyright (c), by Zend Technologies

### LARAVEL VERSION:
v9.52.0@eb85cd9d72e5bfa54b4d0d9040786f26d6184a9e

### BACKPACK PACKAGE VERSIONS:
backpack/crud: 5.5.0
backpack/generators: 3.3.14
backpack/pro: 1.6.0
welcome[bot] commented 1 year ago

Hello there! Thanks for opening your first issue on this repo!

Just a heads-up: Here at Backpack we use Github Issues only for tracking bugs. Talk about new features is also acceptable. This helps a lot in keeping our focus on improving Backpack. If you issue is not a bug/feature, please help us out by closing the issue yourself and posting in the appropriate medium (see below). If you're not sure where it fits, it's ok, a community member will probably reply to help you with that.

Backpack communication channels:

Please keep in mind Backpack offers no official / paid support. Whatever help you receive here, on Gitter, Slack or Stackoverflow is thanks to our awesome awesome community members, who give up some of their time to help their peers. If you want to join our community, just start pitching in. We take pride in being a welcoming bunch.

Thank you!

-- Justin Case The Backpack Robot

Kovah commented 1 year ago

So, I have tried to understand why this happens and looked into the source. The resulting HTML for the field is built in the enum.blade.php with the important part being around line 33:

if($enumClassReflection) {
    $options = array_map(function($item) use ($enumClassReflection) {
        return $enumClassReflection->isBacked() ? [$item->getBackingValue() => $item->name] : $item->name;
    },$enumClassReflection->getCases());
    $options = is_multidimensional_array($options) ? array_merge(...$options) : array_combine($options, $options);
}

When I use my backed enum, the options array becomes a multidimensional array with the correct values. In the following line however, that multidimensional array is flattened into a simple array where the actual correct value (10,20,30 in my case) is discarded and the regular array keys (0,1,2) are used.

Of course I do not get the full picture here, but it looks like there is a mistake.

For reference: those are the option arrays for the two enum types before flattening.

SimplePriority::class

array:3 [
  0 => "LOW"
  1 => "MEDIUM"
  2 => "HIGH"
]

Priority::class

array:3 [
  0 => array:1 [
    10 => "Low"
  ]
  1 => array:1 [
    20 => "Medium"
  ]
  2 => array:1 [
    30 => "High"
  ]
]

Possible Fix

When I replace array_merge(...$options) with array_replace(...$options), I get the correct resulting array with the enum values as the array keys:

array:3 [
  10 => "Low"
  20 => "Medium"
  30 => "High"
]
tabacitu commented 1 year ago

Ouch! Thanks for the detailed report @Kovah . @pxpm could you take a look at this please? It should be faster for you to understand & fix 🙏

Kovah commented 1 year ago

Already opened a pull request with a possible fix. Hopefully this fixes it.