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

[AdminBundle] Mark classes internal/final #8453

Merged
merged 8 commits into from Mar 19, 2021
Merged

Conversation

dvesh3
Copy link
Contributor

@dvesh3 dvesh3 commented Mar 15, 2021

Changes in this pull request

Resolves partially #7189

Additional info

Co-authored-by: Bernhard Rusch <brusch@users.noreply.github.com>
@dvesh3 dvesh3 requested a review from brusch March 18, 2021 16:10
@brusch brusch merged commit 5970312 into master Mar 19, 2021
@brusch brusch deleted the mark_internal_adminbundle branch March 19, 2021 11:43
roland4432 pushed a commit to roland4432/pimcore that referenced this pull request Apr 1, 2021
* [AdminBundle] Admin Controllers declared as final and marked @internal - pimcore#7189

* [AdminBundle] Admin Event Listeners declared as final and marked @internal - pimcore#7189

* [AdminBundle] Mark Admin Helpers @internal - pimcore#7189

* [AdminBundle] Security handlers declared final and classes marked @internal  - pimcore#7189

* [AdminBundle] Mark Admin classes @internal - pimcore#7189

* [AdminBundle] Mark Admin classes internal - docs pimcore#7189

* [AdminBundle] Mark DI & bundle class internal and declare as final pimcore#7189

* Apply suggestions from code review

Co-authored-by: Bernhard Rusch <brusch@users.noreply.github.com>

Co-authored-by: Bernhard Rusch <brusch@users.noreply.github.com>
@roland4432
Copy link
Contributor

What's the reason for all classes to be marked as final?

@brusch
Copy link
Member

brusch commented May 4, 2021

@roland4432 Because they are not intended to be overridden or customized. The BC promise / SemVer doesn't apply to those classes.

@roland4432
Copy link
Contributor

roland4432 commented May 4, 2021

Could you maybe leave that decission to the programmer?
This breaks dozens of projects and doesn't make any sense in the symfony flow at all.

And.. it doesn't even have the desired effect, since you can still override the controller just not extend it.

services:
       Pimcore\Bundle\AdminBundle\Controller\Admin\IndexController:
          class: AppBundle\Controller\Admin\IndexController

and the controller is replaced...

@roland4432
Copy link
Contributor

roland4432 commented May 4, 2021

I am interested in what top contributors like @BlackbitNeueMedien @dpfaffenbauer @jdreesen think about that...

@BlackbitDevs
Copy link
Contributor

I never understood the argument to make classes final. Arguments like

Using the class for something else by overriding part of its behavior may jeopardize its integrity. The state of the extended object may become unpredictable or inconsistent with the state of the original object.
from https://matthiasnoback.nl/2018/09/final-classes-by-default-why/

are simply not true. If you want to protect internal state of properties, then it is fine for me to make them private. But to prohibit extending a class will only lead to a lot of copy-paste. There are a lot of use-cases where reusing / extending the AdminBundle controllers will on the one hand make custom features easier to implement and on the other hand make them easier to migrate. Because what will happen is that all the custom features which currently extend the AdminBundle controllers which @roland4432 mentions will have to copy the methods from the original classes to still work. Then this at first will work but when those AdminBundle classes get changed, the copied methods might again not work. Of course now you will say that developers have to check their code anyway for Pimcore updates - but what is the advantage of making those classes final?

@brusch
Copy link
Member

brusch commented May 4, 2021

For me it would be ok as well to just tag them as @internal, but that would still mean that we're not maintaining backward-compatibility for those classes - "use at your own risk".

@roland4432
Copy link
Contributor

Would be totally fine with that.

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

Successfully merging this pull request may close these issues.

None yet

4 participants