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

ResponseTrait #55

Closed
wants to merge 2 commits into from
Closed

ResponseTrait #55

wants to merge 2 commits into from

Conversation

mrgrain
Copy link

@mrgrain mrgrain commented Jun 14, 2015

Moved related methods to a ResponseTrait
Using ResponseTrait, a class will implement the ResponseInterface
This basically allows for creating custom responses without the need to use the default Response constructor.

@weierophinney weierophinney added this to the 1.1.0 milestone Jun 22, 2015
@weierophinney
Copy link
Member

The main reason RequestTrait exists is because there are two RequestInterface implementations (Request and ServerRequest), and I didn't want them to inherit from each other so that I could better protect encapsulation.

Can you demonstrate some of the use cases you have for this change, please?

@mrgrain
Copy link
Author

mrgrain commented Jun 23, 2015

Yes sure, for a example a RedirectResponse could be created as simple as:

class RedirectResponse
{
    use ResponseTrait;

    public function __construct($redirectUrl)
    {
        $this->stream     =  new Stream('php://memory', 'wb+');
        $this->statusCode = 302;
        $this->headers = ['Location: '.$redirectUrl];
    }
}

This is still not perfect, as a lot has to be done manually (e.g. setting the reason phrase) but it offers an other approach for custom responses, in addition to the static generator methods from the develop-branch.

@Maks3w
Copy link
Member

Maks3w commented Jun 23, 2015

@mrgrain I think inheritance it's better in your example. Diactotors Response and your example RedirectResponse share the same semantic field

@mrgrain
Copy link
Author

mrgrain commented Jun 23, 2015

@Maks3w Yes and no. I also would prefer inheritance. The issue I have with Diactotors Response is it's signature. Especially the stream string/resource as first parameter is something I really don't care about as a package user. I know this is an important and interesting field to have, but in most cases I actually would consider the used stream an implementation detail.

@weierophinney
Copy link
Member

@mrgrain One interesting fact of PHP is that constructors can differ in child classes. This allows you to do the following:

class Response implements ResponseInterface
{
    public function __construct($body = 'php://memory', $status = 200, array $headers = [])
    {
        /* initialize */
    }
}

class Redirect extends Response
{
    public function __construct($redirectUrl, $status = 302)
    {
        parent::__construct(
            'php://memory',
            $status,
            [
                'location' => [$redirectUrl],
            ]
        );
    }
}

PHP will raise no errors in the above; constructors are exempt from the parameter inheritance rules present for any other method.

The problem that does arise, however, is that we've defined all properties in our messages as private. This is to enforce encapsulation. It does mean, however, that if you want to extend the messages, you cannot directly manipulate properties without first redefining them in your extending class — and this may lead to odd issues later down the line. And this, @Maks3w, is why inheritance may not be the correct solution for all use cases.

That said: the example I provided above already works, and doesn't require having a trait. Knowing this, @mrgrain, is the PR necessary?

@weierophinney
Copy link
Member

@mrgrain BTW, you might be interested in the following:

@weierophinney
Copy link
Member

I'm going to close this, as I think I've sufficiently demonstrated that you can create custom extensions trivially, and I'm not convinced of the need for extracting the response-specific functionality into a trait at this time.

@weierophinney weierophinney removed this from the 1.1.0 milestone Jun 23, 2015
@mrgrain
Copy link
Author

mrgrain commented Jun 23, 2015

Yes, that's perfectly fine for me :) I amazingly didn't know about the possibility to overload constructors. Learned something new today.

Thanks for the documentation links. They are looking very promising.

@weierophinney
Copy link
Member

@mrgrain Those will be in the upcoming 1.1 version; I'll likely tag it in the next couple of days. :)

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.

3 participants