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

Introduce GetServiceTrait. #3843

Merged
merged 1 commit into from
Jul 30, 2024
Merged

Conversation

demiankatz
Copy link
Member

@demiankatz demiankatz commented Jul 29, 2024

This PR introduces a trait with a getter that wraps around $this->serviceLocator->get() in order to improve type validation in the code. Now, fetching a service from the service locator will be associated with the appropriate type, enabling more accurate IDE typehints and static analysis.

This trait can also serve as a marker for classes that currently make direct access to the service locator, since in the long term it would be better to replace many of these retrieval operations with cleaner dependency injection. I expect that this trait will remain useful indefinitely for some of our more complex factories, however!

TODO

  • Run full test suite

@demiankatz demiankatz added the architecture pull requests that involve significant refactoring / architectural changes label Jul 29, 2024
@demiankatz demiankatz added this to the 10.1 milestone Jul 29, 2024
Copy link
Contributor

@xmorave2 xmorave2 left a comment

Choose a reason for hiding this comment

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

I do like this change. And I do agree, it should be used probably only in factories in the long term, but for now it is useful and we could try to eliminate the need of direct access to service manager from classes in smaller steps in the future.

@demiankatz demiankatz merged commit dfb3a95 into vufind-org:dev Jul 30, 2024
7 checks passed
@demiankatz demiankatz deleted the get-service-trait branch July 30, 2024 17:53
LuomaJuha pushed a commit to LuomaJuha/NDL-VuFind2 that referenced this pull request Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture pull requests that involve significant refactoring / architectural changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants