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

Updated FlatFileDriver.php #308

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

wernerm
Copy link

@wernerm wernerm commented Jun 15, 2017

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

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
@acrobat
Copy link
Member

acrobat commented Sep 15, 2017

@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!

@wernerm
Copy link
Author

wernerm commented Sep 18, 2017

@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...?

@acrobat
Copy link
Member

acrobat commented Sep 19, 2017

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an empty line before return statements.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact the last assignment is not necessary, just return new ....

unlink($file);
}
}
rmdir($directory);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a new line after control structures.

*/
private function removeDirectoryRecursive($directory)
{
foreach (glob("{$directory}/{,.}[!.,!..]*", GLOB_MARK|GLOB_BRACE) as $file)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow PSR-2

private function getJobFiles($queueName) {
$iterator = $this->getJobIterator($queueName);
$files = array_keys(iterator_to_array($iterator));
return $files;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here as above.

@sagikazarmark
Copy link
Contributor

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?

@acrobat
Copy link
Member

acrobat commented Oct 8, 2017

Ping @wernerm

@evilfer
Copy link

evilfer commented Jul 4, 2018

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?

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.

4 participants