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

- Add support for reserve-job. Added in beanstalkd 1.12+ #222

Merged
merged 7 commits into from Feb 3, 2021

Conversation

scottmac
Copy link

@scottmac scottmac commented Aug 23, 2020

Before 1.12, we'll return null if this isn't supported.

Fixes #221

@SamMousa
Copy link
Collaborator

Shouldn't we throw an exception? Returning null is not optimal for use cases where you reasonably expect it to return a value.

@scottmac
Copy link
Author

I have no strong opinion on this, happy to change :)

@SamMousa
Copy link
Collaborator

Let me have a look tomorrow, will get back to you!

- Throw an Unknown Command exception for older versions
@scottmac
Copy link
Author

Actually, I like that idea.

Also re-throwing an Unknown Command for older versions seems right

@SamMousa
Copy link
Collaborator

What happens if the response is not found?

Also there seem to be no tests included.

@SamMousa SamMousa mentioned this pull request Aug 25, 2020
7 tasks
src/Pheanstalk.php Outdated Show resolved Hide resolved
@SamMousa SamMousa self-requested a review August 25, 2020 07:29
@seth-xdam
Copy link

What's happening with this? I've forked the repository specifically to merge in and use this feature. I'd rather move back to yours, though.

@SamMousa
Copy link
Collaborator

SamMousa commented Feb 2, 2021

Apologies, shifting priorities, I'll try to plan some time for this soon

@seth-xdam
Copy link

Cool. If it helps, we've been using it in production without making any changes besides what's already in this PR. Thanks for your work!

@SamMousa
Copy link
Collaborator

SamMousa commented Feb 3, 2021

I'm merging this, but it will only be available in the next major version of this library, just an FYI.
That version will very likely require PHP8.

@SamMousa SamMousa merged commit 92e8b2f into pheanstalk:master Feb 3, 2021
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.

Support for reserve-job
3 participants