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

[8.x] Allow adding catch callbacks for chains #33364

Merged
merged 8 commits into from
Jun 29, 2020

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented Jun 28, 2020

This PR add a new Bus::chain() method similar to Bus::batch() and also allows attaching catch callbacks to chain:

Bus::chain([
    new Job(),
    new Job2(),
    new Job3()
])->catch(function ($e) {
    info($e->getMessage());
})->dispatch();

The callbacks will be invoked if any of the jobs fail.

*/
public function invokeChainCatchCallbacks($e)
{
collect($this->chainCatchCallbacks)->each->__invoke($e);
Copy link
Member

@GrahamCampbell GrahamCampbell Jun 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think userland code should ever call dynamic methods like that? Can we not just literally invoke it?

(collect($this->chainCatchCallbacks)->each)($e);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GrahamCampbell hmmm what's the reason?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic methods are not meant to be called directly. It's hacky to do that. Just like it's hacky to use reflection to make a private method callable and then call it. Those methods are there to be used by PHP native syntax. __invoke is meant to be called only be actually calling the class and __get is meant to be called only by doing a property access, etc. Obviously, there are exceptions, but I don't think this is one of those cases.

@taylorotwell taylorotwell merged commit c93ee2e into laravel:master Jun 29, 2020
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