!!! FEATURE: Extend the QueueInterface with countReserved and countFailed #29
Conversation
daniellienert
force-pushed
the
feature/extend-interface
branch
from
5c30e18 to
2eb3fa4
Compare
daniellienert
changed the title
Also add countReady() and deprecate the count() command to clearify the interface
daniellienert
force-pushed
the
feature/extend-interface
branch
from
2eb3fa4 to
5a9ab5e
Compare
There was a problem hiding this comment.
This is a very useful addition, thank you!
But we should mark it breaking so that it does not end up in a minor update.
Besides I think we should have the corresponding PRs for the 3 implementations ready before we merge this one. I'm happy to help with that.
Lastly I don't think that we need to keep the count() method in the QueueInterface
| @@ -40,14 +42,15 @@ class QueueCommandController extends CommandController | |||
| * Displays all configured queues, their type and the number of messages that are ready to be processed. | |||
| * | |||
| * @return void | |||
| * @throws Exception | |||
There was a problem hiding this comment.
Our CGL don't state anything about the requirement of @throws annotations. Personally I'm strongly against those.
Obviously no reason to block this, but if we decide to add the annotations we should do it for the whole package, and maybe in a separate PR
| use Flowpack\JobQueue\Common\Queue\QueueManager; | ||
| use Neos\Flow\Annotations as Flow; | ||
| use Neos\Flow\Cli\CommandController; | ||
| use Neos\Flow\Mvc\Exception\StopActionException; |
There was a problem hiding this comment.
Why is this namespace imported?
There was a problem hiding this comment.
Nevermind, it's needed for the @throws annotations below
Classes/Queue/QueueInterface.php
Outdated
| * @return int The number of ready messages in the queue | ||
| * @deprecated Will be removed with the next major. Use countReady instead. | ||
| */ | ||
| public function count(): int; |
There was a problem hiding this comment.
If we change the interface we don't need to keep deprecated methods IMO as the implementations need to be adjusted anyways
| public function countFailedReturnsZeroByDefault() | ||
| { | ||
| $this->assertSame(0, $this->queue->countFailed()); | ||
| } |
There was a problem hiding this comment.
We should add tests effectively covering the new features:
countFailedReturnsNumberOfFailedMessages() and countReservedReturnsNumberOfReservedMessages()
daniellienert
changed the title
Which is replaced by countReady() now.
Comply with next major doctrine-common QueueInterface (see corresponding Pull Request: Flowpack/jobqueue-common#29)
There was a problem hiding this comment.
@daniellienert I adjusted the beanstalkd implementation (Flowpack/jobqueue-beanstalkd#11), added some functional tests covering the new features and tested it successfully with all our three implementations!
PS: I'm still opposed to introducing the @throws annotations (at least if we don't do it for all packages) and the new methods are the only ones with return types now. But those are nitpicks of course!
|
@bwaidelich: Thanks a lot! |
Also add countReady() and deprecate the count() command
to clearify the interface