-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Make message classes final #31
Comments
Please, do not.
Lack of knowledge is a wobbly basis for decisions, isn't it? Let's assume ... which is not possible because of |
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. |
Neither do I. The current message classes of this project are already value objects. Making them
Above is counter example: PSR-7 authors believe that Another example are Mock objects, which become really cumbersome with final Therefore, which bad ideas are you want to prevent? And why do |
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.
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.
I mean literally: what's the reasoning here.
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?
Leaking application logic into the HTTP message layer is a common thing I see. Talking about ApiRequests, LocalRequests (working without actual HTTP involvement).
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. |
Sorry, I do not understand your reasoning either. On the one hand you say:
On the other hand you say:
But, that also means: you propose to make
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());
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 |
@sagikazarmark Ah, sorry, I see you already figured that out at #32 (comment) |
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.
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. |
Your questions are already answered above:
So let me ask the same question: How is this an argument?
Again, already explained above:
Sorry, but both of your statements does not make any sense. |
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
|
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. |
Sorry, but why is it not a mock? Because it is not created via a framework? You may notice the expression |
The MockHandler is just a handler that help you write tests. |
@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
So the name has nothing to do with mocks? Are you sure? As far as I know, those MockHandlers return mocks 😕 |
I tried to give you some comments in my code snippet.
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
Yes both are objects.
Yes it does.
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. |
You have used the phrase "override behavior", thus I cannot really agree. From Wikipedia:
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
Well it's actually a mock too: It simulates a server or similar:
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
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 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 #31 (comment): |
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 ? |
@ncou You won't be able to 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...
} |
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. |
Of course, but you have to handle the public function withUploadedFiles(array $uploadedFiles)
{
$clone = clone $this;
$clone->delegate = $this->delegate->withUploadedFiles($uploadedFiles);
return $clone;
} |
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. |
I personally really like this article on 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 |
From guzzle/psr7#158 (comment)
FYI @sagikazarmark
The text was updated successfully, but these errors were encountered: