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.4] Fix Queue Prefix bug #18982

Merged
merged 1 commit into from
Apr 28, 2017
Merged

[5.4] Fix Queue Prefix bug #18982

merged 1 commit into from
Apr 28, 2017

Conversation

laurencei
Copy link
Contributor

@laurencei laurencei commented Apr 28, 2017

The queue prefix introduced in 5.4.20 from here (#18860) is broken and does not allow queue processing. This PR should fix issue #18978

The reason is the queue driver is prefixing the prefix on all queues - which is correct. However, currently when you run queue:work, it is also prefixing the queue, before passing it to the driver, which adds another prefix:

prefix-prefix-default

It is even worse for queue:listen - which prefixes, passes it to queue:work, adds another prefix, and then passes it to the driver:

prefix-prefix-prefix-default

This PR removes the prefixing from the worker & listener. They dont need to worry about prefixing - but that is handling by the drivers upon receiving a queue name.

There is also a second bug specifically related to the database queue driver. It adds another prefix during the table lookup unneccessarily, resulting in an extreme instance of:

prefix-prefix-prefix-prefix-default

I've done some initial testing on this PR and it all seems to work on my integration tests. But feel free to test some more.

@taylorotwell taylorotwell merged commit 9173ede into laravel:5.4 Apr 28, 2017
@taylorotwell
Copy link
Member

Well that's frustrating.

@laurencei
Copy link
Contributor Author

Yeah. I'm trying to think of a good test to avoid regression?

@taylorotwell
Copy link
Member

I don't know. Anything really. I don't want to make another release until we confirm this is fixed. @browner12

@it-can
Copy link
Contributor

it-can commented Apr 28, 2017

@laurencei in my case this fix works with Beanstalk! Thanks!

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