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

Fix content_type for AMQP Message #740

Merged
merged 2 commits into from
Mar 10, 2016
Merged

Fix content_type for AMQP Message #740

merged 2 commits into from
Mar 10, 2016

Conversation

macintoshplus
Copy link
Contributor

If use the AmqpHandler with the php-ext amqp, the content type is plain/text for each message in the queue because the property name is wrong written.

This pull request, change the property name Content-type by correct property name content_type.

The patch work fine with RabbitMQ.

The property "Content-type" does not exist. Use "content_type" instead.

Work for RabbitMQ.
@Seldaek
Copy link
Owner

Seldaek commented Mar 10, 2016

https://www.rabbitmq.com/tutorials/amqp-concepts.html says:

AMQP peers typically use the "content-type" and "content-encoding" fields to communicate this information, but this is by convention only.

This does not really reassure me that this PR is correct.. Any clue @videlalvaro? :)

@videlalvaro
Copy link

Per protocol spec it should be content-type, but not sure how the library maps this internally. See https://www.rabbitmq.com/resources/specs/amqp-xml-doc0-9-1.pdf

The other PHP library uses underscores as well: https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Message/AMQPMessage.php#L28 so if by mistake your message is created using content-type then it won't be encoded AFAIK, and maybe that's the problem @macintoshplus is referring to

@Seldaek
Copy link
Owner

Seldaek commented Mar 10, 2016

OK thanks. It seems AMQPExchange from the amqp extension uses Content-type http://php.net/manual/sr/amqpexchange.publish.php

Depends on the docs language tohugh, http://php.net/manual/pl/amqpexchange.publish.php shows content_type.. So maybe it changed? @macintoshplus could you try what happens if we send both? Would that be a good way to work with all versions? Ping @lstrojny any help here would be welcome, if we can check for the extension version or something that'd be the best I guess.

@macintoshplus
Copy link
Contributor Author

Hi,

RabbitMQ ignore all unknown properties. I don't know other AMQP implementation.

Send twice key for the content-type, should be a good idea.

@videlalvaro
Copy link

Why send it twice and not send the proper one, assuming RabbitMQ is the
target broker?

On Wed, Mar 9, 2016 at 10:14 PM, Jean-Baptiste Nahan <
notifications@github.com> wrote:

Hi,

RabbitMQ ignore all unknown properties. I don't know other AMQP
implementation.

Send twice key for the content-type, should be a good idea.


Reply to this email directly or view it on GitHub
#740 (comment).

@Seldaek
Copy link
Owner

Seldaek commented Mar 10, 2016

My point was that I don't think the code is wrong, but just outdated. If the php AMQP extension indeed changed this then depending on the installed version we should send A or B. That's why I want to hear from the maintainers there before taking action either way.

@lstrojny
Copy link
Contributor

@Seldaek just go with content_type, Content-type, according to the history, never worked.

@Seldaek
Copy link
Owner

Seldaek commented Mar 10, 2016

OK thanks :)

Seldaek added a commit that referenced this pull request Mar 10, 2016
@Seldaek Seldaek merged commit 0c45c9b into Seldaek:master Mar 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants