Skip to content

[HttpKernel] the debug log processor must be a callable #52426

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

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Nov 2, 2023

Q A
Branch? 6.4
Bug fix? no
New feature? no
Deprecations? no
Issues
License MIT

A Monolog processor must be a callable, but a DebugLoggerInterface implementation is not necessarily a callable.

@xabbuh xabbuh force-pushed the pr-51284 branch 2 times, most recently from 6987632 to 81baf53 Compare November 2, 2023 13:19
@xabbuh xabbuh changed the title [HttpKernel] do not register the debug logger as a Monolog processor [HttpKernel] the debug log processor must be a callable Nov 7, 2023
@xabbuh
Copy link
Member Author

xabbuh commented Nov 7, 2023

re-reading the existing code I have updated this PR to only enforce the processor to be a callable and reverted the other changes


public function __construct(DebugLoggerInterface $processor, bool $enable = null)
public function __construct(callable $processor, bool $enable = null)
Copy link
Member

Choose a reason for hiding this comment

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

should be ProcessorInterface|callable for monolog 3, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could change it that way, but it's not needed IMO as the ProcessorInterface does nothing more than enforcing the __invoke() method to be present: https://github.com/Seldaek/monolog/blob/main/src/Monolog/Processor/ProcessorInterface.php#L26

Copy link
Member

Choose a reason for hiding this comment

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

oh perfect then

@nicolas-grekas
Copy link
Member

Thank you @xabbuh.

@nicolas-grekas nicolas-grekas merged commit fbc44f2 into symfony:6.4 Nov 7, 2023
@xabbuh xabbuh deleted the pr-51284 branch November 7, 2023 13:00
nicolas-grekas added a commit that referenced this pull request Nov 9, 2023
This PR was merged into the 6.4 branch.

Discussion
----------

[HttpKernel] Fix DebugLoggerConfigurator

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | -
| License       | MIT

Issue introduced in #52426: by turning the callable into a closure, we broke the `instanceof` check later in the class.

Commits
-------

33721f5 [HttpKernel] Fix DebugLoggerConfigurator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants