Should AmazonMerchantShipment::parseXML really set ShipmentId like this?

[mehgcap]

Hello,
I've found what might be a bug, or might just be something I am not fully understanding. I'm working with the AmazonMerchantShipment and AmazonMerchantShipmentCreator classes, trying to get a ship response (including label) from Amazon. I can set everything in the Creator instance, but when I try to call AmazonMerchantShipmentCreator::getShipment(), an error is logged that "Shipment ID must be set in order to fetch it!"

After some poking around the two classes, here's my tentative bug report.

When AmazonMerchantShipmentCreator::getShipment() is called, it gets an XML response which it passes to the class's parseXML function. In that function, the constructor of AmazonMerchantShipment gets the XML as AmazonMerchantShipmentCreator makes a new AmazonMerchantShipment instance. In that constructor, AmazonMerchantShipment calls its own parseXML function, which assigns things from the given XML to the class's $data array. This includes setting $data["ShipmentId"] to the shipment ID in the xml. In other words, when AmazonMerchantShipmentCreator::getShipment() is called, it basically gives XML to AmazonMerchantShipment's constructor, which sets AmazonMerchantShipment::data to the values it needs from that XML. Among these values is ShipmentId.

However, in AmazonMerchantShipment::fetchShipment(), a check is performed to see whether AmazonMerchantShipment::options has a key called ShipmentId. Finally we arrive at the bug: AmazonMerchantShipment::parseXML only sets the ShipmentId in the data array, not the options array. Thus, the check for that ID in the options array always fails, even though the XML that made the instance had the ID set.

Put simply, when given valid XML from AmazonMerchantShipmentCreator, AmazonMerchantShipment sets its data array with ShipmentID, but never sets its options array with that same ID. Then, when it tries to use the ID, it checks in its options array--the array that never got the ID placed in it. It seems like AmazonMerchantShipment::parseXML should set both arrays with a value for ShipmentID to fix this problem. That, or every check should inspect both options and data.

Again, I may be mistaken here. I'm not an expert, and I haven't gone through the entire library. Still, when I call AmazonMerchantShipment::fetchShipment() with the stock code, I get the warning that the shipment ID isn't set. If I add my fix, the warning goes away. Of course, I don't have a label yet, so I might be completely off base here. :) Still, I thought I would bring this up in case it really is a bug. Thanks for reading.

[Peardian]

Thank you for reporting this. The description of the code seems to be a tad inaccurate, but the bug you found is real. I probably neglected to set the shipment ID option when filling the shipment object because I couldn't (and still can't) think of any use cases in which this would be needed. Regardless, the option could still be set for the sake of convenience.

The getShipment() method (like all get methods in this library) only retrieves data that has already been stored to the object by a previous API request, which in this case would be createShipment(). Upon creating the shipment, Amazon provides full information for the shipment, which this library then stores in a shipment object. The only two available actions from that point are to fetch the data again (the same data already loaded) or to cancel the shipment which was just created, which is why having the shipment ID option isn't very useful.

Hopefully that description helps you with using the class, and thanks again.

[mehgcap]

I realized after I wrote it that my code description was a bit off, but I'm glad you understood what I was going for. :)

The way my application flow is right now, I do the following:

• make an instance of AmazonMerchantShipmentCreator, call it $creator
• set $creator's properties--item list, weight, etc
• get a list of the shipping options available to $creator, then save the ID of the cheapest one
• make an instance of AmazonMerchantShipment, call it $shipment
• call $shipment->setService($cheapestService["ShippingServiceId"]) to tell the shipment to use the cheapest option we found
• $shipResponse = $creator->getShipment();
• $shipResponse->fetchShipment();
• $details = $shipResponse->getData();

Now, $details is an array of all the necessary information I'll need for the rest of the application. It has the label, dates, rate, service name, and so on. I have no idea if this is the best way to do what I'm trying to do, but it ships packages and prints labels. I should say that this is with my fix in place in the php-amazon-mws library.

[Peardian]

Most of the flow is correct, but calling fetchShipment() is redundant. Try dumping $shipResponse->getData(); prior to calling fetchShipment() and you should see that it already contains all of the data you need. Cutting out the extra request will speed things up and save you from extra throttling.

[mehgcap]

Thank you for the suggestion. I'll remove that extra call, and skip straight to the getData() call instead.

Given my workflow, would I still be able to use the library as-is, without modifying it to set the ShipmentId value in $options? I'm just wondering what you had in mind that made such a fix unnecessary, compared to what I've done.

[Peardian]

Yes, you can still use the library as-is. The shipment ID option is only needed when you are fetching the data separately from the shipment creation process.