Make message classes final

[Nyholm]

From https://github.com/guzzle/psr7/pull/158#issuecomment-376237838

I'm pretty confident that I will propose making all message classes in this project final if we ever hit a 2.0 version.

Reason: HTTP messages are represented as value objects, implementing an interface. From this point I don't see any valid use cases for extending the message classes.

FYI @sagikazarmark

[schnittstabil]

Please, do not.

Reason: HTTP messages are represented as value objects, implementing an interface. From this point I don't see any valid use cases for extending the message classes.

Lack of knowledge is a wobbly basis for decisions, isn't it?

Let's assume ServerRequestInterface was not defined from the beginning, and all messages are final. Now, someone comes up with that idea, and wants to implement it - like PSR-7 does:

• interface ServerRequestInterface extends RequestInterface
• class ServerRequest extends Request implements ServerRequestInterface

... which is not possible because of final class Request :unamused:

[sagikazarmark]

I'm sorry, I don't get your reasoning.

I've seen a few attempts to extend message classes and in my humble opinion they were all bad ideas trying to add something to the HTTP messaging layer that simply didn't belong there.

Even when there are cases when you want to add something custom to the message layer, there are better ways than extending***. PSR-7 provides you interfaces which you can implement and wrap other messages if necessary.

Decorators are a good example of that.

*** I don't really like using that word in this context as those other ways are kind of "extensions" too. The point here is to avoid abusing inheritance for extension.

[schnittstabil]

I'm sorry, I don't get your reasoning.

Neither do I. The current message classes of this project are already value objects. Making themfinal does not make them more valuable.

I've seen a few attempts to extend message classes and in my humble opinion they were all bad ideas trying to add something to the HTTP messaging layer that simply didn't belong there.

Above is counter example: PSR-7 authors believe that ServerRequestInterface extends RequestInterface is a good idea. Making them final [EDIT: of course: make the classes final, interfaces cannot be final] means to throw the baby out with the bath water.

Another example are Mock objects, which become really cumbersome with final classes. Yes, one could mock the interfaces instead, and delegate all 30 methods to an instance - for some simple tests of course. But the worst part: (usually) that Mock object is a valid message object, which does not violate the Liskov substitution principle etc.

Therefore, which bad ideas are you want to prevent? And why do final outweigh the disadvantages?

[sagikazarmark]

I think you are missing my point. I didn't say that PSR-7 cannot be extended, I said inheritance shouldn't be abused for that. It unfortunately happens in the PHP world all the time.

PSR-7 authors believe that ServerRequestInterface extends RequestInterface is a good idea

Yes, because inheritance has a meaning there. It says the server request is still just an HTTP request with some server specific stuff. But it's still a plain HTTP request.

Lack of knowledge is a wobbly basis for decisions, isn't it? Making them final means to throw the baby out with the bath water. Let's assume ServerRequestInterface was not defined from the beginning, and all messages are final.

I mean literally: what's the reasoning here.

Another example are Mock objects, which become really cumbersome with final classes.

Why on earth would you mock HTTP messages? They are value objects, there isn't really a behaviour that you need to verify. It doesn't matter which methods are called if you want to check object equality. So again: why would you want to mock?

which bad ideas are you want to prevent?

Leaking application logic into the HTTP message layer is a common thing I see. Talking about ApiRequests, LocalRequests (working without actual HTTP involvement).

And why do final outweigh the disadvantages?

As I mentioned above: thanks to the interface you are perfectly in control of what happens: you can decorate messages for instance. I'm against inheritance which is a form of extension. There is time and place for that. But usually when inheritance involves value objects that's at least a break of SRP.

[schnittstabil]

Sorry, I do not understand your reasoning either. On the one hand you say:

Yes, because inheritance has a meaning there. [ServerRequestInterface extends RequestInterface]

On the other hand you say:

I don't see any valid use cases for extending the message classes.

But, that also means: you propose to make Request final! Which breaks the implementation of this project:

class ServerRequest extends Request ...

---

Why on earth would you mock HTTP messages?

It seems, you are not very familiar with mocking. The system under test (sut) is typically different from the mock:

// does not work with final(!)
$mock = $this->getMockBuilder(ServerRequest::class)
             ->setMethods(['getAttribute'])
             ->getMock();
$mock->expects($this->once())
     ->method('getAttribute')
     ->with($this->equalTo('something'));

$sut = new RequestHandler();
$res = $sut->handle($mock);
$this->assertEquals(200, $res->getStatusCode());

---

It doesn't matter which methods are called if you want to check object equality.

Btw, PSR-7 Messages are not that kind of value objects. PHP is not Java, for most PSR-7 implementations the following holds:

// for example:
$res = new Response(200)
$res2 = $res->withStatus(400)->withStatus(200);

// We cannot check object equality of PSR-7 Messages using equals() or similar, all we can do is:
$res === $res2 //=> false

value object in this case just means immutable objects. And even that does not hold in the strict sense for PSR-7, because of getBody() etc.

[schnittstabil]

But, that also means: you propose to make Request final!

@sagikazarmark Ah, sorry, I see you already figured that out at https://github.com/Nyholm/psr7/pull/32#issuecomment-378049481

[sagikazarmark]

But, that also means: you propose to make Request final!

Honestly, I think that has very little relevance. You should rely on the contract, where the inheritance expresses a connection between the two types of requests.

It seems, you are not very familiar with mocking.

Again: how is this an argument?

IMHO your code sample shows an anti-pattern: you shouldn't mock value objects. (Not to mention that you mock an implementation, instead of an interface?????) The goal is to make sure that your function returns an object with the desired properties. The way it is achieved is almost irrelevant.

Regarding object equality: does a request have the same headers as the other? Same body?...etc. Then they are equal...and your test can pass (or fail).

I don't really see this conversation going anywhere, especially because things are getting personal, so I'm outta here.

[schnittstabil]

Your questions are already answered above:

It seems, you are not very familiar with mocking.

Again: how is this an argument?

The system under test (sut) is typically different from the mock

---

IMHO your code sample shows an anti-pattern: you shouldn't mock value objects.

So let me ask the same question: How is this an argument? Why shows my code an anti-pattern? Sorry, but your very next statements, does not explain it all.

---

Not to mention that you mock an implementation, instead of an interface?????

Again, already explained above:

Yes, one could mock the interfaces instead, and delegate all 30 methods to an instance.

---

But, that also means: you propose to make Request final! Which breaks the implementation of this project.

Honestly, I think that has very little relevance. You should rely on the contract, where the inheritance expresses a connection between the two types of requests. ... Regarding object equality: does a request have the same headers as the other? Same body?...etc. Then they are equal...and your test can pass (or fail).

Sorry, but both of your statements does not make any sense.

[schnittstabil]

Not to mention that you mock an implementation, instead of an interface?????

Do you know guzzle? Just open an arbitrary test of the project you've contributed, and you will see tests like that:

    public function testIgnoresWhenNoLocation()
    {
        $response = new Response(304);
        $stack = new HandlerStack(new MockHandler([$response]));
        // ...
        $this->assertEquals(304, $response->getStatusCode());
    }

@sagikazarmark FYI, the mock is $response. It should be obvious, but for clarification:

1. Response is not an interface, it is a concrete implementation
2. the mock is set up via its constructor to return the status code 304

[Nyholm]

Michael, I’ll stop you right there. Mark is the person who done all the work on Guzzle the last year.

Also, the test you show is using a concrete class. Not a mock.

I’ve not seen any argument based on computer science or good software practice that says that they should NOT be final. I’ve only heard “it will be easier to mock”. And Michaels latest code example (and Marks arguments) shows that you should not mock requests and responses.

I’m happy to continue the discussion if arguments are based on software design. One can not attack other people and question their knowledge. If this continues I’ll lock the thread.

[schnittstabil]

Also, the test you show is using a concrete class. Not a mock.

Sorry, but why is it not a mock? Because it is not created via a framework? You may notice the expression new MockHandler([$response]) as well. As a MockHandler handles mocks, I do not see your point...

[Nyholm]

// This is not a mock, it is the real object
$request = new Request();

// This is a mock or a "fake" version of a class
$request = $this->getMockBuilder(Request::class)->getMock();

The MockHandler is just a handler that help you write tests.

[schnittstabil]

@Nyholm As you said, you want to discuss on arguments, I take you at your word:

$request = $this
            ->getMockBuilder(Request::class)
            ->setConstructorArgs(['GET', 'https://foobar.example'])
            ->setMethods(null)
            ->getMock();

Is the above $request a mock, or not? You just gave two examples without explanation. How do you distinguish real objects from mock objects? Btw, both are objects, and both are real, aren't they? Please take into account, that the above $request object behave the same as an object created via new Response('GET', 'https://foobar.example').

---

The MockHandler is just a handler that help you write tests.

So the name has nothing to do with mocks? Are you sure? As far as I know, those MockHandlers return mocks :confused:

[Nyholm]

I tried to give you some comments in my code snippet.

How do you distinguish real objects from mock objects?

The point of mocks is that other classes should not be able to see the difference. You, that write the tests, you create mocks because you want to override a behavior. You can find more about mocks and why in the PHPUnit documentation

Btw, both are objects, and both are real, aren't they?

Yes both are objects.

Please take into account, that the above $request object behave the same as an object created via new Response('GET', 'https://foobar.example').

Yes it does.

So the name has nothing to do with mocks? Are you sure?

