iron-io / iron_mq_php

PHP client for IronMQ.
www.iron.io
BSD 2-Clause "Simplified" License
89 stars 43 forks source link

postMessage should return object with single "id" instead of array of "ids" #10

Closed dxjones closed 11 years ago

dxjones commented 11 years ago

The following PHP code posts a single message to a Queue.

$mesg = 'Hello World!';
$result = $ironmq->postMessage('demo_queue', $mesg);

The id of the message is buried inside an array, since postMessage is simply a wrapper for postMessages.

echo 'id = ' . $result->ids[0] . "\n";

It would be more useful, convenient, and logical to extract the single id from the array inside postMessage, so we could access it thusly:

echo 'message id = ' . $result->id . "\n";

This would be consistent with the workings of getMessage, which also returns a result with a single "id" field.

thousandsofthem commented 11 years ago

@dxjones updated. Now postMessage returning id, postMessages - array of ids. Consistency with getMessage is not that important to introduce some additional complexity

treeder commented 11 years ago

@thousandsofthem maybe return both so as not to break backwards compatability? This is a breaking change.

thousandsofthem commented 11 years ago

Unfortunately, both formats at same time will not work. @treeder what do you like more:

(single message) old

stdClass Object
(
    [ids] => Array
        (
            [0] => "5827713494696071032"
        )

    [msg] => Messages put on queue.
)

vs new (without backward compatibility)

"5827713494696071032"

and

(multiple messages) old

stdClass Object
(
    [ids] => Array
        (
            [0] => "5827713494696071032",
            [1] => "5827713494696071033"
        )

    [msg] => Messages put on queue.
)

vs new (without backward compatibility)

Array( "5827713494696071032",  "5827713494696071033")
dxjones commented 11 years ago

I thought @treeder was suggesting returning an object containing a new "id" field, in addition to the current behaviour.

This change would not break existing code, but it allows you to access $result->id as well as $result->msg.

The intent of my original suggestion was to make postMessage more similar to getMessage, which deal with single messages and return an object with a single "id" field.

treeder commented 11 years ago

Ya, that's what I was suggesting. If there's a single message, return old with additional id method/field.

ie:

$result->ids[0] == $result->id
thousandsofthem commented 11 years ago

Well, this change will not break postMessage. But what about postMessages ? Both methods should produce something similar to each other. array of objects with id field in each.... feels weird and not compatible with old format

$result->ids[0]
->
$result[0]->id
treeder commented 11 years ago

Does postMessages currently return $result->ids[0] ?

thousandsofthem commented 11 years ago

yes, direct translation of api response $result->ids[0] for postMessage and $result->ids[0], $result->ids[1] etc for postMessages

P.S. Things changed in last commit - now postMessage returning string (id), postMessages - array of strings(id)

dxjones commented 11 years ago

arrgh! Doesn't this change remove access to the 'msg' that was returned by the API?

My initial suggestion was simply to include an extra field "id", it would have required adding a single line:

id = ids[0]

This wouldn't break any existing code, ...

sigh, I think this change is a step backwards.

treeder commented 11 years ago

+1 on what @dxjones said.

@thousandsofthem at the very least we should roll back that last commit immediately, it's backwards incompatible and that just can't happen unless you do a "major" version bump (you only bumped the "patch" version) and I don't think this is worth doing a major version bump over. So it should either stay the way it was or do something like @dxjones said.

Would something like this work? (I don't know php)

$decoded = self::json_decode($res);
$decoded.id = $decoded.ids[0]
return $decoded;
thousandsofthem commented 11 years ago

Ah, you're right. Completely reverted changes in postMessages and partially in postMessage (as you suggested)

thousandsofthem commented 11 years ago

Closing the issue. no breaking changes made, added "id" for postMessage