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

Lazyload #80

Merged
merged 7 commits into from Oct 24, 2016
Merged

Lazyload #80

merged 7 commits into from Oct 24, 2016

Conversation

kvij
Copy link
Contributor

@kvij kvij commented Oct 18, 2016

Added lazyloading feature. Now $account->BankAccounts will return an array of BankAccount objects. Unfortunatly the Exact API is a bit inconsistent. When setting an array of BankAccounts to a new Account and than save results in an error (Account guid not provided O_o) while a new Subscription cant be saved without a vallid collection of SubscriptionLines.

The feature is backward compatible with manually building the collections (nested arrays). Setting a single object to a collection attribute is not supported.

I have also added an PrimaryKey accessor to help with storing in a cache table and duplicate records.

@stephangroen stephangroen merged commit a403616 into picqer:master Oct 24, 2016
@stephangroen
Copy link
Member

Might be moving to new major version for this, because the result of $account->BankAccounts now returns an object instead of an array with the previous values.

It could theoretically be possible for someone to depend on the old result and use $account->BankAccounts['__deferred']['url'] to fetch something themselves.

This is a great addition though, very nice!

@kvij
Copy link
Contributor Author

kvij commented Oct 24, 2016

I do not think this is likely (much easier to do $bankAccount->filter("Account eq quid'...'") messing with nextUrl was not feasible either (infinite loop bug on 1 and 61 results) but it is indeed a possibility and you are entirely correct. On usecase I can come up with is when the uri is stored somewhere for later use.

Perhaps we can also remove the plural named classes when we move to a new major version.
Thank you for handling all all the merges I know I dumped a lot of code in a short time on you :-)

@stephangroen
Copy link
Member

I don't think it's likely either, but I'd like to keep to semver and make sure people can trust on no breaking changes at all.

We can indeed also remove the plural named classes and note that as a breaking change, together with the lazy loading capabilities. Cleans up the code :)

No problem, I'm glad you took the time and submitted these improvements. I had to find sometime myself to do a good review. They're great!

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

Successfully merging this pull request may close these issues.

None yet

2 participants