Nyholm / psr7

A super lightweight PSR-7 implementation
MIT License
1.16k stars 75 forks source link

Please delete attribute FInal from classes! #168

Closed symbioticphp closed 3 years ago

symbioticphp commented 3 years ago

I need to extend the class

mahagr commented 3 years ago

See https://github.com/Nyholm/psr7/issues/139

symbioticphp commented 3 years ago

The standard describes recommended basic interaction interfaces, and your solution provides the basic solution. I think many people understand that decoration is better than inheritance, but in this case there is a problem of creating an object for the sake of an object and an unnecessary description of geters proxying your methods, also decorators make it unclear to understand the code. You have made a solution for mass use, but closing the possibilities for inheritance, without an urgent need, is doubtful. I think it is correct to declare the methods themselves final and the constructor, and not to touch the class itself. Many people have to duplicate such code in the form of wrappers, and even with traits:

class ServerRequestDecorator implements \Psr\Http\Message\ServerRequestInterface
{
    public function __construct(\Psr\Http\Message\ServerRequestInterface $request)
    {

    }

    public function getProtocolVersion()
    {
        // TODO: Implement getProtocolVersion() method.
    }

    public function withProtocolVersion($version)
    {
        // TODO: Implement withProtocolVersion() method.
    }

    public function getHeaders()
    {
        // TODO: Implement getHeaders() method.
    }

    public function hasHeader($name)
    {
        // TODO: Implement hasHeader() method.
    }

    public function getHeader($name)
    {
        // TODO: Implement getHeader() method.
    }

    public function getHeaderLine($name)
    {
        // TODO: Implement getHeaderLine() method.
    }

    public function withHeader($name, $value)
    {
        // TODO: Implement withHeader() method.
    }

    public function withAddedHeader($name, $value)
    {
        // TODO: Implement withAddedHeader() method.
    }

    public function withoutHeader($name)
    {
        // TODO: Implement withoutHeader() method.
    }

    public function getBody()
    {
        // TODO: Implement getBody() method.
    }

    public function withBody(\Psr\Http\Message\StreamInterface $body)
    {
        // TODO: Implement withBody() method.
    }

    public function getRequestTarget()
    {
        // TODO: Implement getRequestTarget() method.
    }

    public function withRequestTarget($requestTarget)
    {
        // TODO: Implement withRequestTarget() method.
    }

    public function getMethod()
    {
        // TODO: Implement getMethod() method.
    }

    public function withMethod($method)
    {
        // TODO: Implement withMethod() method.
    }

    public function getUri()
    {
        // TODO: Implement getUri() method.
    }

    public function withUri(\Psr\Http\Message\UriInterface $uri, $preserveHost = false)
    {
        // TODO: Implement withUri() method.
    }

    public function getServerParams()
    {
        // TODO: Implement getServerParams() method.
    }

    public function getCookieParams()
    {
        // TODO: Implement getCookieParams() method.
    }

    public function withCookieParams(array $cookies)
    {
        // TODO: Implement withCookieParams() method.
    }

    public function getQueryParams()
    {
        // TODO: Implement getQueryParams() method.
    }

    public function withQueryParams(array $query)
    {
        // TODO: Implement withQueryParams() method.
    }

    public function getUploadedFiles()
    {
        // TODO: Implement getUploadedFiles() method.
    }

    public function withUploadedFiles(array $uploadedFiles)
    {
        // TODO: Implement withUploadedFiles() method.
    }

    public function getParsedBody()
    {
        // TODO: Implement getParsedBody() method.
    }

    public function withParsedBody($data)
    {
        // TODO: Implement withParsedBody() method.
    }

    public function getAttributes()
    {
        // TODO: Implement getAttributes() method.
    }

    public function getAttribute($name, $default = null)
    {
        // TODO: Implement getAttribute() method.
    }

    public function withAttribute($name, $value)
    {
        // TODO: Implement withAttribute() method.
    }

    public function withoutAttribute($name)
    {
        // TODO: Implement withoutAttribute() method.
    }

}
symbioticphp commented 3 years ago

@mahagr Please publish your traits in a separate package, people will be grateful. I will use it as a temporary solution. In my project, the size of the code is very important, because the application will be compiled into a single file. That is why unnecessary duplication of code is unacceptable for me. Your CMS fits well as an application into the ideology of my my project.

Nyholm commented 3 years ago

I would be happy to remove the final keyword on the classes. The only thing I need is an argument based in computer science and good practices.

So, could I ask you, why you need to extend the class?

withinboredom commented 3 years ago

I've been coming up with very intricate solutions around this, 100% in unit tests where I'd like to mock these classes so I can inject failures into my router.

Nyholm commented 3 years ago

I really do appreciate this idea. Please don't see my as a douchebag that is just trying to make other people's life for no reason.

100% in unit tests where I'd like to mock these classes so I can inject failures into my router.

That is indeed a scenario I can understand your frustration. However, best practice is to mock interfaces. Ie normally in TDD you write the interface, then the test and then actual implementation.

Also, the classes in this library is value objects, which means you usually don't need to mock them at all.

withinboredom commented 3 years ago

That is indeed a scenario I can understand your frustration. However, best practice is to mock interfaces.

To be more precise, these are integration tests. Not unit tests, I don't know why I said unit. :) So basically, I'd only want to mock the part that would trigger a specific error, so other parts of the code can use the actual behavior.

Also, I'd love to be able to extend these classes just to rewind the stream after adding it. Having to remember to do it in every test is fairly annoying. ;)

Nyholm commented 3 years ago

I dont like it, but I opened a PR =)

169

AndreKR commented 3 years ago

@withinboredom @dissonancephp Did you consider using reflection to build mocks?

Just recently I found out about the technique and it turned out to be more convenient than constantly subclassing things to build mocks.

withinboredom commented 3 years ago

@AndreKR How do you use reflection to change what a function returns?

AndreKR commented 3 years ago

I find that manipulating private member variables that are then used by the functions is usually enough.

Zegnat commented 3 years ago

I would mock the interface itself and not depend on an interface implementation for it. Which is often a lot more straightforward and might be part of your testing framework already (e.g. $this->createMock(\Psr\Http\Message\RequestInterface::class);).

Though this may not apply to integration testing. Hard to say anything there without diving into the tests. But usually I would be looking at the logic that creates the value object (which a PSR-7 message is an example of) and tweak that to change the actual value object that gets created, rather than after-creation changing the value object itself. As the whole idea with immutable value objects is that they are safe to pass around, and you may be losing that property.

Lots of options, and it will need to be judged on a case-by-case basis what is the easiest and proper way to go. I personally would never recommend extending a value object if other options exist.

But we’ll continue the discussion on dropping final from this library in the PR :)

AndreKR commented 3 years ago

I would mock the interface itself and not depend on an interface implementation for it.

I was assuming this is out of the question because the code under test type hints actual class names. If you just need to provide an interface, you usually wouldn't have any problems and wouldn't need to come here requesting the open-ization of the classes. :)

Nyholm commented 3 years ago

The final keyword is removed. However, you should not extend the classes. See https://github.com/Nyholm/psr7/blob/master/doc/final.md

symbioticphp commented 3 years ago

The final keyword is removed. However, you should not extend the classes. See https://github.com/Nyholm/psr7/blob/master/doc/final.md

Thank you very much! The point was not only to expand, but also to add a specific implementation to my namespaces (/MyVendor/Psr7), so that in the future you can easily switch to your own solution or any other.