Skip to content
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] Dependent form fields logic broken when submitting the form #2240

Open
gremo opened this issue Oct 5, 2024 · 19 comments
Open
Labels
LiveComponent question Further information is requested

Comments

@gremo
Copy link
Contributor

gremo commented Oct 5, 2024

Minimal repro: https://github.com/gremo/github-issue-2240

I'm implementing a search form for employee work hours. The form should allow the following options: no filtering at all, filtering by office only, filtering by employee only and iltering by both office and employee.

Therefore, the model has no strict constraints, and the form fields are optional. The form works correctly if no filters are selected, or if either the office or the employee is selected, or if the office is selected first followed by the associated employee.

class WorkHoursSearch
{
    public ?Office $office = null;

    public ?Employee $employee = null;
}
class WorkHoursSearchForm extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        $builder = new DynamicFormBuilder($builder);

        $builder
            ->add('office', EnumType::class, [
                'class' => Office::class,
                'choice_label' => fn (Office $office): string => $office->getReadable(),
                'placeholder' => 'Any office',
                'required' => false,
            ])
            ->addDependent('employee', 'office', function (DependentField $field, ?Office $office) {
                $field->add(EnumType::class, [
                    'class' => Employee::class,
                    'placeholder' => 'Any emplyee',
                    'choices' => $office?->getEmployeeChoices(),
                    'choice_label' => fn (Employee $employee): string => $employee->getReadable(),
                    'required' => false,
                ]);
            })
        ;
    }

    public function configureOptions(OptionsResolver $resolver): void
    {
        $resolver->setDefaults(['data_class' => WorkHoursSearch::class]);
    }
}

When do problems occur? They happen when, after selecting the office and employee, the user decides to change the office.

Office 1 -> Employee 1 -> Office 2 -> Submit

scanario1

In this case (scenario 1) the selected employee is no longer part of the user list for the new office, but the previous value is still sent with the form (because form values prop is a LiveProp), so the form is invalid (an unknown value is present).

A more critical issue (scenario 2) occurs when the office has no associated employees:

Office 1 -> Employee 1 -> Office 3 -> Submit

scanario2

In this case, there’s no way to make the form valid and processable, even by selecting "Any employee." While this would understandably result in zero results, that's not the main concern.

In both cases there's no feedback provided to the user, no errors, no one knows what's happening.

I genuinely believe this is an issue that needs to be addressed. Personally, in my case, I don't see any solution, not even a workaround, but I still need to try something with JavaScript. Ideally, if field B depends on A to function, then when A changes, B should always be reset in some way. Otherwise, as described, the old value of B, which makes the form invalid, will continue to be sent to the server.

View old question here

I think there is a very obvious issue with the dependant form fields UX feature, take the meal example.

Let's add the logic for handing the form submission:

#[AsLiveComponent]
class MealPlanner extends AbstractController
{
    // ...

    #[LiveAction]
    public function save()
    {
        $this->submitForm();

        dd($this->getForm()->getData());
    }
}

Change also the form action and add a submit button:

