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

Logger Dependency Injection #54

Closed
wants to merge 1 commit into from
Closed

Logger Dependency Injection #54

wants to merge 1 commit into from

Conversation

bbatsche
Copy link

You have such lovely log statements, it'd be nice if I could get them to go somewhere useful. ;-)

Also loaded composer's autoload files when running PHPUnit.

@dvdoug
Copy link
Owner

dvdoug commented Aug 29, 2016

Hello

Thanks for the patch.

The classes use the LoggerAwareTrait from PSR-3 which means they all support injecting a logger via ->setLogger($someLogger) for those that want one. Does that not work for your particular use case?

@bbatsche
Copy link
Author

Well shucks, should have paid more attention to the PSR-3 API.

Still though, that would work for logs coming from Packer, but since Packer creates it's own instance of VolumePacker I wouldn't be able to see any of VolumePacker's log statements unless Packer forwarded on its own logger.

Putting the interface in the constructor also makes dependency injection a bit easier (although it's by no means impossible without it).

@dvdoug
Copy link
Owner

dvdoug commented Sep 3, 2016

I'm still dubious about putting it in the constructor since to me it's not really a dependency at all (it works without one).

But I do agree the main Packer needs to forward on the logger implementation regardless of how the logger is initially passed in.

@dvdoug
Copy link
Owner

dvdoug commented Sep 20, 2016

I've just released v2.0.1 which injects the logger into the child classes at runtime. You'll need to call setLogger on the main Packer object though :)

@dvdoug dvdoug closed this Sep 20, 2016
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