laravel-api-response-builder icon indicating copy to clipboard operation
laravel-api-response-builder copied to clipboard

Cannot configure a converter key as NULL

Open sitawit opened this issue 3 years ago • 5 comments

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

  • Name and version of OS: Windows 10
  • PHP version: 7.4
  • Laravel version: 8
  • ResponseBuilder version used: 9.2.3

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.

sitawit avatar Jun 17 '21 09:06 sitawit

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.

MarcinOrlowski avatar Jun 17 '21 11:06 MarcinOrlowski

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.

sitawit avatar Jun 18 '21 07:06 sitawit

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 avatar Jun 18 '21 12:06 MarcinOrlowski

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

MarcinOrlowski avatar Jun 18 '21 12:06 MarcinOrlowski

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,
],

sitawit avatar Jun 18 '21 13:06 sitawit