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

Add __toString on ApiResponse #11

Open
dextermb opened this issue Aug 24, 2018 · 8 comments
Open

Add __toString on ApiResponse #11

dextermb opened this issue Aug 24, 2018 · 8 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@dextermb
Copy link
Contributor

I think that we should add __toString() to ApiResponse as when returning from a controller it attempts to convert the returned object to a string, if we were to implement this it would save us from adding ->json() on the end of every API return.

Proposed code

class ApiResponse
{
    // ...

    public function __toString()
    {
        return $this->json()
    }

Thoughts? We could also implement ArrayAccess for even more accessibility.

cc @DivineOmega @JacobBrassington @wheatleyjj

@dextermb dextermb added the enhancement New feature or request label Aug 24, 2018
@DivineOmega
Copy link
Contributor

+1 on implementing __toString() in this manner.

I'm curious about your desired implementation of the ArrayAccess interface. Maybe worth making a new issue for this?

@dextermb
Copy link
Contributor Author

@DivineOmega Nothing crazy, would just allow people to push into data. Maybe something like the example below

$initial_data = ['message' => 'hello, world'];
$response = ApiResponse::success() # or new ApiResponse or ApiResponse::error()

$response->data($initial_data);

// Edit array elements
if(false) {
    $response['message'] = 'foo bar';
}

// Add new elements to data
if(true) {
    $response['users'] = [];
}

// or just $response with __toString
return $response->json();

// Expected response after transforming data along the way
//
// {
//     "status": 200,
//     "success": true,
//     "error": null,
//     "data": {
//         'message': 'hello, world',
//         'users': []
//     }
//     "meta": null
// }

@DivineOmega
Copy link
Contributor

Looks good. Shouldn't be breaking in any way.

@dextermb
Copy link
Contributor Author

@DivineOmega Just noticed an issue, __toString must return a string... obviously but a response class is not an string so ->json() wouldn't work.

@DivineOmega
Copy link
Contributor

@dextermb In that case, the best thing you could do is to make __toString return json_encode(get_object_vars($this));. It would be missing the HTTP status code, and the JSON header, though.

@dextermb
Copy link
Contributor Author

@DivineOmega Would Laravel's response pick it up as JSON and know to respond with JSON headers?

@DivineOmega
Copy link
Contributor

@dextermb No, it would return a plain text response unless to requester specifically asked for JSON.

Idea: Why not extend Illuminate\Http\Response? The class would then be a proper response itself, so there would be no need for this manual conversion.

@dextermb dextermb added the help wanted Extra attention is needed label Aug 31, 2018
@dextermb
Copy link
Contributor Author

Would like to see someone have a go at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants