always declare a queue in factory, even with auto_setup_fabric disabled #61

prolic · 2017-11-25T11:49:15Z

alternative solution to #57

coveralls · 2017-11-25T14:23:17Z

Coverage remained the same at 92.686% when pulling a8b83e7 on declare_queue into 73a777b on master.

oqq

Maybe add a purge command for rabbitmq before each test to test every branch separated. Random queue/exchange names could also work therefor.

oqq · 2017-12-15T10:06:25Z

src/Container/QueueFactory.php

@@ -163,8 +163,6 @@ public function __invoke(ContainerInterface $container): Queue
                $exchangeObjects[$exchange] = ExchangeFactory::$exchange($container, $this->channel, true);
            }

-            $queue->declareQueue();


This would potentially fail, if auto_setup_fabric is true, since you would bind a queue to an exchange which is not declared.

prolic · 2017-12-16T14:17:08Z

How? When auto setup fabric is used, the exchange gets created first.

…

On Dec 15, 2017 6:10 PM, "Eric Braun" ***@***.***> wrote: ***@***.**** requested changes on this pull request. Maybe add a purge command for rabbitmq before each test to test every branch separated. Random queue/exchange names could also work therefor. ------------------------------ In src/Container/QueueFactory.php <#61 (comment)>: > @@ -163,8 +163,6 @@ public function __invoke(ContainerInterface $container): Queue $exchangeObjects[$exchange] = ExchangeFactory::$exchange($container, $this->channel, true); } - $queue->declareQueue(); This would potentially fail, if auto_setup_fabric is true, since you would bind a queue to an exchange which is not declared. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#61 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYEvN_tK-ms9dlAZ3sYr47g5ZKu7HHyks5tAkV7gaJpZM4QqZvU> .

oqq · 2017-12-16T16:05:23Z

Sorry, I expressed it badly.

You create an exchange and bind the queue to the exchange. And than you create the queue.
This would fail, since the queue would be bound to the exchange before the queue exists.

Currently, the tests doesn't fail, while you create the queue in another test. So every test should purge the whole rabbitmq queues/exchanges before run.

prolic · 2017-12-16T16:08:27Z

Ah I see. Will try to fix this tomorrow unless you provide a PR first.

…

On Dec 17, 2017 12:05 AM, "Eric Braun" ***@***.***> wrote: Sorry, I expressed it badly. You create a exchange and bind the queue to the exchange. And than you create the queue. This would fail, since the queue would be bound to the exchange before the queue exists. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#61 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYEvJADhvmtLA1MoRuTHTKyvnmUPYiHks5tA-pDgaJpZM4QqZvU> .

prolic · 2017-12-17T08:35:36Z

@oqq good to release now?

coveralls · 2017-12-17T08:43:30Z

Coverage decreased (-1.4%) to 91.299% when pulling d1da5f2 on declare_queue into 73a777b on master.

oqq · 2017-12-19T09:11:31Z

That seems not to be enough to fix the issue.
All queues where be created how expected. But the exchange binding would not happen.

So given for an exclusive rpc queue, the queue would be created but never be bind to an exchange. I still have to set auto_setup_fabric, but this would also create the exchange, which already exists on the broker with a configuration I am not always aware about.

prolic · 2017-12-23T10:51:13Z

ping @oqq - wanna check this?

coveralls · 2017-12-23T11:18:48Z

Coverage decreased (-1.4%) to 91.304% when pulling 2c48ecb on declare_queue into 73a777b on master.

oqq · 2017-12-31T10:53:20Z

src/Container/QueueFactory.php

+            $exchanges = iterator_to_array($exchanges);
+        }
+
+        if (! is_array($exchanges) || empty($exchanges)) {


It should not be necessary for this library to bind a queue to an exchange.
E.g. a service which purges queues (or handles other tasks) needs only to know the queue name, but not the name of the exchange.

oqq · 2017-12-31T11:03:45Z

src/Container/QueueFactory.php

-                        $bindArguments = $exchangeOption['bind_arguments'] ?? [];
-                        $this->bindQueue($queue, $exchangeName, $routingKeys, $bindArguments);
-                    }
+                $exchangeObjects[$exchange] = ExchangeFactory::$exchange($container, $this->channel, false);


Here my advice for a code cleanup:

$autoSetupFabric = $this->autoSetupFabric || $options['auto_setup_fabric']; if ($autoSetupFabric && isset($options['arguments']['x-dead-letter-exchange'])) { $exchangeName = $options['arguments']['x-dead-letter-exchange']; ExchangeFactory::$exchangeName($container, $this->channel, true); } foreach ($exchanges as $exchange => $exchangeOptions) { $exchangeObjects[$exchange] = ExchangeFactory::$exchange($container, $this->channel, $autoSetupFabric); }

prolic · 2017-12-31T12:12:55Z

An anonymous queue (without name) needs exchange binding upfront, so it can consume messages.

…

On Dec 31, 2017 19:06, "Eric Braun" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Container/QueueFactory.php <#61 (comment)>: > @@ -139,6 +139,19 @@ public function __invoke(ContainerInterface $container): Queue $queue->setFlags($this->getFlags($options)); $queue->setArguments($options['arguments']); + $exchanges = $options['exchanges']; + + if ($exchanges instanceof \Traversable) { + $exchanges = iterator_to_array($exchanges); + } + + if (! is_array($exchanges) || empty($exchanges)) { It should not be necessary for this library to bind a queue to an exchange. E.g. a service which purges queues (or handles other tasks) needs only to know the queue name, but not the name of the exchange. ------------------------------ In src/Container/QueueFactory.php <#61 (comment)>: > foreach ($exchanges as $exchange => $exchangeOptions) { - $exchangeObject = $exchangeObjects[$exchange]; - $exchangeName = $exchangeObject->getName(); - if (empty($exchangeOptions)) { - $this->bindQueue($queue, $exchangeName, [], []); - } else { - foreach ($exchangeOptions as $exchangeOption) { - $routingKeys = $exchangeOption['routing_keys'] ?? []; - $bindArguments = $exchangeOption['bind_arguments'] ?? []; - $this->bindQueue($queue, $exchangeName, $routingKeys, $bindArguments); - } + $exchangeObjects[$exchange] = ExchangeFactory::$exchange($container, $this->channel, false); Here my advice for a code cleanup: $autoSetupFabric = $this->autoSetupFabric || $options['auto_setup_fabric'];if ($autoSetupFabric && isset($options['arguments']['x-dead-letter-exchange'])) { $exchangeName = $options['arguments']['x-dead-letter-exchange']; ExchangeFactory::$exchangeName($container, $this->channel, true);}foreach ($exchanges as $exchange => $exchangeOptions) { $exchangeObjects[$exchange] = ExchangeFactory::$exchange($container, $this->channel, $autoSetupFabric);} — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#61 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYEvIUT14NnlaEvEgEBtU8LnxBhcwsEks5tF2rJgaJpZM4QqZvU> .

oqq · 2017-12-31T16:55:07Z

This factory is not used only for anonymous queues, or is it?

But I see also a service wich simply calls some amqp methods on an anonymous queue which already exists, without knowing her exchange bindings. This would be impossible to implemented with this library if I always need to define exchanges.

This would be a BC break by the way. ;)

prolic · 2018-01-02T08:53:37Z

@oqq I'm thinking about this solution:

only if queue has no name (anonymous queue) then we require exchange binding and do the binding, even if auto_setup_fabric is set to false
in all other cases the exchange binding is not needed

Additional I have another thought on auto_setup_fabric considering queues... current behaviour is, that the exchanges are also getting created. Maybe we remove this feature and add another option auto_setup_exchanges => true/false additionally to the queue factory.

As this is a bigger problem here which leads to unexpected errors, I'm ok with slightly breaking BC and will update docs/ release notes accordingly.

prolic · 2018-02-06T08:48:39Z

@oqq Sorry for taking so much time, I think the new behaviour here is good, I also updated the readme. Can you check please?

prolic · 2018-02-25T13:09:34Z

ping @oqq

oqq · 2018-03-03T07:47:43Z

@prolic this would still produce errors, if you try to bind a queue to an exchange before declaring it.
You call bindQueue() always before declareQueue(), which produces this error:

operation queue.bind caused a channel exception not_found: "no previously declared queue"

prolic · 2018-03-03T08:25:21Z

@oqq updated

oqq · 2018-03-03T09:07:08Z

I have an application with a complex infrastructure and all tests run successfully with this patch. So I assume this would fix the issue.

Thanks!

prolic added the enhancement label Nov 25, 2017

prolic changed the title ~~always declare a queue~~ always declare a queue in factory, even with auto_setup_fabric disabled Nov 25, 2017

oqq suggested changes Dec 15, 2017

View reviewed changes

oqq reviewed Dec 31, 2017

View reviewed changes

prolic added 7 commits February 6, 2018 16:44

always declare a queue

2338d2a

update tests

fff0bad

fix queue factory

e839002

fix queue factory exchange binding

5b1c538

adjust tests

60bf048

update queue factory behaviour and readme

f516eef

it's 2018!

a3706b0

prolic force-pushed the declare_queue branch from 992e2e0 to a3706b0 Compare February 6, 2018 08:46

update php-amqp

b724cf1

prolic force-pushed the declare_queue branch from b3315b3 to b724cf1 Compare February 6, 2018 09:48

fix docheader

4d136af

prolic added 2 commits March 3, 2018 16:23

declare queue earlier

4200378

bump phpunit

f217e54

reduce phpunit to 6.5

54cb329

oqq approved these changes Mar 3, 2018

View reviewed changes

prolic merged commit 45b7037 into master Mar 3, 2018

prolic deleted the declare_queue branch March 3, 2018 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

always declare a queue in factory, even with auto_setup_fabric disabled #61

always declare a queue in factory, even with auto_setup_fabric disabled #61

prolic commented Nov 25, 2017

coveralls commented Nov 25, 2017 •

edited

Loading

oqq left a comment

oqq Dec 15, 2017

prolic commented Dec 16, 2017 via email

oqq commented Dec 16, 2017 •

edited

Loading

prolic commented Dec 16, 2017 via email

prolic commented Dec 17, 2017

coveralls commented Dec 17, 2017 •

edited

Loading

oqq commented Dec 19, 2017

prolic commented Dec 23, 2017

coveralls commented Dec 23, 2017 •

edited

Loading

oqq Dec 31, 2017

oqq Dec 31, 2017

prolic commented Dec 31, 2017 via email

oqq commented Dec 31, 2017

prolic commented Jan 2, 2018

prolic commented Feb 6, 2018

prolic commented Feb 25, 2018

oqq commented Mar 3, 2018

prolic commented Mar 3, 2018

oqq commented Mar 3, 2018

always declare a queue in factory, even with auto_setup_fabric disabled #61

always declare a queue in factory, even with auto_setup_fabric disabled #61

Conversation

prolic commented Nov 25, 2017

coveralls commented Nov 25, 2017 • edited Loading

oqq left a comment

Choose a reason for hiding this comment

oqq Dec 15, 2017

Choose a reason for hiding this comment

prolic commented Dec 16, 2017 via email

oqq commented Dec 16, 2017 • edited Loading

prolic commented Dec 16, 2017 via email

prolic commented Dec 17, 2017

coveralls commented Dec 17, 2017 • edited Loading

oqq commented Dec 19, 2017

prolic commented Dec 23, 2017

coveralls commented Dec 23, 2017 • edited Loading

oqq Dec 31, 2017

Choose a reason for hiding this comment

oqq Dec 31, 2017

Choose a reason for hiding this comment

prolic commented Dec 31, 2017 via email

oqq commented Dec 31, 2017

prolic commented Jan 2, 2018

prolic commented Feb 6, 2018

prolic commented Feb 25, 2018

oqq commented Mar 3, 2018

prolic commented Mar 3, 2018

oqq commented Mar 3, 2018

coveralls commented Nov 25, 2017 •

edited

Loading

oqq commented Dec 16, 2017 •

edited

Loading

coveralls commented Dec 17, 2017 •

edited

Loading

coveralls commented Dec 23, 2017 •

edited

Loading