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

Initialise all properties when a new model is constructed #826

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

glennschmidt
Copy link

Set collection properties to an empty collection, other properties to null.
Prevents an ‘Undefined index’ notice when calling getters.

I've addressed this via the constructor of the base model class, so it doesn't matter if some of the getters already check for isset and others don't. Let me know if you think this approach will do the job, or if it will create other issues.

Resolves #652

Set collection properties to an empty collection, other properties to null.
Prevents an ‘Undefined index’ notice when calling getters.

Resolves calcinai#652
@Healyhatman
Copy link
Contributor

Healyhatman commented Sep 28, 2021

The problem is when you send the data to Xero, for some endpoints if a property is missing it ignores it, but if a property is NULL, it might delete it or otherwise perform an unwanted update. For example if all the properties are initialised to null and you update an Employee, any attributes not specified will be deleted. I wonder if instead one could use the __isset magic method?

@glennschmidt
Copy link
Author

I thought that's why we have the $_dirty list. Doesn't it only send properties that have been explictly set (marked as dirty)?

@calcinai
Copy link
Owner

calcinai commented Oct 4, 2021

@glennschmidt exactly. I think this should be fine. My only thought is about consistency–this leaves specific handling/initialisation in some models, and also leaves the uninitialised indices for non-collection properties.

Thoughts on that?

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

Successfully merging this pull request may close these issues.

Getters throw exception if property is empty
3 participants