AMQPEnvelope instead of AMQPBasicProperties

prolic / HumusAmqp

PHP 7.4 AMQP library

https://humusamqp.readthedocs.io/

MIT License

76 stars 17 forks source link

Closed thomasvargiu closed 7 years ago

thomasvargiu commented 7 years ago

coveralls commented 7 years ago

Changes Unknown when pulling c25ce1558af21c8931b89260f73f1ad96cdc44ce on thomasvargiu:fix/amqp-envelope into \ on prolic:master**.

coveralls commented 7 years ago

Changes Unknown when pulling 1ea277ed1c7e2d81dd5ac07415d4cb6f7fd61f55 on thomasvargiu:fix/amqp-envelope into \ on prolic:master**.

coveralls commented 7 years ago

Changes Unknown when pulling 41500eaf1e8770dd907146207987354dd721b57e on thomasvargiu:fix/amqp-envelope into \ on prolic:master**.

coveralls commented 7 years ago

Changes Unknown when pulling ae7e8776c12112652a423db5d5167327d40ea762 on thomasvargiu:fix/amqp-envelope into \ on prolic:master**.

prolic commented 7 years ago

There is a problem with this PR as it addresses too much things in a single PR, so I cannot cherry-pick - would have to take all or nothing.

I'm okay with some of those changes like:

declare (strict_types=1); to declare(strict_types=1);

Other things like the change of string concatination from:

$this->logger->error('Exception: ' . $e->getMessage()); to $this->logger->error('Exception: '.$e->getMessage());

is something I really don't like.

Another thing that makes it more unreadable:

if (! $this->lastDeliveryTag) { to if (!$this->lastDeliveryTag) {

the space here really makes is more readable.

@inheritdoc to {@inheritdoc} is good on the other hand.

@return integer to @return int is also good and would be accepted.

The fix for #24 is rejected cause of reasons explained in the given issue. This library will be updated as soon as php-amqp launches a new release.

So really sorry, I have to reject this PR as is.

thomasvargiu commented 7 years ago

You're right, I'm sorry. I will submit another pull request, first for older amqp extension versions, and another one for binding names.

prolic commented 7 years ago

forget about older php-amqp versions! They only have problems! 1.8.0 is coming the next days, and this library will be upgraded then.

thomasvargiu commented 7 years ago

Ok! Thanky you!

thomasvargiu commented 7 years ago

@prolic just another thing. In .travis.yml you have: - ./vendor/bin/php-cs-fixer fix -v --diff --dry-run so I applied these fix.

If you'll agree, I'll change it with --level=psr2.

prolic commented 7 years ago

no please not, I have this https://github.com/prolic/HumusAmqp/blob/master/.php_cs for php-cs rules - just updated a minute ago.

thomasvargiu commented 7 years ago

Much better! Thanks.