Feature/advanced should verify behaviour

[martyf]

Thanks for such a great addon. This is an advanced use case but has come up with changes to how Laravel 11 handles events.

Our requirements

Our specific use case is having Global-set configuration options that allow us to toggle captcha behaviour on or off based on an environment - really helpful for local dev. We have a Global that has our form config - success messages, and the like - and three toggle switches, one for Dev, Staging and Prod, that determines whether captcha should be attempted for that environment.

Laravel 10 and before

With Laravel 10, we could re-bind the ValidateFormSubmission listener. We created our own Listener that extended yours, and extended the shouldVerify behaviour to look at the Global setup and the environment, and determine if it was enabled or not. If not, we could return false and avoid verification.

Laravel 11

However with Laravel 11, type-hinted Events are automatically registered - meaning we get both the package's listener, and our own bound listener.

This means it runs twice, and fails as only one call succeeds.

Suggestion

This PR suggests adding a new config option - advanced_should_verify - that accepts an invokable class that receives the $submission. This needs to return true or false.

For example, our class may look like:

<?php

namespace App\Support;

use Statamic\Forms\Submission;

class ShouldVerifyCaptcha {

    public function __invoke(Submission $submission):bool {
        // get the settings
        $forms = GlobalSet::findByHandle('forms')->in(Site::current()->handle);

        // get the env
        $environment = config('app.env');

        // if the config is false, then do not validate
        if ($forms->get('forms_'.$environment, false) === false) {
            return false;
        }

        return true;
    }
}

When our Global config is set to have captcha disabled for the environment, it will return false, and not verify the captcha.

In the config, you would just need to reference this:

<?php

return [
    // ...
    'advanced_should_verify' => \App\Support\ShouldVerifyCaptcha::class,
    // ...
];

This allows a developer to add their own advanced verification enabled (or not) logic that extends the existing behaviour of forms and collections. The developer can choose to look at things like Globals, or look at the submission content itself, or even something else. It opens up additional opportunities to help extend the flexibility for those who want it, but also requires no changes for those who don't need it.

Happy to discuss this further if you had other ideas or approaches.

[aryehraber]

Hi @martyf,

Thanks, great to hear the addon has been useful to you so far!

First off, great PR description! I can tell you spent some time on this and really appreciate the clear story 👌

Love this idea and would definitely like to get this into the addon.

A few initial thoughts:

1) I think I would prefer to add a default invokable ShouldVerifyCaptcha class to the config (Spatie do this in many of their Laravel packages and I'm a big fan of this pattern, e.g. Activity Log config).

2) Need to do a little more thinking around this next point, but Captcha supports various events, not just form submissions (see the different listeners). Therefore, it feels a little odd to me that this custom class would only handle the ValidateFormSubmission listener.

If you'd like to give these points some thought and work on the PR a little more, then please go ahead. If you would prefer not, that's also fine, I can merge your work as is and use it as a starting point (and get you on the contributor list 😉).

Anyways, thanks for the work so far!

[martyf]

You can clearly see that I don't use it for other listeners hey 😉

One thought I had - but felt like overkill but maybe now it's not - is to set up a facade that could accept two things:

1. a listener type, and
2. a closure

Benefit to this approach would be the logic could be added to an existing Service Provider, such as:

StatamicCaptcha::shouldVerify(ValidateFormSubmission::class, function(Submission $submission) {
    // ...
});

This would allow for different "should verify" logic for each listener type - especially given each may or may not receive properties - might be a bit hard to generalise this logic for each type.

Downside would be that it would need to apply for each listener - so duplicate code - or could do this too for all listeners:

StatamicCaptcha::shouldVerify('*', function() {
    // ...
});

What do you think of that approach?

Happy to chip away at that tomorrow (late afternoon here at the moment) if you see value in that approach?

[aryehraber]

I appreciate the quick response and sharing your further thoughts on this!

You actually got the gears in my head turning a little extra with this PR so I have a vague vision of how I'd like to see this implemented. It's actually very close to your initial work so far, so I'll hack a little further on your changes and push to this branch so we can discuss (and test) before merging.

If I don't manage to hack anything working/useful together, we can perhaps pursue the Facade idea. Does that sound alright?

[martyf]

Sounds good to me. Let me know if there’s anything I can do.

[aryehraber]

Hey @martyf!

Just pushed my changes and would love for you to review and test them if you can. Basically, I took your initial (great) idea and expanded on it to allow any event to be hooked into, not limiting it to only form submissions.

Don't be thrown off by the large number of changes, I took this opportunity to refactor the addon's internals which I've wanted to do forever now.

I changed the name of the config option to custom_should_verify and decided to leave it as null by default (no need to complicate this). Next, I added a small interface which ensures the invokable class has the correct method signature. Here's a quick example with different possibilities that Captcha already supports:

<?php

namespace App\Support;

use AryehRaber\Captcha\Contracts\CustomShouldVerify;
use Illuminate\Auth\Events\Login;
use Statamic\Events\EntrySaving;
use Statamic\Events\FormSubmitted;
use Statamic\Events\UserRegistering;

class MyCustomShouldVerify implements CustomShouldVerify
{
    public function __invoke($event): ?bool
    {
        if (app()->environment('dev')) {
            return false;
        }

        if (auth()->check()) {
            return false;
        }

        if ($event instanceof FormSubmitted) {
            // return $event->submission;
        }

        if ($event instanceof Login) {
            // return $event->user;
        }

        if ($event instanceof UserRegistering) {
            // return $event->user;
        }

        if ($event instanceof EntrySaving) {
            // return $event->entry;
        }
    }
}

[martyf]

This is awesome, and works a charm - gives flexibility for each event type, and can go beyond just an event (i.e. global, or just for a single vent type).

Does exactly what I need it to do too - great stuff!

[aryehraber]

Thanks @martyf, that's fantastic to hear! Thank you for opening this PR and getting the ball rolling, super appreciated 🙏

[martyf]

Thank you for such a helpful addon :)

[aryehraber]

FYI: just release v1.12.0 with these changes 🚀