xero-php icon indicating copy to clipboard operation
xero-php copied to clipboard

Defensive code required in LineItem

Open OS4 opened this issue 1 year ago • 4 comments

vendor/calcinai/xero-php/src/XeroPHP/Models/Accounting/LineItem.php line 253 is currently

    public function getItemCode()
    {
        return $this->_data['ItemCode'];
    }

I believe that something like

    public function getItemCode()
    {
        if (isset($this->_data) && isset($this->_data['ItemCode'])) {
            return $this->_data['ItemCode'];
        } else {
            return "";
        }
    }

Is required to stop errors being thrown

OS4 avatar Jan 18 '24 13:01 OS4

Hi @OS4. Typically in this situation you'd be using isset() if you weren't certain that it would be set–returning an empty string might be a little dangerous to tell the difference.

calcinai avatar Jan 18 '24 20:01 calcinai

Yes, that's true. Typically in these situations we would return "-unknown-", makes it stand out. Returning null may be a better solution, but I wasn't sure where in the library this function was called, and as it was probably going to return a string an empty one seemed best. I don't think it's used anywhere in your library code. We call it directly to get the item codes to push to Xero. The issue arises with postage, which doesn't use a code, which was causing the error to be reported. For us an empty string was suitable :-) , sorry. You are better placed to determine what defensive code is required and what the correct return value should be. Is it correct to call this function directly, or should we be going via a better API call?

OS4 avatar Jan 19 '24 10:01 OS4

Hi @OS4, a decision was made early on to let the models reflect the objects returned from Xero as closely as possible. As Xero accepts (and sometimes sends) incomplete objects, they are stored this way client-side.

In your code, you can simply test isset($lineItem->ItemCode) before trying to access it.

calcinai avatar Jan 22 '24 19:01 calcinai

Secondary explanation: Some Xero models can return null for a property, some return nothing at all.

If you save a Xero model with a property set to null, it could then set the property to null or otherwise delete it, whereas not sending the property at all has a different (or no) effect.

Healyhatman avatar May 01 '24 01:05 Healyhatman