Updated FlatFileDriver.php #308
Conversation
Bug fixes and minor (opinionated) improvements for the flat file driver. * Changed from using an additional suffix (.proceed) to hidden (dot-prefixed) files to emulate a non-visible message state * Fixed incomplete queue removal * Fixes for inconsistent FIFO processing, message ordering and listing - changed from LIFO to FIFO throughout * Refactored some repetitive code
|
@wernerm Thanks for your PR! Can you rebase the branch onto the current master and check the travis test-suite as the tests are failing. Thanks! |
|
@acrobat The test is failing because it expects the last added item to be popped first (last-in-first-out/LIFO) whereas I believe the first added item should be popped first (first-in-first-out/FIFO). The reason I made the change was for consistency (the behaviour of the other drivers bundled with bernard). In my opinion, FIFO is also the expected behaviour. Maybe I should update the test in the new PR...? |
|
Hmm yes indeed! @sagikazarmark what do you think? Change the FlatFileDriver also to a FIFO instead of LIFO? Sounds better to me if I think about my implementations with queue systems, but it's a BC break ofcourse |
| private $baseDirectory; | ||
|
|
||
| /** | ||
| * @var integer |
There was a problem hiding this comment.
For now, private properties have no docblock on purpose. This might change, but in the meantime they should go away for consistency.
|
|
||
| /** | ||
| * Creates an iterator of all message files in the queue | ||
| * @param string $queueName |
There was a problem hiding this comment.
Please use newlines between description, param and return blocks.
| private function getJobIterator($queueName) { | ||
| $queueDir = $this->getQueueDirectory($queueName); | ||
| $iterator = new \GlobIterator($queueDir.DIRECTORY_SEPARATOR.'*.job', \FilesystemIterator::KEY_AS_FILENAME); | ||
| return $iterator; |
There was a problem hiding this comment.
Please add an empty line before return statements.
There was a problem hiding this comment.
In fact the last assignment is not necessary, just return new ....
| unlink($file); | ||
| } | ||
| } | ||
| rmdir($directory); |
There was a problem hiding this comment.
Please add a new line after control structures.
| */ | ||
| private function removeDirectoryRecursive($directory) | ||
| { | ||
| foreach (glob("{$directory}/{,.}[!.,!..]*", GLOB_MARK|GLOB_BRACE) as $file) |
| private function getJobFiles($queueName) { | ||
| $iterator = $this->getJobIterator($queueName); | ||
| $files = array_keys(iterator_to_array($iterator)); | ||
| return $files; |
|
Like the idea, thanks for the PR. Don't even know why the FlatFile driver is not FIFO in the first place. Can you please take a look at my comments and rebase your branch? |
|
Ping @wernerm |
|
Hi, a FIFO flat drive would be very useful. To avoid breaking changes, it could perhaps be configurable FIFO/LIFO. Is this going to be merged? |
Bug fixes and minor (opinionated) improvements for the flat file driver.