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

Ability to not run defer on validation errors #52890

Open
wants to merge 4 commits into
base: 11.x
Choose a base branch
from

Conversation

allanmcarvalho
Copy link

@allanmcarvalho allanmcarvalho commented Sep 23, 2024

Refer feature is very flexible and robust, but when using it I found a point where it does not have the desired behavior and at the same time does not implement anything that makes this possible.

I'm referring to preventing defer from being called when validation errors exist.

Where might this be useful? In my case, for example, I send an upload using FilePond in a request before sending the form, so in the form I only send the ID of the file sent previously. If validation passes, I process the file and then use the refer() to delete the original file without harming the response.

It is important to emphasize that I call defer within a request class, that is, when validation fails, defer has already been configured.

// Use it is easy
  defer(fn() => Filepond::delete($id))->evenWithErrors(false);

@devajmeireles
Copy link
Contributor

IMHO, it doesn't make sense to have it as part of the default defer behavior. We can simplify it by just executing defer when there are no validation errors, all based on a simple if, or if you want to think about useful features for defer, maybe:

defer(fn () => /* ... */)->when(true);

defer(fn () => /* ... */)->unless(true);

@allanmcarvalho
Copy link
Author

allanmcarvalho commented Sep 23, 2024

IMHO, it doesn't make sense to have it as part of the default defer behavior. We can simplify it by just executing defer when there are no validation errors

Yes, this should be the best way for most cases. However, I am creating a package for Lavavel that creates some macros. For example, in a macro for a request, it takes an ID sent in the request post and transforms it into an UploadedFile (FilePond example above) and merges it into the request. After doing this merge, it should delete the file at the end of the request since it is expected that the code gave the correct end to the file. However, as we can imagine, in case of a validation error the package cannot delete the file.
In other words, for native implementation in the application, you not only can, but should call defer only after concluding that it is safe to call, but in some cases, such as when developing packages, you may want to cancel or no longer call this defer. The implementation of defer itself already foresees something similar since it is possible to give it a name, to later cancel it.

or if you want to think about useful features for defer, maybe:

defer(fn () => /* ... */)->when(true);

defer(fn () => /* ... */)->unless(true);

Your idea of ​when and unless sounds good to me. If it is in the maintainers' interest, we can cancel this PR and create another one aimed at this path. This way we can pass callbacks to these methods to decide after the response whether or not the defer should be called.

@timacdonald
Copy link
Member

timacdonald commented Sep 23, 2024

Isn't defer(fn () => /* ... */)->always() the answer to this? We built this specifically so that deferred closure run when exceptions occur.

@allanmcarvalho
Copy link
Author

allanmcarvalho commented Sep 23, 2024

Isn't defer(fn () => /* ... */)->always() the answer to this? We built this specifically so that deferred closure run when exceptions occur.

No @timacdonald, the scenario is the opposite of always. For example, Laravel by default does not call defer if an error >= 400 occurs (unless always is configured). What I see here as a new feature is not calling defer in cases where there are validation errors.

Validations are treated by Laravel as redirects, that is, they are not exceptions. Therefore, the defer here may in some cases have an unexpected effect for the user in scenarios where it has been called before validation.

@allanmcarvalho
Copy link
Author

If @taylorotwell doesn't agree with this view, we can put a "conditional" attribute on the DeferredCallback class. This way, we can create a "defer_if" helper function passing it a boolean or a callback that returns a boolean that will decide whether or not the defer will be called..

@timacdonald
Copy link
Member

I see. I think we should instead add an escape hatch to the middleware so you have complete control over when the defer functions are invoked.

Especially because the proposed evenWithErrors functionality is tied to HTTP, whilst defer is execution context agnostic.

//Service provider.

InvokeDeferredCallbacks::when(fn ($request, $response, $callback) => /* ... */);

@allanmcarvalho
Copy link
Author

I think defer_if is a better choice, because it also allows this control to packages (plugins) that decide to use it. This way, the defer issuer (package or application) can create a conditional (which will be analyzed after the response if it is a Closure) to decide if from the moment it was defined until the response it remains valid and should be called.

If we consider that defer will only be used by end users (application), I believe your idea is the best. However, there are cases in which packages can use a conditional directly in the function, giving the sender more granularity and control over the decision to issue or not.

// Any package service provider

defer_if(fn() => Auth::user()->notifications->whereNull('read')->exists(), fn() => Notification::Dispatch(Auth::user())); 

In the above example, the package has control.

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.

3 participants