-
Notifications
You must be signed in to change notification settings - Fork 15
feat: Queue::push()
to return QueuePushResult instead of boolean
#62
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
base: develop
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 15520363740Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning a string is simple, but if I'm going to experience a breaking change, I'd rather gain something from it—like meaningful error details. That's why I think QueuePushResult
is a better fit here.
class QueuePushResult
{
public function __construct(
public bool $success,
public ?string $jobId = null,
public ?string $error = null
) {}
}
In any case, this behavior change should be clearly documented for users.
@datamweb Thank you for the feedback. I'm leaning toward implementing |
Thanks for your thoughtful question. While queue push failures might not be very common, when they do happen, identifying the root cause can be quite difficult—especially when no exception is thrown. In my first experience using the Queue system, I encountered a failure due to a misconfiguration related to an outdated database version. I also saw(slack) another user facing the same issue( Introducing a structured response like Right now, this is just a proposed enhancement to improve developer experience, but the pattern has potential to be extended to other methods gradually. We can also invite other team members to contribute and provide input as we evolve this design. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against using a result object as the return type as long as it will bring better dev experience.
Queue::push()
to return job ID instead of booleanQueue::push()
to return QueuePushResult instead of boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks clean and well-structured. One small suggestion: it would’ve been great if the methods getStatus()
, getJobId()
, and getError()
were documented with PHPDoc to improve readability and maintainability.
That said, everything looks great overall. Nice work! 👌
I thought these were quite clear and self-explanatory, but added. |
Description
This PR updates thepush()
method to return the unique job ID of the queued job instead of a simplebool
. This allows consumers to track jobs based on their identifier.This PR updates the
push()
method to return aQueuePushResult
object instead of a simplebool
. This change provides more detailed information about the outcome of the push operation, including the unique job ID and any error message.Queue::push()
now returns astring
(job ID) on success ornull
on failureMethod signature updated frombool
to?string
push()
is no longerbool
, but aQueuePushResult
object.bool
return must be updated to check->getStatus()
instead.Closes #61
Checklist: