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

[5.7] Allow TransportManager to create log driver with any Psr\Log\LoggerInterface instance #26842

Merged
merged 2 commits into from
Dec 14, 2018

Conversation

sebdesign
Copy link
Contributor

Addresses #26833

@sebdesign
Copy link
Contributor Author

sebdesign commented Dec 13, 2018

@driesvints would it be better to wrap the conditional in an if statement (instead of a ternary) and fetch the channel from configuration in there? Because now this call is useless if there is no LogManager bound.

@driesvints
Copy link
Member

@sebdesign yeah I agree.

@driesvints driesvints changed the title Allow TransportManager to create log driver with any Psr\Log\LoggerInterface instance [5.7] Allow TransportManager to create log driver with any Psr\Log\LoggerInterface instance Dec 13, 2018
@driesvints
Copy link
Member

@sebdesign I also think it's best to explain in your main comment why you're sending this in. It helps Taylor to understand clearly why this change is needed.

@driesvints
Copy link
Member

Could also use a failing test for the problem at hand.

@taylorotwell taylorotwell merged commit 69d11ff into laravel:5.7 Dec 14, 2018
@sebdesign
Copy link
Contributor Author

@driesvints When I implemented this feature I had a test that asserted the TransportManager would have the default log channel, but Taylor removed it, so I thought it would be useless to add a similar test again.

But I can definitely add a failing test. Should I push it here since this PR has already been merged? I'm sorry for the poor commit message though.

@driesvints
Copy link
Member

@sebdesign you can send in a new PR for that :)

sebdesign added a commit to sebdesign/framework that referenced this pull request Dec 14, 2018
…ce instance

In addition to laravel#26842, this test asserts that the `TransportManager` can create a log driver when the application has a different implementation of the `Psr\Log\LoggerInterface`, e.g. when the `illuminate/mail` package is used outside of Laravel.
sebdesign added a commit to sebdesign/framework that referenced this pull request Dec 14, 2018
…ce instance

In addition to laravel#26842, this test asserts that the `TransportManager` can create a log driver when the application has a different implementation of the `Psr\Log\LoggerInterface`, e.g. when the `illuminate/mail` package is used outside of Laravel.
taylorotwell pushed a commit that referenced this pull request Dec 15, 2018
…ce instance (#26855)

In addition to #26842, this test asserts that the `TransportManager` can create a log driver when the application has a different implementation of the `Psr\Log\LoggerInterface`, e.g. when the `illuminate/mail` package is used outside of Laravel.
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.

3 participants