laravel-arcanist / arcanist

🧙‍♀️ Arcanist takes the pain out of building multi-step form wizards in Laravel.
https://laravel-arcanist.com
MIT License
403 stars 32 forks source link

Change visibility to protected #21

Closed erikwittek closed 3 years ago

erikwittek commented 3 years ago

There are some attributes/methods in the AbstractWizard and WizardStep classes which we need to access in child-classes.

I've changed their visibility to protected.

ksassnowski commented 3 years ago

Thanks for the contribution!

Can you tell me a little about what you're trying to do and why this change is necessary? I'm not opposed to these changes, but the fact that some things seem to be difficult to extend and require extending from the AbstractWizard class are a pretty strong code smell to me (for the package, not your code).

I mentioned elsewhere that I plan to do a major rewrite of this library before releasing v1 so I'm trying to learn about how people are actually using/trying to extend Arcanist.

Thanks!

erikwittek commented 3 years ago

Hey,

in the case of the AbstractWizard we're trying to implement the onAfterComplete() and onAfterDelete() hooks, where we need access to the Renderer:

protected function onAfterDelete(): Response | Responsable | Renderable
{
    return $this->responseRenderer->jsonRedirect(/** ... */);
}

protected function onAfterComplete(ActionResult $result): Response | Responsable | Renderable
{
    return $this->responseRenderer->jsonRedirect(/** ... */);
}

For context: jsonRedirect() is a function to instruct the SPA router to perform a redirect to a page outside of the wizard. It's implemented on the Renderer to have all the response generation in a single class.

The rules() method in the Step is needed, because we're customising the the process() method in a subclass to filter the fields which are processed:

public function process(Request $request): StepResult
{
    $data = $this->validate($request, $this->rules());

    return collect($this->fields())
        // Filter fields to be processed
        ->filter(function (Field $field) {
            return $field->isEditable();
        })

        ->mapWithKeys(fn (Field $field) => [
            $field->name => $field->value($data[$field->name] ?? null)
        ])
        ->pipe(fn (Collection $values) => $this->handle($request, $values->toArray()));
}

Of course, both of these changes are special to our use case, so we wouldn't want to push them upstream. But having access to this kind of attributes and methods - especially in abstract classes - would help us not to duplicate too much logic.

Thanks for your work on this package. It's much appreciated. If you need anything else, please do say so.

ksassnowski commented 3 years ago

Thanks for the explanation! I'll keep this in mind for version 1.0