Success response with an empty array

[AdamGaskins]

I dropped this library into one of my projects, but some of the tests started failing because the default value of [] is converted to [ 'success' => true ]. This makes sense if you just call return $this->respondWithSuccess() with no arguments, but in my case I'm returning an array of some models and when there aren't any in the database I actually want the response to be an empty array.

I ended up using my own method with null as the default:

    public function respondWithSuccess($contents = null): JsonResponse
    {
        $contents = $this->morphToArray($contents ?? [ 'success' => true ]);

        return $this->apiResponse($data);
    }

Didn't want to just create a PR though since I know this would be a breaking change.

[kzvonov]

@ultrono hey, I can also take thi one) If you don't mind.

[ultrono]

@AdamGaskins Can you provide some example code so I can take a look?

@kzvonov Sure 👍

[AdamGaskins]

The code in my OP basically sums up the change (using null instead of [] as the default). respondNoContent and respondCreated are the other two methods that I propose changing that have [] as the default.

[ultrono]

Let me put a pin in this one for the moment, until the end of the week. Will have a proper look then. Have a lot of on in the next few days. In the meantime I'd accept a PR if we can make this a none breaking change 👍

[JacekAndrzejewski]

This change conflicts with #15.

[AdamGaskins]

Yeah, the reason I didn't create a PR was because by definition this is a breaking change, and a direction needs to be decided for the library.

Here's what I'm proposing in this issue, which makes the most sense to me:

// No argument returns the default response

// Should respond: { "success": true }
return $this->respondWithSuccess();


// Whatever you pass as an argument becomes the response

// Should respond: { {"id": 1}, {"id": 2} }
return $this->respondWithSuccess([ ['id' => 1], ['id' => 2] ]);
// Should respond: []
return $this->respondWithSuccess([]);

I don't think the current functionality makes sense:

// Currently responds: { "success": true }
return $this->respondWithSuccess();


// Currently responds: { "success": true } ??
return $this->respondWithSuccess([]);

And I personally don't think that #15 should be merged into the library either. If you do want 'success' => true to be appended to your custom responses for whatever reason, you should be using JsonResponses anyway, or you could override the method in your base controller, both of which are a super easy add in your own project!

[ultrono]

@AdamGaskins Was just leaning to merging #15 but you make a good point.

This library was initially something to scratch my own itch, were the project I was working on needed some sanity. There are a lot of use cases recent PR's demonstrate and I'd like to keep the package as simple as possible.

As we now have functionlaity to handle a variety of object types (#9) returning an empty array if 💯 percent something required. At the moment as mentioned this isn't possible, as I've essentially made a decision for consumers of this package.

At the moment I'm leaning to setting the default to null as suggested.

Alternatively, could some form of confitional be added at https://github.com/f9webltd/laravel-api-response-helpers/blob/master/src/ApiResponseHelpers.php#L40-L42 that specifically handles null? This could avoid a frull breaking change.

Open to options / opinions 👍

[ultrono]

Sorry, closed this one in error!

[twdnhfr]

as @JacekAndrzejewski stated in here:
#15 (comment)

wouldn't it be a solution to make this a config thing and be able to set some kind of $defaultData?

[AdamGaskins]

@twdnhfr I think there are two different things--

1. Being able to configure what the default response is ({ "success": true }). I would support this, but it's up to ultrono how much complexity he wants to add to the library.
2. Merging the default response into every response. If this were added, I personally think it should be configurable and turned off by default. Adding "success": true into every response doesn't seem like the 90% use case to me. Also again, depends how much complexity ultrono wants to add to the library. It would be pretty easy to make a parent JsonResponse class to add this to all your responses without needing a library to do it.

Just my 2 cents! Appreciate the discussion and thoughts.

[ultrono]

So, after some thought via the discussion, here is my thinking (it encompasses a few issues and PRs).

• Allow functionality to set the default response via a method within the package trait (i.e. $this->setDefaultApIResponseData()). I'll make that method chainable so you can optionally call it inline i.e. $this->setDefaultApIResponseData(['foo' => 'bar'])->respondWithSuccess(...) This would also be merged into the default response
• handle null, allowing an empty array to be returned

Going to give this a go tomorrow or the day after, this week has been insanely busy 😪

[JacekAndrzejewski]

* Allow functionality to set the default response via a method within the package trait (i.e. `$this->setDefaultApIResponseData()`).

@ultrono I think a better name would be something like setDefaultSuccessResponse(). I already gave it a shot, but I didn't like my implementation so I didn't submit it. I used a class variable, but chainable method seems to be a better solution.

[AdamGaskins]

Maybe I'm not understanding how you intend $this->setDefaultApiResponseData() to work, but wouldn't you have to call that function in each method of your controller with the default data?

public function store(PostStoreRequest $request)
{
    // ...
	return $this->setDefaultApiResponseData([ 'message' => 'Success' ])
                ->respondWithSuccess();
}

public function delete(Post $post)
{
    // ...
	return $this->setDefaultApiResponseData([ 'message' => 'Success' ])
                ->respondWithSuccess();
}

That seems worse to me than just passing it directly into the respondWithSuccess() method.

[JacekAndrzejewski]

Maybe I'm not understanding how you intend $this->setDefaultApiResponseData() to work, but wouldn't you have to call that function in each method of your controller with the default data?

No, you wouldn't need to do that, because default response would be in some variable in trait which would have a default value. So that default would be returned if you didn't call this method.
And I guess you could make this method update variable bound to the class, so you could do it only once in constructor and then call it in places where you need to change it.

[JacekAndrzejewski]

@ultrono Did you give it a go and wrote anything? If not then I can work on this today, I need to work in non legacy project for a while or I'll go crazy

[ultrono]

My plan was:

• fetch the default response data from a class variable, meaning you can still call $this->respondWithSuccess()
• the setDefaultApiResponseData could be called in the class constructor or a parent class class, or directly i.e. $this->setDefaultApiResponseData([])->respondWithSuccess()
• Set the default success response to ['success' => true]as to avoid a breaking version change

I'm really busy at the momnent and haven't had time to work on this. If @JacekAndrzejewski wants to pick this up I'd be happy to merge 👍 I'd imagine a couple of tests would be required too.

[kzvonov]

@ultrono I can pick this one if everybody is okay about it)

[ultrono]

I believe this issue can be closed as a good PR from @JacekAndrzejewski has just been released in v1.5.0 🚀