MarcinOrlowski / laravel-api-response-builder

Builds nice, normalized and easy to consume REST JSON responses for Laravel powered APIs.
MIT License
720 stars 79 forks source link

Cannot configure a converter key as NULL #201

Closed sitawit closed 2 years ago

sitawit commented 3 years ago

What's wrong? MarcinOrlowski\ResponseBuilder\Exceptions\IncompatibleTypeException Incompatible types. Cannot merge NULL into string (key 'key'). marcin-orlowski\laravel-api-response-builder\src\Util.php:46

Steps to reproduce the behavior:

  1. Install new Laravel with "composer create-project laravel/laravel example-app"
  2. Install this package with "composer require marcin-orlowski/laravel-api-response-builder"
  3. Publish a configuration with "php artisan vendor:publish"
  4. Edit a "collection converter" to use "key" as NULL
  5. Create any sample API to return a convert with method "RB::success($items)"

Expected behavior Return a successful response without problem and without key

Runtime environment

Notes The problem lines in MarcinOrlowski\ResponseBuilder\Util::mergeConfig(). The original configuration has a string type, where the actual environment wants NULL type. That is whey it will conflicts, the NULL for "key" configuration should be possible according to the document. I'm not personally sure how this should be handle but I am current do a quick work around by override a class myself without checking the type when merging.

MarcinOrlowski commented 3 years ago

RB::success($items)

If $items is a collection then you cannot have a null key because that would violate requirement for the returned payload to always be an JSON object. And then all contents of object must have a key.

Incompatible types. Cannot merge NULL into string (key 'key'). marcin-orlowski\laravel-api-response-builder\src\Util.php:46

If the above is the case, then the only issue I see is that you should get more clear error message here.

sitawit commented 3 years ago

I see. https://github.com/MarcinOrlowski/laravel-api-response-builder/blob/master/docs/config.md As I read the document again, it is clear but it is not so clear that we definitely need key for a collection and we can switch between having key and not having key for a single object. Because I think those explanation is just a recommendation not mandatory.

If that is the case, the error should be more clear but I think adjust the document should be more easier.

However, I was expected that we can choose between having key and not having key regardless of the object type (just a single object or a collection) but that is up to you.

In conclusion, this issue is not a bug. I have my answer now. You can close the ticket as you see fit. Thanks for the reply.

MarcinOrlowski commented 3 years ago

I updated the documentation, so hopefully it is now a bit more clear when you can use NULL as key. I also rephrased exception message for the same reason. Thanks for the feedback.

MarcinOrlowski commented 3 years ago

BTW: I'd like to test one more thing - could you please post your config that caused you this behavior (with key set to NULL etc)? Exactly as you had when you were reporting this. Thanks

sitawit commented 3 years ago

In a response_builder.php config file, I changed the class converter from default to this one. This is the only adjustment I did, specifically to just a collection.

\Illuminate\Support\Collection::class               => [
    'handler' => \MarcinOrlowski\ResponseBuilder\Converters\ToArrayConverter::class,
    'key'     => null,
    'pri'     => 0,
],