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 #53 IronMqProvider return message's body with name of queue #54

Merged
merged 2 commits into from
Dec 2, 2014

Conversation

pasxel
Copy link

@pasxel pasxel commented Nov 27, 2014

No description provided.

@k-k
Copy link
Contributor

k-k commented Nov 29, 2014

@pasxel - Thanks for the contribution. Yeah, I can't remember or see a reason why Im adding the queue name to the message for IronMQ.

A better fix, if you dont mind updating this PR is to modify the publish method to remove the queue name there. Let me know, I don't mind fixing it as well.

@scrutinizer-notifier
Copy link

The inspection completed: 1 new issues

k-k added a commit that referenced this pull request Dec 2, 2014
Fix #53 IronMqProvider return message's body with name of queue
@k-k k-k merged commit a176586 into uecode:master Dec 2, 2014
@k-k
Copy link
Contributor

k-k commented Dec 2, 2014

@pasxel - Thanks!

@cristiangraz
Copy link

@kmfk On line 90 of handleIronMqNotifications, it's attempting to call key() against the request body and set it as the queue name. Without the queue name, it doesn't seem the events are actually triggering. The queue name is used to trigger notifications on line 103 when it calls Events::Notification. The only way I've been able to get my events to trigger is to add '' => '' to my message body when I call the publish method.

It looks like the AWS messages have a Subject in the metadata that the queue name. It seems the queue name is needed to trigger the correct notifications (it's currently broken for me). I agree that the queue name shouldn't be in the message body... what do you think about adding an X-Iron-Queue-Name header in the create() method of the IronMqProvider? That way all requests will have the queue name. I'm happy to submit a pull request, however I'm not sure what the BC issues will be. Unless I'm missing something, IronMQ queues seem to be broken since the name was removed.

@cristiangraz
Copy link

@kmfk Any thoughts on this?

After thinking about this some more, we could retain backwards compatibility by adding the queue name to the body of the message (like you originally had, however maybe with a name like __queueName), then removing it from the body before dispatching any events. It would keep it invisible from the end user so there would be no additional data fields in their message, but wouldn't require changing IronMQ settings (to send headers) like I originally suggested.

@k-k
Copy link
Contributor

k-k commented Jan 23, 2015

@cristiangraz - sorry for the delay, have been swamped at work. Yeah, as soon as you replied I realized the mistake here. I think the difference is that @pasxel was not using the push queue, but manually polling messages. The request handler does actually remove the queue name from the message.

I'm going to revert this in a new PR and add a fix for @pasxel's issue separately in the receive method - as, after reviewing more thoroughly, that should have been the original fix.

@k-k k-k mentioned this pull request Jan 23, 2015
k-k pushed a commit that referenced this pull request Jan 23, 2015
@k-k k-k mentioned this pull request Jan 23, 2015
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