flugg / laravel-responder

A Laravel Fractal package for building API responses, giving you the power of Fractal with Laravel's elegancy.
MIT License
865 stars 86 forks source link

Fixed the problem with the only parameter functionality #119

Closed tkayfun closed 6 years ago

tkayfun commented 6 years ago

See issue https://github.com/flugger/laravel-responder/issues/118

flugg commented 6 years ago

Thank you for explaining the issue in such detail in https://github.com/flugger/laravel-responder/issues/118.

I've been tinkering a bit and there is indeed a bug as it should handle both arrays and strings. However, your solution creates new problems, which one of the unit tests picked up on.

We need to merge the fields with already parsed fields for the same entity, but it should also be explicitly casted to an array if it's a string. I believe this should work fine:

$fieldsets[$key] = array_unique(array_merge(key_exists($key, $fieldsets) ? (array) $fieldsets[$key] : [], $fields));
tkayfun commented 6 years ago

Thanks for the quick reply, and sorry for my late response! I will look into it tomorrow, and edit my PR after I've tested it with your code. Keep you updated!

tkayfun commented 6 years ago

I had this code already(without the key exist functionality, your piece of code is actually better by checking if the key already exists!), I casted it in my project to quickly work with it because I needed it immediately but forgot to update it in the laravel-responder package folder (because I was experimenting)! 🤦‍♂️ Now I casted the field explicitly to an array, and by doing the short-handed if/else (instead of the coalescing operator (??)) block it is fixed.

Thanks for the reminder, it works like a charm! Hope to see it updated soon, so I can update the package 👍 :)

Besides this issue, good work on this nice package! Keep up the good work 💯 @flugger

flugg commented 6 years ago

Thanks! I’ll tag a release soon :)