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

[11.x] Fix 'pushProcessor method not found on LoggerInterface' error #52117

Merged
merged 8 commits into from
Jul 16, 2024

Conversation

cosmastech
Copy link
Contributor

@cosmastech cosmastech commented Jul 14, 2024

I came across this trying to unit test a package I'm working on where I want to use a logger spy. Though the only requirement stated is that the logger must be of Psr\Log\LoggerInterface, in practice, the log channel cannot be built.

Due to the change when adding the Context functionality, there's an implicit dependency that the LoggerInterface used by Logger must have a pushProcessor() method (provider by Monolog's ProcessableHandlerInterface).

The alternative approach is to add a method for Logger@pushProcessor().

todo:

  • add unit tests

Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@cosmastech cosmastech changed the title [11.x] Add Logger@pushProcessor() [11.x] Fix 'pushProcessor method not found on LoggerInterface' error Jul 15, 2024
@cosmastech cosmastech force-pushed the log-manager/can-use-psr-logger branch from 8da09a9 to d94d533 Compare July 15, 2024 01:24
@cosmastech cosmastech marked this pull request as ready for review July 15, 2024 01:25
@cosmastech cosmastech force-pushed the log-manager/can-use-psr-logger branch from d94d533 to 51c0903 Compare July 15, 2024 01:28
@taylorotwell
Copy link
Member

Can this be formatted differently? I can't really tell what is going on.

@taylorotwell taylorotwell marked this pull request as draft July 15, 2024 22:16
@cosmastech cosmastech force-pushed the log-manager/can-use-psr-logger branch from 51c0903 to aa274e8 Compare July 15, 2024 23:38
@cosmastech
Copy link
Contributor Author

@taylorotwell hopefully this is easier to read. I find use of tap() inside of tap() to be very hard to read and to reason about, so I removed a tap statement.

For clarity, the goal is to only call Logger::pushProcessor() if the Logger::$logger instance has a method called pushProcessor.

As I was modifying this, another thought would be to allow Logger::withContext() to accept a Closure. Seems like a bigger, more error-prone lift.

The failing tests look like they're related to sqlite changes from earlier today.

@cosmastech cosmastech marked this pull request as ready for review July 15, 2024 23:56
@cosmastech cosmastech force-pushed the log-manager/can-use-psr-logger branch from 51bcd55 to 9f11814 Compare July 16, 2024 09:44
@taylorotwell taylorotwell merged commit c898498 into laravel:11.x Jul 16, 2024
28 checks passed
@taylorotwell
Copy link
Member

Thanks!

@cosmastech cosmastech deleted the log-manager/can-use-psr-logger branch July 17, 2024 11:00
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