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

PHP Enums support #4678

Merged
merged 22 commits into from
Oct 17, 2022
Merged

PHP Enums support #4678

merged 22 commits into from
Oct 17, 2022

Conversation

pxpm
Copy link
Contributor

@pxpm pxpm commented Sep 21, 2022

WHY

BEFORE - What was wrong? What was happening before this PR?

A powerfull introduction in 8.1 was Enums. Backpack was completely un-ware of them until now.

It was not possible use a field casted to an Enum::class.

See the following example, a little adaptation of the Article model and ArticleCrudController:

// the Enum class
enum StatusEnum: string
{
    case DRAFT = 'DRAFT';
    case PUBLISHED = 'PUBLISHED';

    public static function getOptions()
    {
        return array_combine(array_column(self::cases(), 'name'), array_column(self::cases(), 'value'));
    }
}

// In the Article model:

protected $casts = [
        'featured'  => 'boolean',
        'date'      => 'date',
        'status'    => StatusEnum::class
    ];

// In ArticleCrudController - adapting our field to use the enum:

$this->crud->addField([
                'name' => 'status',
                'label' => 'Status',
                'type' => 'select_from_array',
                'options' => StatusEnum::getOptions(),
            ]);

AFTER - What is happening after this PR?

After this PR it works as expected.

HOW

How did you achieve that, in technical terms?

Basically added a layer between getting and returning the models attributes values.
That layer will check if Enums is a thing (only available in 8.1), if they are check if the model attribute is any kind of Enum, in case it is, return the appropriate string representation of it.

NOTE For the List/ShowOperation i started from #4633 and added support for text and select_from_array columns (probably the ones that make sense to use with an Enum ? )

Is it a breaking change?

I don't think so, no.

How can we test the before & after?

You can use the example I gave on this issue description.

@ziming
Copy link
Contributor

ziming commented Sep 21, 2022

Clicking Edit to show the edit form get me Backpack\CRUD\app\Library\CrudPanel\CrudPanel::getAttributeFinalValue(): Argument #1 ($attribute) must be of type string, null given

@pxpm
Copy link
Contributor Author

pxpm commented Sep 21, 2022

Clicking Edit to show the edit form get me Backpack\CRUD\app\Library\CrudPanel\CrudPanel::getAttributeFinalValue(): Argument #1 ($attribute) must be of type string, null given

Should be good now @ziming I will write some tests for it 👍

@ziming
Copy link
Contributor

ziming commented Sep 21, 2022

Looks good so far. but I have only tested with backed enums. From the code it does look like it will work for Non-backed Enum too

pxpm and others added 2 commits September 21, 2022 15:56
pxpm and others added 5 commits September 21, 2022 16:47
… into enum-support

# Conflicts:
#	tests/Unit/CrudPanel/CrudPanelUpdateTest.php
#	tests81/Unit/Models/ArticleWithEnum.php
#	tests81/Unit/Models/Enums/StatusEnum.php
[ci skip] [skip ci]
[ci skip] [skip ci]
@pxpm
Copy link
Contributor Author

pxpm commented Sep 23, 2022

I assigned @jorgetwgroup for testing purposes, and @tabacitu for implementation decisions.

We can also create a select_from_enum column/field that would avoid developer having to setup the options manually.


public static function getOptions()
{
return array_combine(array_column(self::cases(), 'name'), array_column(self::cases(), 'value'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return array_combine(array_column(self::cases(), 'name'), array_column(self::cases(), 'value'));
return array_column(self::cases(), 'value', 'name');

The resulting array will be

[
    'NAME_1' => 'Name 1 Value',
    'NAME_2' => 'Name 2 Value',
]

Copy link
Member

@tabacitu tabacitu left a comment

Choose a reason for hiding this comment

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

Pedro an I have decided to add enum support a different way - by creating enum field and column (actually adding functionality to the enum field but you know what I mean).

That should make this change super-non-breaking, with zero touches to the core 💪

@ziming
Copy link
Contributor

ziming commented Sep 27, 2022

there is already a database enum field in backpack so are you all going to call it php enum to differentiate it?

edit: ah i see so now the enum field support both database and php enum

@pxpm
Copy link
Contributor Author

pxpm commented Sep 27, 2022

there is already a database enum field in backpack so are you all going to call it php enum to differentiate it?

edit: ah i see so now the enum field support both database and php enum

No zimming, the enum field should work for both enum types (PHP and database) I already did that change in this PR, I am finishing now the enum column too.

@pxpm pxpm assigned jcastroa87 and unassigned pxpm Sep 29, 2022
@pxpm
Copy link
Contributor Author

pxpm commented Sep 29, 2022

@ziming I think it's now finished, tests are passing and I added the column too.

If you give this a test and found something let me know.

I am now going to write the docs for it.

Cheers

@pxpm
Copy link
Contributor Author

pxpm commented Oct 2, 2022

Added docs for field and column Laravel-Backpack/docs#379

@ziming
Copy link
Contributor

ziming commented Oct 4, 2022

Looking forward to this PR. Hopefully it gets back ported to laravel 8 too. Which sometimes happen

laravel/framework#44445

@jcastroa87
Copy link
Member

Hello @pxpm thank to you because in this revision everything works as we speak about it.

I will go step by step.

Define the Enum
Screen Shot 2022-10-10 at 20 52 14

Set Model cast
Screen Shot 2022-10-10 at 20 52 21

Add column enum type
Screen Shot 2022-10-10 at 20 52 55

Front
screenshot-backpack test-2022 10 10-20_53_21

Add field "using the same function as column"
Screen Shot 2022-10-10 at 20 53 03

Front
screenshot-backpack test-2022 10 10-20_53_10

On my side everything is working as expected.

About code for me look everything ok, just a warning for @tabacitu this required PHP 8.1 to work normal.

Cheers.

@jcastroa87 jcastroa87 assigned tabacitu and unassigned jcastroa87 Oct 10, 2022
@tabacitu tabacitu merged commit ebbe9a1 into main Oct 17, 2022
@tabacitu tabacitu deleted the enum-support branch October 17, 2022 10:33
@tabacitu
Copy link
Member

Excellent work on this one 👏👏👏 Testing and merging it was smooth sailing too. So so happy with it.

slow_clap_cap

Merged! Tagging later today as part of the CRUD 5.4 minor release 🎉

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

Successfully merging this pull request may close these issues.

5 participants