{# ... #}

{{ form_start(form, {
    attr: {
        novalidate: true,
        'data-action': 'live#action:prevent',
        'data-live-action-param': 'save',
    }
}) }}
    <button>Save</button>
{{ form_end(form) }}

{# ... #}

Now let's use the form:

  1. Select breakfast as meal
  2. Select egg as food
  3. Change meal to lunch (eggs are NOT in lunch choices)

Until now the form re-renders correcly with HTTP 200 status code. No validation errors are shown.

Now submit the form pressing the "save" button:

  • 422 Unprocessable Content is returned, because the form is invalid
  • The "eggs" value is sent even if I didn't selected the food option (the placeholder "What's for lunch" is selected)
  • Value "eggs" is not in the lunch options, so it's an extraneous/invalid value
  • I think this is happening because of the nature of live components
  • No validation errors are shown!

image

image

Here is a short video of the steps:

change

I opened a similar issue SymfonyCasts/dynamic-forms#35 but I think that problem is not related to the library itself.

@smnandre
Copy link
Member

smnandre commented Oct 5, 2024

Let's keep the "required" and there is no problem.. you should not be able to submit without selecting a value.

(try without novalidate: true, )

@gremo
Copy link
Contributor Author

gremo commented Oct 5, 2024

Seems not a solution to me :) the real problem is submitting the "old" value to the form.

And problem is even worst when one of the meal doesn't include any option.

@smnandre
Copy link
Member

smnandre commented Oct 5, 2024

This is the only "solution" i have .... as I don't see any bug on the demo.

Please look at the following video.

dependent-form-fields.mov

@smnandre smnandre added the question Further information is requested label Oct 5, 2024
@gremo
Copy link
Contributor Author

gremo commented Oct 5, 2024

So @smnandre we rely on client side validation to make the form "work"?

And you are not considering one thing: meal could be optional, so required would not help here. Should I provide a more real example? Like the one explained here: SymfonyCasts/dynamic-forms#35

@smnandre
Copy link
Member

smnandre commented Oct 5, 2024

-- friendly tone here 😃 --

i just spent 15 minutes to understand your problem, replicate it, point were you missed something, record and post a video to show you the demo works exactly how it is supposed to.

And now you politely suggest me to look at another exemple, with another code.

I'm sorry.. i don't have that energy right now ;)

I'll check tomorrow or monday the other issue.

@gremo
Copy link
Contributor Author

gremo commented Oct 5, 2024

No problem, @smnandre. I really appreciate your work and time to look at this issue. My tone was meant to be "normal", I guess, but not being fully understood is frustrating 🥲

I understand that the example "works" thanks to the required attribute and client side javascript. Tomorrow I'll update my example to show that, in some situation, neither the required attribute will help.

@smnandre
Copy link
Member

smnandre commented Oct 5, 2024

My tone was meant to be "normal"

I know ;)

@gremo
Copy link
Contributor Author

gremo commented Oct 5, 2024

@smnandre I’ve updated the question and the repository, trying to explain myself as clearly as possible.

I’m not sure if I’m just unlucky 😢 , if no one has ever noticed this issue, or if there’s something wrong with the version of the libraries I have installed.

Personally, I believe no one has noticed this behavior before because, as you likely mentioned, they use the required attribute, which in my case should not be specified (since it must be possible to perform the search with empty values).

@smnandre
Copy link
Member

smnandre commented Oct 6, 2024

For the record:

the discussion continued acrosss repositories (😅 ) but point is:

there is some case not fully covered / implemented in the DependentFormField here...

Let's see if we can suggest something :)

@gremo
Copy link
Contributor Author

gremo commented Oct 6, 2024

To summarize what's mentioned in the repository and the excellent research by @smnandre : there is definitely an issue with the library, I quote the source code:

// A fake hidden field where we can "store" an error if a dependent form
// field is suddenly invalid because its previous data was invalid
// and a field it depends on just changed (e.g. user selected "Michigan"
// as a state, then the user changed "Country" from "USA" to "Mexico"
// and so now "Michigan" is invalid). In this case, we clear the error
// on the actual field, but store a "fake" error here, which won't be
// rendered, but will prevent the form from being valid.

The last statement seems odd since, in fact, the form becomes invalid in any case.

But @smnandre , let's pretend we're not using the Dependant Fields library, because in my opinion, the error is more of a logical one and lies in how the data submission is handled (in the context of live components). Here's a 'vanilla' example:

public function buildForm(FormBuilderInterface $builder, array $options): void
{
    $builder
        ->add('office', EnumType::class, [
            'class' => Office::class,
            'choice_label' => fn (Office $office): string => $office->getReadable(),
            'placeholder' => 'Any office',
            'required' => false,
        ])
    ;

    $formModifier = function (FormInterface $form, ?Office $office): void {
        $form->add('employee', EnumType::class, [
            'class' => Employee::class,
            'placeholder' => 'Any employee',
            'choices' => $office?->getEmployeeChoices(),
            'choice_label' => fn (Employee $employee): string => $employee->getReadable(),
            'required' => false,
        ]);
    };

    $builder->addEventListener(
        FormEvents::PRE_SET_DATA,
        function (FormEvent $event) use ($formModifier): void {
            $formModifier($event->getForm(), $event->getData()?->office);
        }
    );

    $builder->get('office')->addEventListener(
        FormEvents::POST_SUBMIT,
        function (FormEvent $event) use ($formModifier): void {
            $formModifier($event->getForm()->getParent(), $event->getForm()->getData());
        }
    );
}

What happens now? The errors are shown. But look what happens when:

Office 1 -> Employee 1 -> Office 3 -> Submit

vanilla

We are now basically stuck. We can't go even back and select "Any office" and "Any employee", without a full page refresh.

@smnandre
Copy link
Member

smnandre commented Oct 6, 2024

Let's imagine all is easy and possible. So regarding the best user-experience in this scenario..
.... what would be the best behaviour for a user, just after selecting "Office 3" ?

A) set Employee to "null"
B) set Employee to the "first accepted value of Office 3"
C) show an error

I personnaly would opt for A or B... what about you ?

@smnandre
Copy link
Member

smnandre commented Oct 6, 2024

But look what happens when [...]

I do agree this is a weird situation, and we'll try to find a good solution, but you cannot compare to anything "not live" because.... the Employee 1 -> Office 3 would simply not be possible :)

So more than a submit problem, it's a "invalid state that should not happen" in a way

@gremo
Copy link
Contributor Author

gremo commented Oct 6, 2024

what would be the best behaviour for a user, just after selecting "Office 3" ?

Option 1, set to null, without a doubt. If, for some reason, field B (which depends on A) is required, the validation stack will kick in, right? In my case, setting the employee to null is fine, allowing the user to submit the form and search for 'all employees that belong to office 3'.

the Employee 1 -> Office 3 would simply not be possible :)

You mean in a non-live scenario, right? Of course, yes. If we submit the form the traditional way and update the employee field accordingly, this wouldn’t be an issue. But in this case, the form updates as soon as you select 'office 3,' and then everything gets stuck.

I'm feeling a bit discouraged, honestly 😢 I don't believe there's a possibility to resolve everything within the ComponentWithFormTrait. I'm afraid I might need to write custom JavaScript for every similar situation (and work on setting field B every time field A changes).

@smnandre
Copy link
Member

smnandre commented Oct 6, 2024

But in this case, the form updates as soon as you select 'office 3,' and then everything gets stuck

I know i know ... 😅

Option 1, set to null, without a doubt

This is where i'll personnaly drop entirely formtrait and dependantformfieldd to just manually use 2 props 🤷

Again, DependantFormField is not an UX package currently, and this use case is legit but still very rare... and what you would expect here would prevent another one to get what they expected.

Probably stupid question, is it possible to make both fields dependant on ... the other one ?

I'm afraid I might need to write custom JavaScript for every similar situation (and work on setting field B every time field A changes).

No JS, just live component, mount, and onUpdate :) I really need to do other things today, but if you're stuck tomorrow i can maybe show you an example :)

@smnandre
Copy link
Member

smnandre commented Oct 6, 2024

I don't believe there's a possibility to resolve everything within the ComponentWithFormTrait

Oh, i'm 100% certain there is none.... but this does not mean LiveComponent is not a good choice (in this very specific case, not saying anything else ;) )

@italodeveloper
Copy link

+1 @gremo here I am having the same problem, I noticed that when removing the DynamicFormBuilder application it works as expected, did you get any solution?

@gremo
Copy link
Contributor Author

gremo commented Dec 13, 2024

@italodeveloper Unfortunately not, still searching for a solution.

@italodeveloper
Copy link

@italodeveloper Unfortunately not, still searching for a solution.

non-elegant solution manipulate a fake select example with live components and its result as id or unique key and load it in the form via options it worked, ugly but it worked

@smnandre
Copy link
Member

I noticed that when removing the DynamicFormBuilder application it works as expected

Ho ? So we did fix something with the recent batch of changes in Live ? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LiveComponent question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants