-
-
Notifications
You must be signed in to change notification settings - Fork 324
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
[LiveComponent] Adds MultiStep Form Feature #2433
base: 2.x
Are you sure you want to change the base?
[LiveComponent] Adds MultiStep Form Feature #2433
Conversation
Hello, thank you very much for your contribution: the idea is very interesting. |
Hello, there is a Storage used which allows you by default to stay on the same step on page reload. So there is the persistence level already included |
Nice! |
return u(static::class) | ||
->afterLast('\\') | ||
->snake() | ||
->toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return u(static::class) | |
->afterLast('\\') | |
->snake() | |
->toString(); | |
return u(static::class)->afterLast('\\')->snake()->toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And is symfony/string still a requirement ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes i required it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the only place where it is needed? In this case it is preferred to not use a dep but use plain vanilla PHP for it
Hey @silasjoisten ! Thanks for this nice contribution ! You bring something that can be truly useful for a lot of folks! I just don't thinks that a trait in the LiveComponent component is the right place... I think we bring to much that are not LiveComponent related: StorageInterface, MultiStepType, and so on. I am also scared also that doing this in to a Trait can limit us in to the implementation and on the DX. I think we can go way further on this topics and bring features that can be also front end related (step transition). |
Hey @WebMamba, Thanks for your detailed feedback and for taking the time to review this! 😊 I firmly believe that this feature fits well within the Symfony UX LiveComponents component. Here’s why:
I’d love to hear your thoughts on these points and where you see potential challenges or conflicts. If you have specific ideas for improving or repositioning the feature, I’m happy to collaborate further. Cheers, |
Hi @silasjoisten! So happy to see you there ❤️ First, thank you very very much for this PR! This feature will be warmly welcomed by many users. Regarding the "where", it is now pretty clear, in my mind, that we will end up with a dedicated "LiveForm" or "UxForm" component/bundle.. probably sooner than later. But right now, we have... LiveComponent :) Also, I do agree with you regarding the similarity with CollectionType + the DX aspects. As i see it, we can introduce it in "experimental" anywhere we want, and we'll see until that where it best fits in UX 3. Next comments in the code :) I need to sleep on the "StorageInterface" thing because i'm having mixed feeling about introducing a generic tool like this for now, while we're moving things around here. |
@@ -51,6 +51,7 @@ | |||
"symfony/serializer": "^5.4|^6.0|^7.0", | |||
"symfony/twig-bundle": "^5.4|^6.0|^7.0", | |||
"symfony/validator": "^5.4|^6.0|^7.0", | |||
"symfony/string": "^5.4|^6.0|^7.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove it ( if only used for the string/camel stuff ?)
->setDefault('current_step_name', static function (Options $options): string { | ||
return array_key_first($options['steps']); | ||
}) | ||
->setRequired('steps'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will require more checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What checks you got in mind?
public function configureOptions(OptionsResolver $resolver): void | ||
{ | ||
$resolver | ||
->setDefault('current_step_name', static function (Options $options): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss this more globally, but i think "current_step" or even "step" could be enough, and improve readability.
Maybe just me, but i feel the implementation choice (e.g., passing the steps as a map) should not impose constraints on the overall naming convention.
wdyt ?
{ | ||
$view->vars['current_step_name'] = $options['current_step_name']; | ||
$view->vars['steps_names'] = array_keys($options['steps']); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} | |
public function getBlockPrefix(): string | |
{ | |
return 'multistep'; | |
} | |
|
||
public function buildForm(FormBuilderInterface $builder, array $options): void | ||
{ | ||
$options['steps'][$options['current_step_name']]($builder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be checked first i guess ? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think so, because configure options is already ensuring that. (see the test)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may miss-read this, but to me at this point there is no certitude that $options['steps'][$options['current_step_name']]
is a callable that accept FormBuilderInterface argument ?
$view->vars['current_step_name'] = $options['current_step_name']; | ||
$view->vars['steps_names'] = array_keys($options['steps']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about "step" and "steps" (or "current_step" and "steps" ?
To illustrate my point here: if you look at ChoiceType, it does not use "prefered_choices**_values**" or "prefered_choices**_names**"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure we can rename this.
* @author Patrick Reimers <[email protected]> | ||
* @author Jules Pietri <[email protected]> | ||
*/ | ||
final class MultiStepType extends AbstractType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is where we should describe/document what is a "step".
* This class provides a session-based storage solution for managing data | ||
* persistence in Symfony UX LiveComponent. It leverages the Symfony | ||
* `RequestStack` to access the session and perform operations such as | ||
* storing, retrieving, and removing data. | ||
* | ||
* Common use cases include persisting component state, such as form data | ||
* or multistep workflow progress, across user interactions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Common use cases include persisting component state, such as form data, or multistep workflow progress, across user interactions.
I'm having doubt if we should push session usage like this, as in the same time we push for stateless components, removed csrf, etc..
And i'm very hesistant to add such feature in another PR :|
I don't think these two things should be handled as one...
Will sleep on it... but already can see several alternatives:
- having an internal/experimental storage (just pour the multi-step form)
- using props
- using existing adapters (doctrine, cache)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is stateless. okay we could introduce a NullStorage. But in order to be able to reload the page and having a persistent state is quite cool in my opinion.
/** | ||
* Abstract method to be implemented by the component for custom submission logic. | ||
*/ | ||
abstract public function onSubmit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what this mean, but onSubmit feels uncommon name.. :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe doSubmit ?
return; | ||
} | ||
|
||
$this->getStorage()->persist(\sprintf('%s_form_values_%s', self::prefix(), $this->currentStepName), $this->form->getData()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$this->getStorage()->persist(\sprintf('%s_form_values_%s', self::prefix(), $this->currentStepName), $this->form->getData()); | |
$this->getStorage()->persist(\sprintf('%s_form_values_%s', self::prefix(), $this->currentStepName), $this->form->getData()); |
This feels very implementation specific to me... more on that later :)
#[ExposeInTemplate] | ||
public function isFirst(): bool | ||
{ | ||
return $this->currentStepName === $this->stepNames[array_key_first($this->stepNames)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return $this->currentStepName === $this->stepNames[array_key_first($this->stepNames)]; | |
return $this->currentStep === reset($this->steps); |
wdyt ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reset can return false which makes it not typesafe enough for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then currentStep === .. would return false no ?
#[ExposeInTemplate] | ||
public function isLast(): bool | ||
{ | ||
return $this->currentStepName === $this->stepNames[array_key_last($this->stepNames)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return $this->currentStepName === $this->stepNames[array_key_last($this->stepNames)]; | |
return $this->currentStep === end($this->steps); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would like to use the new php functions array_key_first and array_key_last.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha the "new" :)
The "problem" (a detail, to be honest) here is that you first look for a key, then get the value indexed by that key, then compare currentStepName is identical.
But we can update this in time if necessary :)
$found = false; | ||
$previous = null; | ||
|
||
foreach (array_reverse($this->stepNames) as $stepName) { | ||
if ($this->currentStepName === $stepName) { | ||
$found = true; | ||
|
||
continue; | ||
} | ||
|
||
if ($found) { | ||
$previous = $stepName; | ||
|
||
break; | ||
} | ||
} | ||
|
||
if (null === $previous) { | ||
throw new \RuntimeException('No previous forms available.'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$current = array_search($currentStep, $steps, true);
if (!$current) {
throw new \RuntimeException('No previous steps available.');
}
$previous = $steps[$current - 1];
(or similar)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with you example the exception is thrown if no current step is available.
$this->getStorage()->persist(\sprintf('%s_current_step_name', self::prefix()), $this->currentStepName); | ||
|
||
$this->form = $this->instantiateForm(); | ||
$this->formView = null; | ||
|
||
$formData = $this->getStorage()->get(\sprintf( | ||
'%s_form_values_%s', | ||
self::prefix(), | ||
$this->currentStepName, | ||
)); | ||
|
||
$this->formValues = $formData; | ||
$this->form->setData($formData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate with "next" method... should this be the main "getCurrentForm" or "getStepForm" method or something like that ?
$this->submitForm(); | ||
|
||
if ($this->hasValidationErrors()) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If !form->isValid
, then a UnprocessableEntityHttpException
is triggered in the componentwithformtrait, no ? So as i see it, the hasValidationErrors() check is redondant (i may have missed something)
$this->submitForm(); | |
if ($this->hasValidationErrors()) { | |
return; | |
} | |
$this->submitForm(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure to be honest. i can test it.
#[LiveProp] | ||
public ?string $currentStepName = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make this non nullable? I don't see when a multi-step could have "no current step"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are in a trait and we dont have a constructor or something here. which means it depends on the #[PostMount] hook. if thats not executed it is null by default. i think thats something by design we should not change.
|
||
$this->stepNames = $this->formView->vars['steps_names']; | ||
|
||
// Do not move this. The order is important. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question :D i cann add a better comment
Co-authored-by: Simon André <[email protected]>
Co-authored-by: Simon André <[email protected]>
Co-authored-by: Simon André <[email protected]>
Co-authored-by: Simon André <[email protected]>
Co-authored-by: Simon André <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raw thought:
IMO, this feature can have its place in Form component, it will be useful for people/project that not using ux yet and will ease transitioning to ux.
And that way StorageInterface
make sense in the Form component.
|
||
public function buildForm(FormBuilderInterface $builder, array $options): void | ||
{ | ||
$options['steps'][$options['current_step_name']]($builder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allowing to use FormType
as step can be good too. What do you think?
Maybe with something like that (haven't tested it):
$options['steps'][$options['current_step_name']]($builder); | |
$step = $options['steps'][$options['current_step_name']]; | |
if (is_callable()) { | |
$step($builder); | |
} elseif (is_string($step) && is_a($step, FormTypeInterface::class)) { | |
$builder->add('step', $step, [ | |
'inherit_data' => true, | |
]); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes i was thinking about the same. so callback or class string of FormTypeInterface.
|
||
public function buildForm(FormBuilderInterface $builder, array $options): void | ||
{ | ||
$options['steps'][$options['current_step_name']]($builder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$options['steps'][$options['current_step_name']]($builder); | |
$options['steps'][$options['current_step_name']]($builder, $options); |
IMO, passing $options
to keep consistent with buildForm
method can be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure we can do that but my question is. Why would it make sense to pass the options of the "parent" form StepType to the children? you got an example or use case where it might be useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is more to be consistent with how work the buildForm
method, where you have access to options.
In your PR, you cannot set your form options yet, so there is no direct example. instantiateForm
method pass an empty $options
(except the current_step_name
) to FormFactory::create
.
You may want to create step form that is configurable, either from:
- your live component (not yet possible)
- using
FormTypeExtension
from Form component
Is it clearer that way?
My overall reaction here would be : i kinda feel 80% of this PR should be in ... the symfony/form component. Both the "steps" logic and the "storage" logic are not related with Symfony UX and could be used without it. I'd even go to say LiveComponent could only provide a storage (in the browser DOM, via a LiveProp) for this feature. |
I tend to agree that part of the added logic could belong to symfony/form instead. Curious to know @HeahDude thoughts on this |
This pull request introduces a new feature to Symfony UX LiveComponent that enables developers to easily create multistep forms in their applications. This functionality simplifies the implementation of complex, multi-step workflows while maintaining a clean and structured developer experience.
Key Features
Example Usage
getParent
to Create a custom MultiStep form type:Acknowledgments
This feature was collaboratively developed during the #SymfonyHackday following SymfonyCon Vienna 2024. Special thanks to the contributors for their outstanding work:
Your efforts and dedication have made this feature possible!
Still on my todo list:
Look and feel