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

Eloquent collections and models #42

Merged
merged 3 commits into from
Feb 10, 2020
Merged

Eloquent collections and models #42

merged 3 commits into from
Feb 10, 2020

Conversation

LarsGrevelink
Copy link
Contributor

Hi @usmanhalalit!

I might have overdone it a little bit so please correct me since I do not know the reason some stuff was put in.

The real reason for this pull request was the call to $model->toArray() which can sometimes have unwanted side effects. Developers often know the best way to eager/lazy load their data and when triggering the toArray function appends and visible fields are triggered while they may not be used in exporting at all. Retrieving attributes from models/arrays directly actually allows you to pull them one by one, visible or not.

Which led me to remove the if to check whether an Eloquent collection is at play which let me remove the $isEloquentCollection var as well. It might be that Arr::get evolved over the years or that I am missing some cases so please let me know if I do.

The same goes for collecting the model, is that because of expecting potential objects? In that case it would make sense but since the tests did not fail I removed it for now. Let me know when I need to re-add it. No issue at all but can't we use an (array) casting to enforce this?

Love to hear your feedback.

Cheers mate - we love the plugin,
L

…JSON serialised output,

Collecting a model is not needed when using Arr::get,
Triggering the `toArray` function has unforeseen side effects like loading relations/values for not used properties in the export
@LarsGrevelink
Copy link
Contributor Author

LarsGrevelink commented Feb 8, 2020

Ran the tests also against illuminate/database:5.2.0 directly which seems to pass as well and make sure the earliest version should still be compatible.

collect($model) did actually add the support for generic classes to be managed here as well but introduced the same performance issue due to the Collection constructor calling toArray in case of Arrayable instances.

I have added additional tests to make sure this stays supported and added the Arr::accessible check to prevent unnecessary collecting and leave instances like Eloquent models untouched. If you'd rather want to use type casting to convert objects to arrays, let me know.

Cheers,
L

@usmanhalalit usmanhalalit merged commit f15ae41 into usmanhalalit:master Feb 10, 2020
@LarsGrevelink
Copy link
Contributor Author

Thanks @usmanhalalit for the fast action on this pull request. Appreciated.

@LarsGrevelink LarsGrevelink deleted the feature/battling-side-effects branch February 10, 2020 12:22
@usmanhalalit
Copy link
Owner

That was a thoughtful improvement, definitely needed out of the box thinking, appreciate you giving back @LarsGrevelink .

This was referenced Feb 15, 2020
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.

None yet

2 participants