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

[5.x] Set lastPushed when executing the delayed enqueue closure #951

Merged
merged 1 commit into from
Jan 4, 2021

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented Jan 4, 2021

This fixes #950

$this->lastPushed = $job; will run when the closure is executed. Before this PR it was set before the closure is executed so multiple jobs could have been dispatched after it.

@wouterrutgers
Copy link

Horizon doesn't work anymore for me after this update. It throws an exception Call to undefined method Laravel\Horizon\RedisQueue::enqueueUsing().

Without debugging this thoroughly, I noticed you wrapped the same call in a method_exists somewhere else in the same file: https://github.com/themsaid/horizon/blob/6c3704e9fb8eba4947a3a5f7e4a2968c0572d215/src/RedisQueue.php#L106.

Is that something this new block of code should have been wrapped in as well?

@driesvints
Copy link
Member

@wouter2203 Does updating the framework to the latest version help?

@wouterrutgers
Copy link

@driesvints Yes, it does, that's something we noticed as well indeed. It probably has to do something with this update on the framework: laravel/framework#35415

I guess that's what we get when we have the framework locked at a specific version and having horizon at ^5.0. ;)

However, for the sake of clean code, I guess the method_exists should still be added. Or remove the other check as well and maybe bump the minimum version of illuminate/queue to whatever version the update was added in illuminate/queue.

FYI: we have the framework locked at a specific version because it broke our project once while only a minor version was released. It had something to do with an edge case, but we manually update the framework since then so we can run our test suite and see if everything is still working as expected.

@driesvints
Copy link
Member

Yeah we should bump the minimum version for illuminate/queue. I'll check in on that but I guess it's sort of too late since we already tagged a new version.

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