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

Fix issue where non set optional values were transformed #602

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

rust17
Copy link
Contributor

@rust17 rust17 commented Nov 8, 2023

fix: #583 and be compatible with backward which mentioned in #590

@rust17
Copy link
Contributor Author

rust17 commented Nov 23, 2023

@rubenvanassche 😃 I think it's helpful. If you think this change useful too, please consider merging this.

@rubenvanassche
Copy link
Member

Good solution, never would have though about that one. Hopefully it will not break performance too hard. Thanks

@rubenvanassche rubenvanassche merged commit f61000f into spatie:main Dec 1, 2023
10 of 11 checks passed
@xHeaven
Copy link
Contributor

xHeaven commented Dec 2, 2023

@rubenvanassche Probably just a nitpick, also didn't test it at all so I'm just talking out of my head right now: do we really want to do get_object_vars() inside the foreach loop? Can't we do $objectVars = get_object_vars($data); or something like this outside the loop, and referencing that variable inside the loop? I think it'd help a little if you have massive datasets if we didn't query all the object vars in every iteration from the exact same dataset.

@rust17
Copy link
Contributor Author

rust17 commented Dec 3, 2023

@xHeaven Yes, that sounds more reasonable, moving the get_object_vars() outside of the foreach could potentially improve performance. A pr is on the road.

@rubenvanassche
Copy link
Member

rubenvanassche commented Dec 4, 2023

That's indeed a good point, did 60 PR's and issues on Friday. Never thought about that 😅

@rubenvanassche
Copy link
Member

The only thing is, we now only checking this when a property is optional. If you're not having any Optional properties the call won't be made. Which is maybe an even bigger performance gain? All depends on how many Optional properties you're having. Difficult case ... let's go with the call outside the loop for now though.

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.

Non-self explanatory Optional property initialization
3 participants