Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Add shortcut method for empty responses #58

Merged
merged 10 commits into from
Jun 23, 2015

Conversation

franzliedke
Copy link
Contributor

StringResponse::blank(401), as that's another common use case (though now StringResponse may not be perfect).

It's called blank() because empty() cannot be used as method name until PHP 7. Any better ideas?

@weierophinney
Copy link
Member

@franzliedke I'd argue a status 204 with a reason phrase "No Content" as defaults would make most sense here; you could then call the method noContent(). Thoughts?

@weierophinney weierophinney added this to the 1.1.0 milestone Jun 22, 2015
@@ -80,12 +92,12 @@ private function __construct()
* @param string $contentType
* @return Response
*/
private static function createResponse($body, $status, array $headers, $contentType)
private static function createResponse($body, $status, array $headers, $contentType = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep the original signature, but have the caller send a null value or an empty string.

@weierophinney
Copy link
Member

Also, needs tests. :)

@franzliedke
Copy link
Contributor Author

@weierophinney Took care of your feedback:
Parameter restored, method renamed, status code 204 by default, test added.

@weierophinney
Copy link
Member

On review, I'm not sure this belongs in the StringResponse; there's no string!

I was commenting earlier on another PR, and I wonder if perhaps we could do this instead:

namespace Zend\Diactoros\Response;

use Zend\Diactoros\Response;
use Zend\Diactoros\Stream;

class NoContent extends Response
{
    public function __construct(array $headers = [], $status = 204)
    {
        $body = new Stream('php://memory', 'r');
        parent::__construct($body, $status, $headers);
    }
}

With usage like:

// Assuming "use Zend\Diactoros\Response\NoContent;
return new NoContent();

This provides the simplicity you're looking for, while keeping it semantically separate from the string-based response types. The constructor omits the body, and ensures the body is read-only. Since a "no content" status typically won't change, headers become the first argument, as those are the one aspect of the response that you may want to vary (e.g., to provide a location header, a link header, an authorization token, etc.).

Thoughts?

@franzliedke
Copy link
Contributor Author

Why not.

I wouldn't change the arguments order, though, that would be confusing. Also, you may want to have empty responses for something like 201 Created or 202 Accepted, too, so changing the status is not so uncommon.

When only wanting to change the header, but not caring about the status, we could even provide an alternate static constructor, or people could just resort to this:

$response = (new NoContent)->withHeaders(...);

@weierophinney
Copy link
Member

@franzliedke You've convinced me; go for it!

@franzliedke
Copy link
Contributor Author

Can we call it EmptyResponse now?

@weierophinney
Copy link
Member

@franzliedke sure.

@franzliedke
Copy link
Contributor Author

Done as you suggested. Used php::temp instead, though, as that's what we had in the StringResponse class.

@weierophinney
Copy link
Member

@franzliedke I'd chosen php://memory as the liklihood of an empty response ever going to the temp filesystem is essentially nil. :) Either will work, though; php://temp uses memory until memory limits are reached.

@franzliedke
Copy link
Contributor Author

Ah, thanks for clarifying.

I think I'm done with this one. :)

@weierophinney weierophinney merged commit efa0fa7 into zendframework:develop Jun 23, 2015
weierophinney added a commit that referenced this pull request Jun 23, 2015
Add shortcut method for empty responses
weierophinney added a commit that referenced this pull request Jun 23, 2015
weierophinney added a commit that referenced this pull request Jun 23, 2015
weierophinney added a commit that referenced this pull request Jun 23, 2015
weierophinney added a commit that referenced this pull request Jun 24, 2015
Combined entries from #52, #58, #59, and #61 into one single narrative entry.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants