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

[4.x] Support Laravel Precognition on front end forms #8886

Merged
merged 28 commits into from
Nov 30, 2023

Conversation

ryanmitchell
Copy link
Contributor

@ryanmitchell ryanmitchell commented Oct 27, 2023

This PR adds support for Laravel Precognition in front end forms ({{ form:create }}).

To do this I've moved the validation logic out of FormController and into a FormRequest.

I needed to surface the Validator being used behind the scenes so added a public method on Statamic's Validator to give access to that.

From my own tests using the Alpine driver this seems to be working well.

Closes statamic/ideas#1005

@ryanmitchell ryanmitchell marked this pull request as ready for review October 27, 2023 13:26
@ryanmitchell
Copy link
Contributor Author

I've just noticed the json response isnt quite the same now - moving to draft until I resolve it.

@ryanmitchell ryanmitchell marked this pull request as draft October 27, 2023 15:31
@ryanmitchell ryanmitchell marked this pull request as ready for review October 27, 2023 16:02
@robdekort
Copy link
Contributor

I've been playing around with this PR. This stuff looks really interesting. Thanks Ryan! One thing I found though, is that all fields validate on blur. This is not something that happens in the Precognition video, and I doubt it's the intended behaviour.

Here's an example of what I get:
https://github.com/statamic/cms/assets/69107412/ef415bed-4d04-4c11-a3f3-04dc79fb9844

I understood Ryan got the same behaviour. I don't know why it happens, but perhaps this has something to do with the Statamic internals. Just wanted to leave this here in case it rings a bell for anyone.

@jasonvarga
Copy link
Member

Can you show how you are triggering the validation in your html/js?

@robdekort
Copy link
Contributor

Sure. I followed the precognition docs and use the following on my form fields:

@change="form.validate('{{ handle }}')"

@jasonvarga
Copy link
Member

Using laravel-precognition-vue?

@robdekort
Copy link
Contributor

Using laravel-precognition-vue?

Ah sorry, no. I use alpine: laravel-precognition-alpine.

@ryanmitchell
Copy link
Contributor Author

ryanmitchell commented Oct 30, 2023

I think it's to do with the alpine driver - it always sends all the fields, maybe thats not the case with other drivers. Its possible to work around with some Alpine functions... eg

        <form x-data="{
            form: $form('post', '/!/forms/test', { message: '', email: '', }),
            hasChanged: [],
            hasSubmitted: false,
            change(handle) {
                if (! this.hasChanged.includes(handle)) {
                    this.hasChanged.push(handle);
                }

                this.form.validate(handle)
            },
            showError(handle) {
                return (this.hasSubmitted || this.hasChanged.includes(handle)) && this.form.invalid('message')
            },
            submit() {
                this.hasSubmitted = true;
                this.form.submit()
                    .then(response => {
                        this.form.reset();

                        alert('Form submitted.')
                    })
                    .catch(error => {
                        console.log(error);
                    });
            },
        }" @submit.prevent="submit">

            <label for="message">Message</label>
            <input
                id="message"
                name="message"
                x-model="form.message"
                @change="change('message')"
            />
            <template x-if="showError('message')">
                <div x-text="form.errors.message"></div>
            </template>

            <label for="email">Email</label>
            <input
                id="email"
                name="email"
                x-model="form.email"
                @change="change('email')"
            />
            <template x-if="showError('email')">
                <div x-text="form.errors.email"></div>
            </template>

@robdekort
Copy link
Contributor

Interesting. Thanks for sharing! If it does turns out to be an issue with the Alpine driver specifically, I’d be happy to open an issue on the Precognition repo.

@marcorieser
Copy link
Contributor

marcorieser commented Oct 31, 2023

Tinkered with Precognition in plain Laravel and this PR. In plain Laravel just the changed field gets validated. With this PR all fields / rules are validated.

I can narrow it down to the overwritten method getValidatorInstance() in the FormRequest.

As soon as I return the parent implementation when the request is precoginitive, it seems to work as in plain Laravel. Not sure which side effects could occur but maybe this helps.

This works:

  protected function getValidatorInstance()
  {
      if ($this->isPrecognitive()) {
          return parent::getValidatorInstance();
      }

      return $this->getCustomValidator()->validator();
  }

@ryanmitchell
Copy link
Contributor Author

@marcorieser thanks for digging. I've taken a difference approach: 114c39f (#8886)

@robdekort
Copy link
Contributor

This does the trick guys! Great!

@marcorieser
Copy link
Contributor

Strange thing is that sometimes the validation error appear instantly when blurring eg. an email field but sometimes it takes up to 1s (locally). No idea why. Rob is experiencing the same he told me.

@robdekort
Copy link
Contributor

Your latest commit makes a massive difference. I've done a few tests and it seems instant now.

@robdekort
Copy link
Contributor

@robdekort
Copy link
Contributor

I just tested this PR (not using Precognition) with the native form tags (submission via post and using fetch()) and I can confirm these methods still work with this pull request.

@jasonvarga
Copy link
Member

Thank you!

@robdekort
Copy link
Contributor

Thank you!

Welcome! Does it show I'm eager for it to get merged? ;-)

src/Http/Requests/FrontendFormRequest.php Outdated Show resolved Hide resolved
src/Http/Requests/FrontendFormRequest.php Outdated Show resolved Hide resolved
src/Http/Requests/FrontendFormRequest.php Outdated Show resolved Hide resolved
@jasonvarga
Copy link
Member

In general, this is working pretty great though. 🤘

@ryanmitchell
Copy link
Contributor Author

Well that certainly made things a lot easier.
That should be all the changes you suggested made.

@jasonvarga jasonvarga dismissed their stale review November 16, 2023 15:52

The requested changes were made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forms - integrate Laravel's Precognition
4 participants