-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] Add MultiStepType
#59548
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
[Form] Add MultiStepType
#59548
Conversation
MultiStepType
MultiStepType
MultiStepType
MultiStepType
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.
Interesting idea!
I wonder if we could create a default theme for this form type that would (at least):
- render the current step form (this is already the case but might require some tweaks for the next point below)
- render the back/next "submit" buttons based on the "step number" (which is currently missing but can be calculated easily using the provided options)
While data storage and form handling are beyond the scope of this type, I believe having that default theme would be great.
src/Symfony/Component/Form/Extension/Core/Type/MultiStepType.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Form/Extension/Core/Type/MultiStepType.php
Outdated
Show resolved
Hide resolved
Hey thanks for the review!
I added some more helpers in view vars which makes rendering easier and also i added some options in order to allow navigate through steps. I also updated this PR description with an example of how to use the form in a controller. |
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 the Steps and Navigator elements should be designed as two distinct types (even if they are related). This would make it easier for users to extend/customize each one independently.
{ | ||
$resolver | ||
->setRequired('steps') | ||
->setAllowedTypes('steps', 'array') |
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.
array
-> (string|callable)[]
this is a fresh feature already merged in 7.3
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 all current_step / next_step etc allow callable ? That would ease integration with Stepper or Navigator or any future WizardStepPathFinder-like.. 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.
Good idea but i dont know what i should do here. Maybe i dont fully understand the approach
After reviewing the PR description again following your latest update, it might be helpful to include an example with a DTO (+ validator constraints bound to the underlying object) as this aligns with the recommended approach for working with forms. I like the idea and how we can configure the steps, but handling it from the controller still feels a bit complex IMO. |
Sure i can add that. Thats just gonna be a fee lines of code.
Well but all of that is implementation detail. Like which storage to use which steps to skip or add to your multistep type. Without an implementation of a storage its not possible to enhance the DX. Mainly we should not forget that the main reason for this PR is to have this available for a nice LiveComponent in symfony UX. If you have any better idea of how to increase the developer experience feel free to tell. I honestly ran out of ideas because i tried a lot of different things today. And i always came to the same conclusion... storage, storage, storage 😅 |
Would this require a lot of work to implement the following (not in this PR of course)
Or is this "one" of the storages we're talking about ? Will there be any interface for the "storage" in symfony/form .. or we let entirely people implementing from scratch this part ? |
We "started" with a storage imterface (and a session storage) when we build it for UX. But in the meantime we figured Symfony has no place for a storage interface (except if we would create something like a storage-contract. But a storage-contract would only provide one interface, implementation like a SessionStorage/FilesystemStorage/RedisStorage/... would need to go into different components (SessionStorage in Framework? FilesystemStore in Filesystem? RedisStorage in ...) Or are we open to add "storage" into Form? |
What about adding a new
I think you raise an important point here. Introducing this type in the Symfony repo would mean designing it in a way that isn't strictly tied to the UX package, as not all users may be using UX packages like we do. It's essential to ensure that it works well for everyone. |
We were thinking about implementing a hidden field containing the current state, but when the page is reloaded we'll lose the current state. In our current example implementaion we're using the session, this is persistent across reloads (until the session is killed). |
Yes we could do that. As we already have the Interface. How do we do the implementation? Shall i require http foundation for the session?
I agree on that. But isn't it usable without symfony ux yet? I mean it might be a bit more code to write but it would be already usable. Regarding the storage where should i put it? Can you maybe help me on this one? |
I would not be shocked if a "FormStepPersisterInterface" or "Loader" or something like this was added into Form 🤷 Like ChoiceLoaderInterface maybe ? Or simply document the events and to plug itself to store the data. We also can consider there is only one entity saved, partially at each step, and then nothing is really needed here ? |
I guess not, indeed! 👍 (documentation will maybe just need a warning regarding funnel forms with login or register in the way, as session is often reset then) |
We shouldn't require the http foundation directly, take a look at
Let me play a bit with this proposal and I'll back next week with more details about it. |
If you concider that yes then the implementation would be Userland |
For reference https://github.com/craue/CraueFormFlowBundle. I've used this bundle in several projects and found it suitable for most advanced cases. It's worth a look and a ping to @craue who has expertise on this topic. |
@silasjoisten I suggest updating your example code to remove the usage of |
Yea i have worked with it as well and i did not like the DX in it. It felt quite old and i mean its a common problem why shouldn't it be part of Symfony itself. Even if its just a simple form flow without skipping things. For some cases yes you need a more complex form flow. but sometimes you want to have it in order to have a nice User experience in you Application. |
What is the current state of this PR? |
I tested the proposal as it is, and it feels like too much responsibility for the user to handle (in the current state). I'm not referring to the form step definition, which is already simple, but to the navigation and data storage part across steps. IMO, there should be a default implementation that handles that for us, not a UX but a Symfony one, flexible enough to be used with or without UX capabilities. This is my current expectation for this feature (https://gist.github.com/yceruto/0fe65c8669016fe48f24c4e047ce7fb1):
my two cents :) still looking forward to this feature 💘 |
Nicee gist you made there! I focus ghis weekend on this Pr. And try to make it as you expect :) i like your idea very much! |
if (\is_callable($currentStep)) { | ||
$currentStep($builder, $options); | ||
} elseif (\is_string($currentStep)) { | ||
$builder->add($options['current_step'], $currentStep); |
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.
the step type may require specific options that differ from those in the root form.
we need to consider this, as it's currently a limitation
Comming from the initial UX repo PR :) nice one If something like this land on sf/form I think it needs storage included otherwise it may be « hard/prone to error » to implement/handle and may lead to devland issues no? |
}); | ||
|
||
$resolver->setDefaults([ | ||
'hide_back_button_on_first_step' => false, |
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.
display_ with default true instead of hide_ default false?
Seems easier to read the positive way not the négative
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.
Many thanks for this PR.
I find the idea really very interesting. This type of need is common in applications.
However I have some questions/remarks.
return false; | ||
} | ||
|
||
if ((!\is_string($step) || !is_subclass_of($step, AbstractType::class)) && !\is_callable($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.
A $step
object that implements FormTypeInterface
should be allowed as well
Also, I have the feeling that parenthesis should be added here (mixing ||
and &&
).
$builder->add($options['current_step'], $currentStep); | ||
} | ||
|
||
$builder->add('back', SubmitType::class, [ |
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'm not a fan of buttons within form type objects. I prefer their integration into the template.
There should be a way for the template to know when each button has to be displayed or not e.g. with the help of the isFirstStep
or isLastStep
methods.
Also, this will remove the need of the hide_back_button_on_first_step
, button_back_options
, button_next_options
and button_submit_options
options. WDYT?
Hi there! I'll get back to this topic next week with a complete alternative proposal that addresses the main implementation concerns (this one: #59548 (comment) and others mentioned). I can't wait to share it with all of you! |
Here we go #60212 ! |
I will close this one thank you @yceruto |
This PR introduces the
MultiStepType
form type, which allows for the creation of form flows. The idea for this form type was initially proposed in a PR for Symfony UX, where it was suggested that the type would be better suited within the Symfony Form component.But this form works also without Symfony UX.
Usage:
In the controller (this is only one possible usage of the form in order to persist the current step and the data):
And Rendering:
Look and feel!