always declare a queue in factory, even with auto_setup_fabric disabled

[prolic]

alternative solution to https://github.com/prolic/HumusAmqp/pull/57

ping @oqq

[coveralls]

Coverage remained the same at 92.686% when pulling a8b83e7833a543a0792c322927e23a938131804d on declare_queue into 73a777bed565a072f1653fd50a1680fb4bb48080 on master.

[prolic]

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

On Dec 15, 2017 6:10 PM, "Eric Braun" notifications@github.com wrote:

@oqq 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 https://github.com/prolic/HumusAmqp/pull/61#discussion_r157163393:

@@ -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 https://github.com/prolic/HumusAmqp/pull/61#pullrequestreview-83763471, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYEvN_tK-ms9dlAZ3sYr47g5ZKu7HHyks5tAkV7gaJpZM4QqZvU .

[oqq]

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]

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

On Dec 17, 2017 12:05 AM, "Eric Braun" notifications@github.com 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 https://github.com/prolic/HumusAmqp/pull/61#issuecomment-352192462, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYEvJADhvmtLA1MoRuTHTKyvnmUPYiHks5tA-pDgaJpZM4QqZvU .

[prolic]

@oqq good to release now?

[coveralls]

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

[oqq]

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]

ping @oqq - wanna check this?

[coveralls]

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

[prolic]

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

On Dec 31, 2017 19:06, "Eric Braun" notifications@github.com wrote:

@oqq commented on this pull request.

In src/Container/QueueFactory.php https://github.com/prolic/HumusAmqp/pull/61#discussion_r159137932:

@@ -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 https://github.com/prolic/HumusAmqp/pull/61#discussion_r159138069:

         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 https://github.com/prolic/HumusAmqp/pull/61#pullrequestreview-86044860, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYEvIUT14NnlaEvEgEBtU8LnxBhcwsEks5tF2rJgaJpZM4QqZvU .

[oqq]

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]

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

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

ping @oqq

[oqq]

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

@oqq updated

[oqq]

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!