-
Notifications
You must be signed in to change notification settings - Fork 15
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
fix: handle errors in closures [1.x] #16
Conversation
Could you add some tests? |
@stancl sorry for the delay but finally I could manage to work on it 😊 |
@stancl okay, I noticed that the proposed change is maybe just too much. Instead I could simply rely on a closure but the issue right now is that errors are not handled properly. My recent changes should fix it with minimal invasion. Could you please have another look? |
Can you update the PR desc? |
@stancl sorry, done 😊 |
Bit confused about what exactly the test is doing. The entire try-catch is inside the closure? |
Oh, wow, you were right... seems I was very tired and too happy that the test executed at all 😁 I hope it's now working better. |
The diff looks good now. I've confirmed the added test fails without the change. Can you open an additional PR for the 2.x branch? I'd merge them at once. |
When you pass a closure instead of a class there is obviously no
failed
method which can be called in case of a failure. This PR adds a simple check and let's the exception bubble up.I raised the PR to 1.x (because that's what I use with Tenancy package) but can of course adapt the changes to 2.x as well if you want.