Migrate to AMQP interop. #276
Conversation
|
Please write a changelog directed at end users. Keep in mind that they probably do not care what file or class you changed, but what features you provided / what bugs you fixed. Try to answer the question "What benefit do I get from upgrading?" |
Backend/AMQPBackend.php
Outdated
| $message->getValue('AMQMessage')->delivery_info['channel']->basic_recover($message->getValue('AMQMessage')->delivery_info['delivery_tag']); | ||
| } elseif (null !== $this->deadLetterExchange) { | ||
| $message->getValue('AMQMessage')->delivery_info['channel']->basic_reject($message->getValue('AMQMessage')->delivery_info['delivery_tag'], false); | ||
| } |
There was a problem hiding this comment.
there is no else clause and if recover is false and no dead letter exchange available messages will be unacked and everything stop working.
Backend/AMQPBackendDispatcher.php
Outdated
| */ | ||
| public function getChannel() | ||
| { | ||
| throw new \LogicException('Not used any more'); |
There was a problem hiding this comment.
That looks like a BC-break. Should it call getContext() instead?
There was a problem hiding this comment.
Forgot to remove it.
There was a problem hiding this comment.
There are some BC breaks in case of a user extends classes. The API is the same though.
Iterator/AMQPMessageIterator.php
Outdated
| ++$this->counter; | ||
| $this->isValid = true; | ||
| } else { | ||
| $this->isValid = false; |
There was a problem hiding this comment.
Please use a return statement and remove the else
There was a problem hiding this comment.
not sure I got it. I dont return anything from next method. The value is stored and used in another method: valid.
There was a problem hiding this comment.
I mean
$this->isValid = true;
return;
}
$this->isValid = false;
Backend/AMQPBackend.php
Outdated
| } elseif (null !== $this->deadLetterExchange) { | ||
| $message->getValue('AMQMessage')->delivery_info['channel']->basic_reject($message->getValue('AMQMessage')->delivery_info['delivery_tag'], false); | ||
| } | ||
| $this->consumer->reject($amqpMessage, $this->recover); |
There was a problem hiding this comment.
It would be good if you could thoroughly review this part. I might've broken it cuz I did not quite get the purpose of basic_recover.
There was a problem hiding this comment.
I didn't write the package that well and can't test it (I don't think I even use it). Maybe someone in @sonata-project/contributors can?
composer.json
Outdated
| @@ -37,12 +37,12 @@ | |||
| "sonata-project/doctrine-orm-admin-bundle": "^3.0", | |||
| "guzzlehttp/guzzle" : "If you want to get a status report when using the rabbitMQ backend.", | |||
| "liip/monitor-bundle": "^1.0", | |||
| "php-amqplib/php-amqplib": "^2.0" | |||
| "enqueue/amqp-lib": "^0.8" | |||
There was a problem hiding this comment.
No requirement for the interop package?
Backend/AMQPBackendDispatcher.php
Outdated
| $this->settings['vhost'] | ||
| ); | ||
| if (!$this->context) { | ||
| if (false == array_key_exists('factory_class', $this->settings)) { |
There was a problem hiding this comment.
Please use !, the return value is already a boolean, no need to compare it to another boolean
Backend/AMQPBackendDispatcher.php
Outdated
|
|
||
| $factoryClass = $this->settings['factory_class']; | ||
| if (false == class_exists($factoryClass) || false == (new \ReflectionClass($factoryClass))->implementsInterface(AmqpConnectionFactory::class)) { | ||
| throw new \LogicException(sprintf('The factory_class option has to be valid class that implements "%s"', AmqpConnectionFactory::class)); |
There was a problem hiding this comment.
Please linebreak long lines
| ->scalarNode('factory_class') | ||
| ->cannotBeEmpty() | ||
| ->isRequired() | ||
| ->defaultValue(AmqpConnectionFactory::class) |
There was a problem hiding this comment.
Please add a call to info()
Backend/AMQPBackendDispatcher.php
Outdated
| ) { | ||
| throw new \LogicException(sprintf( | ||
| 'The factory_class option has to be valid class that implements "%s"', | ||
| AmqpConnectionFactory::class) |
There was a problem hiding this comment.
This is not PSR-2 compliant, and I wrote the php-cs-fixer that fixes that \o/
There was a problem hiding this comment.
Sadly we use outdated versions of the cs-fixer :(
Backend/AMQPBackendDispatcher.php
Outdated
| @@ -229,7 +235,7 @@ public function initialize() | |||
| */ | |||
| protected function getApiQueueStatus() | |||
| { | |||
| if (false === class_exists('Guzzle\Http\Client')) { | |||
| if (!class_exists('Guzzle\Http\Client')) { | |||
There was a problem hiding this comment.
Please use the ::class notation as much as you can.
|
👎 To remove php-amqplib. This is a very used library with a already provided bundle: https://packagist.org/packages/php-amqplib/ It should be kept. Can't we propose both usage? |
|
It still uses php-amqplib but through amqp interop layer which allows us to change it easily to something else. Why should we keep two same things? |
|
@makasim Not sure to get it. Are you telling |
@soullivaneuh Indeed! queue\amqp interops are like PSR-3 for logs or PSR-7 for HTTP but for messaging.
They are all interchangeable thanks to the interop. By default it suggests |
|
Oh right, just forget my comment in this case! 👍 |
|
I think this should be done on the stable branch, and we should keep the php-amqplib requirement, and remove it on next major. |
|
@greg0ire Agreed, BC must be prioritized if doable. |
Sorry, I don't get the idea. Currently, PR is opened against a master branch. It is the right place for incompatible changes, right? |
|
It is, but if you think you can make the PR backwards compatible, then you should target stable instead. |
|
I can add new files and classes instead of re-writing phpamqp-lib related. Is it way to go? |
|
I think as long as you don't change public or protected method signatures you are fine. You should also try to keep all existing protected properties and deprecated those you need to. |
|
You can simply rebase if you want to get rid of them. |
fix doc
880f3ea to
9369846
Compare
|
@greg0ire done (: |
| @@ -19,4 +19,4 @@ test: | |||
| phpunit -c phpunit.xml.dist --coverage-clover build/logs/clover.xml | |||
|
|
|||
| docs: | |||
| cd Resources/doc && sphinx-build -W -b html -d _build/doctrees . _build/html | |||
| cd docs && sphinx-build -W -b html -d _build/doctrees . _build/html | |||
There was a problem hiding this comment.
You still have that fix, and should not I think.
There was a problem hiding this comment.
well, even after I had merged the master, it was failing.
I tried several times without success. dont know why it was like that.
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh it's another file as in my PR!
There was a problem hiding this comment.
Then leave it like that, it's fine.
There was a problem hiding this comment.
The strange thing is. why the tests passed in your fix branch?
There was a problem hiding this comment.
Because I did not change any docs 😓
|
Thanks a lot for your patience @makasim ! |
|
Party time! @greg0ire Could you merge that to master too so I can work on deprecation removal PR? |
|
I'll get right on to it! |
|
dont you mind adding the bundle to the list here https://github.com/queue-interop/queue-interop#projects? |
|
Sure, go ahead! |
|
It's merged, you can follow up on the deprecation removal :) |
|
The changelog line is waaaaaaaaaaaaaaaaaaaay too long. |
| @@ -1,6 +1,16 @@ | |||
| UPGRADE 3.x | |||
| =========== | |||
|
|
|||
| UPGRADE FROM 3.2 to 3.3 | |||
There was a problem hiding this comment.
|
Just received the update and got The child node "factory_class" at path "sonata_notification.backends.rabbitmq.connection" must be configured. Not sure what I am supposed to aim the factory_class towards. |
|
@jayesbe there is a default value, it should kick in: https://github.com/sonata-project/SonataNotificationBundle/pull/276/files#diff-978c02ccc8ccdf14cb8a58dc91ab346cR117 Maybe one of these calls needs to be removed? |
|
perhaps the default needs to be move prior to will try. |
|
I don't think the order matters, but please do try, for science |
|
order doesn't matter.. but removing the |
|
Can you please make a PR for that? |
I am targeting this branch, because it contains BC breaks.
Closes #275
Changelog
To do
Subject
The PR replaces php-amqplib usage with the solution based on AMQP interop. There are several reasons for that: