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

Add AmqpLib support #136

Merged
merged 9 commits into from Jul 21, 2017
Merged

Add AmqpLib support #136

merged 9 commits into from Jul 21, 2017

Conversation

fibula
Copy link
Contributor

@fibula fibula commented Jul 18, 2017

fixes #128

composer.json Outdated
@@ -7,6 +7,7 @@
"enqueue/enqueue": "*@dev",
"enqueue/stomp": "*@dev",
"enqueue/amqp-ext": "*@dev",
"enqueue/amqplib": "*@dev",
Copy link
Member

Choose a reason for hiding this comment

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

Could you please rename it to amqp-lib? Same goes for the composer package name

private $config;
private $connection;

public function __construct(array $config = [])
Copy link
Member

Choose a reason for hiding this comment

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

can you please add doc blocks for the properties and methods

class AmqpConsumer implements PsrConsumer
{
private $channel;
private $queue;
Copy link
Member

Choose a reason for hiding this comment

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

we use php-cs-fixer to make sure the code looks similar. Could you please use it too? https://github.com/php-enqueue/enqueue-dev/blob/master/docs/contribution.md


public function receive($timeout = 0)
{
$end = microtime(true) + ($timeout / 1000);
Copy link
Member

Choose a reason for hiding this comment

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

it would be good to add a basic_consume support. But it could be done at a later stage.

@@ -0,0 +1,49 @@
<?php

namespace Enqueue\Amqplib;
Copy link
Member

Choose a reason for hiding this comment

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

Could you please change it to Enqueue\AmqpLib?

@makasim
Copy link
Member

makasim commented Jul 18, 2017

@fibula It would be also great if you could add some tests. You can use queue-spec at least. It should not be hard to add them

https://github.com/queue-interop/queue-spec

{
$end = microtime(true) + ($timeout / 1000);

while (0 === $timeout || microtime(true) < $end) {
Copy link
Member

Choose a reason for hiding this comment

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

would be able to add a support of basic_consume. You can look how we did it in amqp ext transport

Copy link
Contributor Author

@fibula fibula Jul 19, 2017

Choose a reason for hiding this comment

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

ok, I will give a try

@makasim makasim merged commit ef4a72f into php-enqueue:master Jul 21, 2017
ASKozienko pushed a commit that referenced this pull request Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Amqp lib support
2 participants