Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defensive code required in LineItem #910

Open
OS4 opened this issue Jan 18, 2024 · 4 comments
Open

Defensive code required in LineItem #910

OS4 opened this issue Jan 18, 2024 · 4 comments

Comments

@OS4
Copy link

OS4 commented Jan 18, 2024

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
Copy link
Owner

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
Copy link
Author

OS4 commented Jan 19, 2024

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
Copy link
Owner

calcinai commented Jan 22, 2024

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants