Mark classes as final

[franmomu]

Related to: #1743

To be easier to review I list the ones I left without annotation (I'm not listing the already final ones and abstract):

• https://github.com/hwi/HWIOAuthBundle/blob/master/Event/UserEvent.php (there are other events extending it).
• https://github.com/hwi/HWIOAuthBundle/blob/master/OAuth/Response/PathUserResponse.php (some extends from this).
• https://github.com/hwi/HWIOAuthBundle/blob/master/Security/Core/Authentication/Token/OAuthToken.php (in tests there is a CustomOAuthToken extending)

And ResourceOwners that I wasn't sure what to do, maybe they could be marked as internal

[gassan]

I have extended OAuthProvider. Why should this be prohibited?

[stephanvierkant]

Composition over inheritance is the key. OAuthProvider implements AuthenticationProviderInterface so you can still create a class implementing that interface.

Maybe you can share some details about your use case?

[gassan]

However is the class not big and I have already overwritten almost all parent functions :). Sorry and forget it. it may be final.

[stephanvierkant]

No problem! Thanks for sharing your concern and don't hesitate to tell something about your use case as we can learn from how users implement this bundle.

[stloyd]

@XWB I think that all resource owners (ofc skipping abstract ones) should be marked as final, cause if someone needed to extend them & rewrite something, it means we have bug to be covered.

[gassan]

The users are imho clever enougth, to extend an resource owner and add some own methods. An plugin should be not only robust but also extendable.

[stloyd]

An plugin should be not only robust but also extendable.

As explained in the issue, being extendable in all places means that we force maintainers to support also bugs that may not happen before extending. Also being extendable doesn't mean you're forced to extend classes, there are "better" ways to do it.

Thus we want to force: composition/decoration over inheritance/extending.

This way you can still add your own implementation goals, but you need to call base resource owner inside of new one to get old/existing functionality.

Remember that if you overwrite existing resource owner by extending it you need to "hack" around & lie that your newly created class is the old from the plugin, which is bad practice as it hides bugs in implementation & also makes your code harder to maintain if bundle will change in future internally (going back to say that this increase also bug reports and maintenance burden on shoulders of maintainers...).

Good explain notes from fellow maintainers of API-Platform: https://api-platform.com/docs/core/extending/#leveraging-the-built-in-infrastructure-using-composition

[franmomu]

@XWB I think that all resource owners (ofc skipping abstract ones) should be marked as final, cause if someone needed to extend them & rewrite something, it means we have bug to be covered.

So, shall I add @final to them then?

[stloyd]

Thank you @franmomu !

So, shall I add @final to them then?

This can be done by either you or someone else in new MR.

[stephanvierkant]

Maybe it's time to create a release, so we can used dev-master for v2.0.

[stloyd]

@stephanvierkant There are few things left before release of 1.4 IMHO, please check: https://github.com/hwi/HWIOAuthBundle/milestone/8

[franmomu]

@stloyd sorry to ping you, but is there anything we can help with to have a new release?

By the way, I was going to try dev-master, but apparently it doesn't reference the latest commit: https://packagist.org/packages/hwi/oauth-bundle#dev-master

[stloyd]

@franmomu We need to confirm that: #1689 fixes related issue. This is last bug report in 1.4 milestone.

[franmomu]

👍 Perfect, thanks, I'll try later if I can reproduce it somehow.

[stloyd]

By the way, I was going to try dev-master, but apparently it doesn't reference the latest commit:
https://packagist.org/packages/hwi/oauth-bundle#dev-master

@franmomu I have forced update on packagist, should be good now.

[franmomu]

Great, thanks! in #1750, using the lowest versions of packages that error showed up: https://github.com/hwi/HWIOAuthBundle/pull/1750/checks?check_run_id=3161336247#step:9:126, but another one appeared 😞