AmazonOrderItemList returns non-matching AmazonOrderItem for AmazonOrder

[HMAZonderland]

I've been debugging this application further. If you take a closer look to the else if statement you'll see it will add a Log entry when the AmazonOrderId's do not match. But it allows the code to continue. The code will continue to parse and return the Items for the wrong Order object. Perhaps you should add a return false; there:

if (is_null($xml->AmazonOrderId)){
    $this->log("You just got throttled.",'Warning');
    return false;
} else if (isset($this->options['AmazonOrderId']) && $this->options['AmazonOrderId'] && $this->options['AmazonOrderId'] != $xml->AmazonOrderId){
    $this->log('You grabbed the wrong Order\'s items! - '.$this->options['AmazonOrderId'].' =/= '.$xml->AmazonOrderId,'Urgent');
    return false;
}

/**
 * Retrieves the items from Amazon.
 * 
 * Submits a <i>ListOrderItems</i> request to Amazon. In order to do this,
 * an Amazon order ID is required. Amazon will send
 * the data back as a response, which can be retrieved using <i>getItems</i>.
 * Other methods are available for fetching specific values from the order.
 * This operation can potentially involve tokens.
 * @param boolean <p>When set to <b>FALSE</b>, the function will not recurse, defaults to <b>TRUE</b></p>
 * @return boolean <b>FALSE</b> if something goes wrong
 */
public function fetchItems($r = true){
    $this->prepareToken();

    $url = $this->urlbase.$this->urlbranch;

    $query = $this->genQuery();

    $path = $this->options['Action'].'Result';
    if ($this->mockMode){
       $xml = $this->fetchMockFile()->$path;
    } else {
        $response = $this->sendRequest($url, array('Post'=>$query));

        if (!$this->checkResponse($response)){
            return false;
        }

        $xml = simplexml_load_string($response['body'])->$path;
    }

    if (is_null($xml->AmazonOrderId)){
        $this->log("You just got throttled.",'Warning');
        return false;
    } else if (isset($this->options['AmazonOrderId']) && $this->options['AmazonOrderId'] && $this->options['AmazonOrderId'] != $xml->AmazonOrderId){
        $this->log('You grabbed the wrong Order\'s items! - '.$this->options['AmazonOrderId'].' =/= '.$xml->AmazonOrderId,'Urgent');
    }

    $this->parseXML($xml->OrderItems);

    $this->checkToken($xml);

    if ($this->tokenFlag && $this->tokenUseFlag && $r === true){
        while ($this->tokenFlag){
            $this->log("Recursively fetching more items");
            $this->fetchItems(false);
        }
    }
}

[HMAZonderland]

Ive created a Pullrequest for this change

[Peardian]

I seem to recall that the lack of a return is intentional. I think the idea was that it will still load whatever items Amazon tells you is in the shipment, but it will alert you to the mismatch. Then, you can compare the mismatch items to what should be in the order, or to your other orders.

[HMAZonderland]

It does cause behaviour thats quite unwanted. Returning OrderItems for Orders that do not belong there. I've used the Mock mode to parse 2 sample Xml files. Each of the Orders (3) in the xml files received the same OrderItems, even when they do belong there. I'm not sure why you would want this?

[Peardian]

The chances of it happening outside of mock mode are almost none. I figured that, should this rare bug ever actually happen, it would be better to have the data for analysis.

[HMAZonderland]

I've been trying to figure out how I can be sure the OrderItem belongs to the Order. How to check?

[Peardian]

Now that I look at it again, it seems there isn't one. That would be a useful thing to have, but I can't do anything about it at this time.

[HMAZonderland]

What would your prefer? Having a build in check or return false? Since there is no way to tell by the OrderItem to what Order it belongs.

[Peardian]

I think the best thing to have would be for the OrderItemList to store which order the items belong to as indicated by the parsed XML, and a way to get this order number. That way checks can be performed at any time, even after the data has been loaded.

[HMAZonderland]

Created a PR: #48

[Peardian]

Taken care of