Im not sure about the implementation of this specific class. It is a real class, not a mock, and I assume it is used to ease testing.

---

The point Mark is making is that you do not need to "override a behavior" for value objects. Since they are just getters and setters. They just hold a value.

[schnittstabil]

You, that write the tests, you create mocks because you want to override a behavior.

You have used the phrase "override behavior", thus I cannot really agree. From Wikipedia:

[...], mock objects are simulated objects that mimic the behavior of real objects in controlled ways.

I do not know any resource, that claims you cannot/should not use a real object for mocking. As far as I can remember, the opposite is the case: avoid test doubles fake objects whenever possible. Well, in the example above a real object is used for mocking:

1. The mock object is a simulated object: For example, the HTTP Headers of $response will never go through your network stack - and will never be sent over a medium.
2. $request mimics the behaviour of a 304 Not Modified response in controlled ways.

---

It is a real class, not a mock, and I assume it is used to ease testing.

Well it's actually a mock too: It simulates a server or similar:

Guzzle provides a mock handler that can be used to fulfill HTTP requests with a response or exception by shifting return values off of a queue.

From the docs:

// Create a mock and queue two responses.
$mock = new MockHandler([
    new Response(200, ['X-Foo' => 'Bar']),
    new Response(202, ['Content-Length' => 0]),
    new RequestException("Error Communicating with Server", new Request('GET', 'test'))
]);

My point is: in this case the Responses and the RequestException are mocks too.

---

The point Mark is making is that you do not need to "override a behavior" for value objects. Since they are just getters and setters.

At first, PSR-7 messages do not have setters, they were designed to be immutable. (Sidenote: According to O’Phinney, method names which start with with and without were chosen to reflect that.)

Anyway, that argument does not make sense to me. Why do I never need to "override a behavior" of value objects? Why does "value object" matter in that case? Take my example at https://github.com/Nyholm/psr7/issues/31#issuecomment-378371023: Why shouldn't I want to assert $request->getAttribute('something') was called by my $sut?

[ncou]

just a silly question, I am not totally familiar with the php pattern. but what is the best way to use the futur final classes when you want to add a bunch of helper functions for the Response object or the ServerRequest ?

[schnittstabil]

@ncou You won't be able to extend the ServerRequest. Thus, either you copy-and-paste the 169 lines of code of the final ServerRequest or create your own ServerRequest and delegate all method calls to an instance variable, i.e.:

class ServerRequestProxy implements ServerRequestInterface
{
    /** @var ServerRequest */
    private $delegate;

    /**
     * @param string                               $method  HTTP method
     * @param string|UriInterface                  $uri     URI
     * @param array                                $headers Request headers
     * @param string|null|resource|StreamInterface $body    Request body
     * @param string                               $version Protocol version
     * @param array                                $serverParams Typically the $_SERVER superglobal
     */
    public function __construct(
        $method,
        $uri,
        array $headers = [],
        $body = null,
        $version = '1.1',
        array $serverParams = []
    ) {
        $this->delegate = new ServerRequest($method, $uri, $headers, $body, $version, $serverParams);
    }

    // add all 30 methods of the ServerRequestInterface, similar to:
    public function getServerParams(): array
    {
        return $this->delegate->getServerParams();
    }
}

If you add your helpers above, then you will (probably) violate SRP, therefore you should extend the class instead:

class AwesomeServerRequest extends ServerRequestProxy
{
   // add your helper methods...
}

[ncou]

Thank you, i was thinking to use something similar => create a new classe with a delegate noted as private properties, but use a __call method to redirect all the methods.

something like this (with perhaps a check to throw an execption if the method does not exist) :

public function __call($func, $args)
    {
        return call_user_func_array($func, $args);
    }

As for the SRP i was thinking to add directly my helpers. Thank you for the tips.

[schnittstabil]

Of course, but you have to handle the with* methods appropriately, e.g.:

    public function withUploadedFiles(array $uploadedFiles)
    {
        $clone = clone $this;
        $clone->delegate = $this->delegate->withUploadedFiles($uploadedFiles);
        return $clone;
    }

[ncou]

Hum you are right, i will need to handle the with/witoutXXXX methods in a special way, even if this mean It will not be so easy and clean as i was expecting.

[Zegnat]

I personally really like this article on final. Just putting that here for people reading along.

For adding helper methods, I would be very tempted to do a decorator/wrapper sort of thing:

class RequestHelper implements RequestInterface
{
    private $request;
    public function __construct(RequestInterface $request)
    {
        $this->request = $request;
    }
}

This is specifically possible because RequestInterface does not define a __construct. But yes, this does mean you end up having to manually copy over all of the other methods defined by RequestInterface. Sadly you can’t use __call as PHP forces you to implement the methods from the interface.