[Feature] Conditionable trait

[maurohmartinez]

WHY

Everything about it here.
Credits to @ziming

PR for docs here.

---

I propose to add it to:

• CrudColumn
• CrudButton
• CrudField already done on this PR
• CrudFilter
• CrudPanel to do things like $this->crud->when($condition, function (CrudPanel $crud) {...}. I think it's useful since very often we set values for settings conditionally.

Sample of a crud field to use in docs:

// If $condition is met, let's add a field
$this->crud->when($condition, function (CrudPanel $crud) {
    $crud->field('email')
        ->label('Email Address')
        ->type('email')
        // Unless $anotherCondition is met, let's add a hint!
        ->unless($anotherCondition, function (CrudField $field) {
            $field->hint('My hint')
            // When the operation is create, let's move the field after name
            ->when($this->crud->getOperation() === 'create', function (CrudField $field) {
                $field->after('name');
            });
        })
        // Finally, if the operation is update, let's put this field into a tab!
        ->when($this->crud->getOperation() === 'update', function (CrudField $field) {
            $field->tab('My tab');
        });
});

How about documenting this here?

[tabacitu]

How about documenting this here?

Sounds good to me!

I propose to add it to:

• CrudColumn
• CrudField already done on this PR
• CrudFilter

CrudPanel to do things like $this->crud->when($condition, function (CrudPanel $crud) {...}. I think it's useful since very often we set values for settings conditionally.

Hmm I don't understand why this would be useful to be honest... a when() or unless() on the main CRUD object would be the same as a general-purpose conditional in my eyes...

// this
$this->crud->when($condition, function (CrudPanel $crud) {
    $crud->field('email')
        ->label('Email Address')
        ->type('email')
        // Unless $anotherCondition is met, let's add a hint!
        ->unless($anotherCondition, function (CrudField $field) {
            $field->hint('My hint')
            // When the operation is create, let's move the field after name
            ->when($this->crud->getOperation() === 'create', function (CrudField $field) {
                $field->after('name');
            });
        })
        // Finally, if the operation is update, let's put this field into a tab!
        ->when($this->crud->getOperation() === 'update', function (CrudField $field) {
            $field->tab('My tab');
        });
});

// is the same as doing
if ($condition) {
    $crud->field('email')
        ->label('Email Address')
        ->type('email')
        // Unless $anotherCondition is met, let's add a hint!
        ->unless($anotherCondition, function (CrudField $field) {
            $field->hint('My hint')
            // When the operation is create, let's move the field after name
            ->when($this->crud->getOperation() === 'create', function (CrudField $field) {
                $field->after('name');
            });
        })
        // Finally, if the operation is update, let's put this field into a tab!
        ->when($this->crud->getOperation() === 'update', function (CrudField $field) {
            $field->tab('My tab');
        });
}

Or maybe the example wasn't the best, and there's a real use case for $this->crud->unless() and $this->crud->when()?

[tabacitu]

When this is ready, please

• ask Pedro for his opinion (ping on Discord too), then
• hand it over to Jorge to test (ping on Discord too)

[maurohmartinez]

Hmm I don't understand why this would be useful to be honest... a when() or unless() on the main CRUD object would be the same as a general-purpose conditional in my eyes...

Yeah, probably not needed. I thought it was going to clean up a bit the code.... but it turns out we save only one line (although it still looks cleaner to me).

What do you think @pxpm ? Can there be any use of adding Conditionable to CrudPanel ?

[pxpm]

Hey guys.

I think "at the present moment", there is utility for it in the CrudPanel, since we have a lot of methods that work with fields/columns etc and return the CrudPanel object and not the CrudField or CrudColumn objects. But..

I am not sure if we want to encourage people to use those methods, as the plan is to move those methods out of CrudPanel in the future.

We want to encourage the usage of our class objects like CrudField::name('smt') and less the crud panel object functions like CRUD::addField()->when(blabla).

I don't agree with @tabacitu when he says "it's only one less line". Basically it's only less one line for pretty much everything, it's just a conditional after all. In your example I agree with @maurohmartinez that it looks better without the if.

My only concern is adding it at this point to the CrudPanel, so maybe the @tabacitu example should be changed to:

CrudField::name('email')
        ->label('Email Address')
        ->type('email')
        // Unless $anotherCondition is met, let's add a hint!
        ->unless($anotherCondition, function (CrudField $field) {
            $field->hint('My hint')
            // When the operation is create, let's move the field after name
            ->when($this->crud->getOperation() === 'create', function (CrudField $field) {
                $field->after('name');
            });
        })
        // Finally, if the operation is update, let's put this field into a tab!
        ->when($this->crud->getOperation() === 'update', function (CrudField $field) {
            $field->tab('My tab');
        });

But still, what if you want this field only if user is admin ?

Maybe we can overwrite the unless and when methods on CrudField etc classes, and make them also be constructors of those classes ? So that we can do:

// only add this field when the user is admin
CrudField::when(backpack_user()->hasRole('admin'))
        ->name('email')
        ->label('Email Address')
        ->type('email');

See ? one less if, no mention of CrudPanel. Just beautiful! 👀

[maurohmartinez]

@pxpm I must say I love the syntax of your last example!

My question is, in case the condition is not met, how do we handle it? If we return null in the when method that functions as a constructor, the subsequent chained methods will get executed on null throwing an exception. What was your idea to handle that?

[tabacitu]

I am not sure if we want to encourage people to use those methods, as the plan is to move those methods out of CrudPanel in the future. We want to encourage the usage of our class objects like CrudField::name('smt') and less the crud panel object functions like CRUD::addField()->when(blabla).

Ouch! Good catch Pedro. Ok it's settled, no conditionable on CrudPanel.

My question is, in case the condition is not met, how do we handle it? If we return null in the when method that functions as a constructor, the subsequent chained methods will get executed on null throwing an exception. What was your idea to handle that?

🎰 Ding ding ding ding! It might not be possible.

But... and I will hate myself for saying this... it also might be possible 😅 - Laravel has the optional() helper which allows you to do stuff like optional($user)->someMethodOnTheUser() so we can take inspiration from that. Or even use the Optional object it uses behind the scenes. But. BUT! This is making the scope of this feature baloon, which is a BIG no-no. This is no longer "a quick win". It becomes exactly the kind of feature that I tell Pedro NOT to do, then he goes ahead and does it. Then we spend weeks polishing it, so it ends up taking a lot more of our attention than we anticipated, because we have to deal with decisions and problems we didn't anticipate. So I'll go right ahead and say it: please Pedro don't do it! I will not talk about balooning this. I do not want to hear about it, we have better stuff to think about right now.

At this moment... in v5... we either implement Conditional on some stuff... which is a simple thing to do... or we don't do anything at all. I'm serious. Let's focus on MUSTs in v6 please. If we want to change the scope of this in the future, fine, open an issue about it, we can think about that later. Right now, what's urgent is to get a few features out the door and move everyone to v6. This is a COULD-DO - let's act like it.

[ziming]

Hmm as the original PR creator, here is my 2 cents

• In v6 which is PHP 8 minimum, when() & unless() could be less useful since on PHP 8 you can define your arguments & return types to be of multiple types like

function rules(array|callable|null $abc): ?array {
    // 1-2 famous laravel libraries used this to great effect
}

But in v5 where the minimum PHP is 7.3, when() & unless() is very useful. (Could be in v6 too for things that don't have a dedicated method yet.

• The place I find chaining most useful are places where it can be troublesome to do if conditionals, such as in an array

return [
  // if (abc) { here? oh no! fluent methods would be awesome here!
]

As a side note, in my 1 backpack admin site I still define my backpack fields the original way (the non fluent way) because I'm used to it (the original non fluent way). So it's a bit funny I made this PR when I'm generally not a practitioner of the fluent way in backpack. 🤣

• Lastly sometimes something may not make complete sense (like some edge cases where it could go wrong), but to give the overall API a consistent feel, you may add to it (Some eloquent & query builder fluent methods are sort of like this where at certain edge cases they breakdown but most people won't hit them or they just hit them and make a mental note to themselves that it will cause an error because of XYZ & move on).

But again, this is a tricky topic & an art, so if you are divided about it, can choose not to add to CrudPanel 1st and decide again later.

[pxpm]

@pxpm I must say I love the syntax of your last example!

I love it too, if we can think of a way to make it work ... it's sugar!

Instead of making it a constructor, we can make a Facade for CrudField, so something like this would work just as expected: FieldFacade::when(false, function($field) {})->dontRaiseErrorsIfWhenFails()->neitherThisOne()

It's exactly the premise of when, if the conditional fails, the application don't keep executing the other methods.

Let's leave this for a future talk or @tabacitu is going to have a stroke now, I will give my shoot at this because sometimes I got lucky 🍀 🙃 👍

@tabacitu the way I see it, at this moment what we are adding is: a way for developers to conditionally set the attributes on crud objects, we are not really making the class (CrudField) itself conditionable.

I think the point of Conditionable is to make full classes conditionable, see laravel/framework#46259 for example about the Log class.

CrudField::when($isAdmin, function($field) {
    $field->name('my_admin_field')->unless(...)
})

// V.S
if($isAdmin) {
    CrudField::name('my_admin_field')->unless(...)
}

We are adding only the support for the second example, seems weird having if and then a conditionable class call, but for this PR to don't de-rail into something bigger I am ok if we settle on what we are adding now, the support for conditionable attributes and nothing more.

I would still not add this to CrudPanel, at least for now. Once added can't be removed, and we plan to mess with that class in the future.
I know it wouldn't help much @ziming given his use-case, but CrudPanel is macroable, so creating the unless and when functionality shouldn't be prohibitive for people that want it. It's lame, but it is what it is 🤷

Nobody ever asked for this, so let's keep it simple and contained.

Cheers

[tabacitu]

Let's leave this for a future talk or @tabacitu is going to have a stroke now

Nobody ever asked for this, so let's keep it simple and contained.

☝️

[maurohmartinez]

@tabacitu @pxpm
So, I removed it from CrudPanel ... are we ready now? 😆

[tabacitu]

Ready from my point of view! But I'll let @pxpm double-check, merge and release this - 5.x is in his court. Feel free to ping him if he doesn't reply soon 😅 We can call it 5.6.0 I think - it's a feature... 🤷‍♂️

[pxpm]

I agree with @tabacitu calling it 5.6.0, but I'd rather wait for #5043 to tag it.

Let me know if I should do it without the mentioned PR.

Cheers

[tabacitu]

Sounds awesome! Two features, one release 💪🎉