Skip to content
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

Remove final keyword #169

Merged
merged 5 commits into from
Feb 18, 2021
Merged

Remove final keyword #169

merged 5 commits into from
Feb 18, 2021

Conversation

Nyholm
Copy link
Owner

@Nyholm Nyholm commented Feb 10, 2021

I don't think there is any benefit to remove the final keyword. All the good software scientists and best practises says that you should never efter extend these value objects.

However, some people insists on writing code that I don't approve of and from their perspective the final keyword is just annoying.

This PR does not change the concept, but technically it allows some people to write the code they want to write even tough I consider it suboptimal.


Edit: When I read this description again, it looks that Im condescending. Im really not. Im trying to be pragmatic.

@Zegnat
Copy link
Collaborator

Zegnat commented Feb 10, 2021

I also really dislike this, because extending a PSR implementation tends to people relying on the extended version, which in turn means they have just limited themselves to never be able to switch implementations. A problem that does not happen if you follow a decorator pattern. The entire point is interoperability. I almost feel this is just helping someone else get into trouble down the line.

I have been thinking about ways to help people build decorators, and wonder if it might be better to offer some sort of abstract class to extend that implements all the methods. E.g. we'd be providing the abstract class called ADecoratorBase from this example:

interface AnInterface { // Imagine this is a PSR-7 value object interface.
    public function action(): bool;
}

final class AnImplementation implements AnInterface { // Imagine this is our implementation of said interface.
    public function action(): bool {
        return true;
    }
}

abstract class ADecoratorBase implements AnInterface { // Imagine this is a "decorator" we provide.
    protected AnInterface $base;

    public function action(): bool {
        return $this->base->action();
    }
}

class MyDecorator extends ADecoratorBase { // Now all a user needs to do to decorate is this.
    public function __construct(AnInterface $base)
    {
        $this->base = $base;
    }
}

$obj = new MyDecorator(new AnImplementation());
assert($obj->action() === true, 'Accessing expected method from base.');

Alternatively shipping something like the Grav traits that @mahagr linked to in #139 (comment).

Just wondering if we can do some sort of educative good by doing that, rather than just dropping the final keyword that we both believe as correct for this library 🤔

@mahagr
Copy link

mahagr commented Feb 11, 2021

I have to say that extending class also sounds tempting as it would allow customization without too much work. This is also where the traits come -- it will save a lot of work. The reason why I ended up using traits was to be able to inject the PSR-7 interface into any (existing) class.

I think it's also a good idea to separate decorator/traits from this library in order to prevent fixating code into a single library.

@Nyholm
Copy link
Owner Author

Nyholm commented Feb 11, 2021

@Zegnat, Yeah, but looking at the implementations, most methods just have a few lines, I don't think it make much sense to also add a bunch of abstract classes.

What if we add a page in the documentation to better explain why extending classes are a bad idea. We can also show what one should do instead. The @final comment can link to that page.

@Nyholm
Copy link
Owner Author

Nyholm commented Feb 16, 2021

The PR is updated

Copy link
Collaborator

@Zegnat Zegnat left a comment

Choose a reason for hiding this comment

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

Yeah, but looking at the implementations, most methods just have a few lines, I don't think it make much sense to also add a bunch of abstract classes.

Yet apparently people do not want to copy the few lines and make their own class. I also wouldn’t put the actual implementation in the abstract classes, only make an abstract decorator (wrapper) class that calls the methods from the actual wrapped value object.

But the more I think about that, the less I would want that as part of the Psr7 library. I might just create a Psr7Decorator package later…

What if we add a page in the documentation to better explain why extending classes are a bad idea. We can also show what one should do instead. The @final comment can link to that page.

I like adding implicit the docs.

I wonder if we should point to some patterns that people might actually be looking for, rather than extending. Such as decorating. There is nothing wrong with wanting to wrap a value object adding additional logic to match the application.

doc/final.md Outdated Show resolved Hide resolved
Co-authored-by: Martijn van der Ven <martijn@vanderven.se>
@Nyholm
Copy link
Owner Author

Nyholm commented Feb 16, 2021

Thank you.

Let me improve the docs with some more references.

@Nyholm Nyholm requested a review from Zegnat February 17, 2021 07:58
@Nyholm Nyholm merged commit 23ae1f0 into master Feb 18, 2021
@Nyholm Nyholm deleted the patch-no-final branch February 18, 2021 15:51
@frodeborli
Copy link

First of all; I like the quality of your work - which is why I decided to use your PSR-7 implementation. Unfortunately, I have to drop it or fork it due to the @final keyword.

The PSR does not define the signature for the __construct method. Since your class prohibits overriding the constructor - thanks to the @final keyword, I don't think your implementation complies

I'm sad to learn that you have decided to mark your class with @final, for political reasons. Reason seems to be that you think I should change my style of programming because you think composition is awesome...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants