Skip to content
This repository has been archived by the owner on Feb 1, 2022. It is now read-only.

Add: Rewrite to support latest Laminas, Pheanstalk and PHPUnit #67

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

Conversation

taat
Copy link

@taat taat commented Nov 17, 2020

Main objectives:

  • Support Laminas framework (after rebranding from Zend Framework 3)
  • Support latest Pheanstalk
  • Support modern PHPUnit
  • still be compatible with SlmQueue

Travis not touched yet.
Needs cleanup, adding comments, better docs, removing old files etc.

Still dirty, but does the job.
Please review and let me know.

Copy link
Contributor

@roelvanduijnhoven roelvanduijnhoven left a comment

Choose a reason for hiding this comment

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

@taat Thank you. This looks promising! I was busy last week, but I'll be happy to help you now to finish this by doing reviews somewhat quicker!

I left some comments about some minor things. But I think you are heading in the right direction.

I'll happily review again once you polished your PR and removed your own interal TODOs.

Best of luck, and take care!

'defaults' => array(
'controller' => 'SlmQueueBeanstalkd\Controller\BeanstalkdWorkerController',
'options' => [
'route' => 'queue Beanstalkd <queue> [--timeout=] --start',
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep the lower case b. I think that is better, and it is identical to before.

Copy link
Contributor

Choose a reason for hiding this comment

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

I find the way we use an argument to decide what action we are going to do perform a little bit strange. Let's remodel those to:

  • queue beanstalkd start <queue> [--timeout]
  • queue beanstalkd recover <queue>
  • etc..

],
],
],
'slm_queue' => [
Copy link
Contributor

Choose a reason for hiding this comment

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

I think everything below this line is some dummy / TODO code, that should now be removed. Right?

return array('SlmQueue');
}
}
require __DIR__ . '/src/Module.php';
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you configure your IDE so each line ends with a return? See the warning GitHub issues here.

}

public function statsAction() {
// @todo
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO


use Pheanstalk\Contract\JobIdInterface;

class JobId implements JobIdInterface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting is of here. You can use https://github.com/JouwWeb/SlmQueue/blob/master/phpcs.xml to auto format your files if you want.

$this->id = $id;
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to remove the PHPDoc on every method call that is allready fully typed :). Or remove useles comments about constructors and such.

{
return [
'queue beanstalkd <queue> --start [--timeout=]' => 'Process Beanstalkd queue',
'queue beanstalkd <queue> --recover [--executionTime=]' => 'Recover Beanstalkd worker',
Copy link
Contributor

Choose a reason for hiding this comment

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

stats is missing here.

use Pheanstalk\Pheanstalk;

class JobOptions extends AbstractOptions {
protected $__strictMode__ = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

What does strict mode do? And why is it wrapped around with __? Please do add a comment about this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants