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

[Adr] Kernel events vs controllers #500

Open
wants to merge 2 commits into
base: 1.10
Choose a base branch
from

Conversation

loic425
Copy link
Member

@loic425 loic425 commented Nov 19, 2022

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets
License MIT

@loic425 loic425 requested a review from a team as a code owner November 19, 2022 12:59
Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

I've tried to influence the file, but I'm lost with the problem we want to solve with it. I was thinking about DTO support at the beginning, but DTOs are not even mentioned here. The usage of kernel events can be discussed without mentioning one controller per action. Let's make a call tomorrow, discuss it and try to build ADR based on that :)

Comment on lines +9 to +10
ResourceController has all the actions, a lot of pretty similar code on each action and hard to maintain/test.
and it is not flexible enough in its behavior.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ResourceController has all the actions, a lot of pretty similar code on each action and hard to maintain/test.
and it is not flexible enough in its behavior.
Over the years ResourceController become really bloated. It has all the actions, and a lot of pretty similar code on each of them and the default approach to extend its logic is to extend the class. This class has 17 dependencies, which makes it hard to maintain and test. Moreover, some dependencies are not needed for every method call. As a result, this class does not follows SOLID principles and we want to improve its desing

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

2 participants