calcinai / xero-php

A php library for the Xero API, with a cleaner OAuth interface and ORM-like abstraction.
MIT License
359 stars 262 forks source link

Defensive code required in LineItem #910

Open OS4 opened 8 months ago

OS4 commented 8 months ago

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

calcinai commented 8 months ago

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.

OS4 commented 8 months ago

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?

calcinai commented 8 months ago

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.

Healyhatman commented 5 months ago

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.