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

Every handler needs to inject the config even if is not needed #313

Open
pinclau opened this issue Sep 6, 2024 · 5 comments · May be fixed by #329
Open

Every handler needs to inject the config even if is not needed #313

pinclau opened this issue Sep 6, 2024 · 5 comments · May be fixed by #329
Assignees
Labels
5.x enhancement New feature or request

Comments

@pinclau
Copy link
Member

pinclau commented Sep 6, 2024

Every new handler needs to inject the config in order to work, even if you create a simple handler for outputing a simple get response.
This is because the method isGetCollectionRequest from HandlerTrait request it.

I think this is related to #269

@arhimede
Copy link
Member

arhimede commented Sep 9, 2024

what if we ask @Xerkus to give his opinion in this matter ?

@Xerkus
Copy link

Xerkus commented Sep 9, 2024

config in the handler is messed up. Deprecate and get rid of it.

Those handler traits are problematic from code quality perspective. It would be good to get rid of them as well. Move them into injectable dependencies.

I am cloning the repo and will take a closer look.

@alexmerlin
Copy link
Member

config in the handler is messed up. Deprecate and get rid of it.

Config in handle has only one purpose: to find out if a GET request is for a single item or a collection of items.
I was thinking of a middleware that would identify this so in HandlerTrait::handler method I would use this new value and then I could remove the config from all handlers.

Those handler traits are problematic from code quality perspective. It would be good to get rid of them as well. Move them into injectable dependencies.

Why are they problematic? They are just as easy to test as a handler.

I am cloning the repo and will take a closer look.

I'm curious to see your findings. :)

@Xerkus
Copy link

Xerkus commented Sep 9, 2024

Handler being aware of config structure, of metadata map and its structure is a sever violation of single responsibility principle.

Extract the isGetCollectionRequest into an interface + concrete implementation, register that in container and inject in place of the config as a regular dependency. It will massively simplify the handler.

Better yet, I would recommend to abandon the approach of handling the resource and its collection in the same handler.
They require different urls and respond differently to the http methods. Split into separate handlers and there won't be a reason to perform isGetCollectionRequest check in the first place.

I personally prefer to separate get and post/patch/delete actions further into separate handlers. It makes middleware pipes so much more powerful with a simple composition.

For the response trait I would also recommend to extract it into an object in its entirety and inject it as a dependency. It can be improved by utilizing PSR-17 factories and further improved by the use of Mezzio\ProblemDetails\ProblemDetailsResponseFactory. RFC 7807 problem details response is far superior to any custom error response.

@arhimede arhimede pinned this issue Sep 10, 2024
@arhimede arhimede added enhancement New feature or request 5.x labels Sep 10, 2024
alexmerlin added a commit that referenced this issue Sep 10, 2024
@alexmerlin alexmerlin self-assigned this Sep 13, 2024
@arhimede arhimede linked a pull request Sep 13, 2024 that will close this issue
arhimede added a commit that referenced this issue Sep 13, 2024
Issue #313: Remove `config` dependency from handlers.
@alexmerlin
Copy link
Member

Better yet, I would recommend to abandon the approach of handling the resource and its collection in the same handler.
They require different urls and respond differently to the http methods. Split into separate handlers and there won't be a reason > to perform isGetCollectionRequest check in the first place.

We elliminated the config dependency from all handlers by moving collection-related code to different handlers.
Each resource (Admin, AdminRole, User, UserRole) has it's own collection handler implementing the get method which returns a collection of the requested resource.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.x enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants