laravel-model-states icon indicating copy to clipboard operation
laravel-model-states copied to clipboard

Equalize DefaultTransition and custom Transitions constructors

Open luckcolors opened this issue 2 years ago • 2 comments

Hello again.

This should be the last pull request i need to do. :smile:

I'm sorta confused by the behaviour of resolveTransitionClass() currently.

Why doesn't a custom transition gets the same parameters as the default one and vice versa?

Also i don't see currently how a custom transition can advance to the newState as it's not passed to it. I suppose this is because it's possibly not intended to write a generic custom transition? As in a custom transition should have an hardcoded target state.

Could you shed some light on this? maybe i'm taking the wrong approach. Feel free to look at my proposed change.

This if it gets merged should possibly require a major version since i'm changing the constructors and could break existing people's code.

Thanks again for your help.

luckcolors avatar Aug 01 '22 15:08 luckcolors

Hello. Here's my proposal.

I created a TransitionContext object wich will hold all relevant information that a Transition will need to be able to both handle generic and specific usages.

Code from resolveTransitionClass():

            $transition = new $transitionClass(
                new TransitionContext(
                    $this->model,
                    $this->field,
                    $newState,
                ),
                ...$transitionArgs
            );

Should the user want to override the landing state after handle() execution they can do so since access to newState is now given. Or create a generic transition to be applied ondemand (wich i think is more flexible than overriding the default one and is my usecase).

The only problem now is that the Transition constructor itself needs to be standardized to handle when $model is a TransitionContext. I decided this could be a good tradeoff since it doesn't require users wich want to apply transitions using the transition() to provide a TransitionContext but can provide directly the model itself wich i suppose is something you would want to do always anyways.

        if ($model instanceof TransitionContext) {
            $this->model = $model->model;
        } else {
            $this->model = $model;
        }

I'm still unsure wether the code involving $model derivation should be part of the Transition abstract class. Mostly for the reason that i've seen it defined in DefaultTransition using protected and in tests using private, and i think it could be better to allow the user to choose.

Let me know what you think. Thanks for your time. :smile:

luckcolors avatar Aug 02 '22 09:08 luckcolors

I have squashed the commits. This should be ready for review now.

luckcolors avatar Aug 10 '22 08:08 luckcolors

Dear contributor,

because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it.

spatie-bot avatar Dec 12 '22 11:12 spatie-bot