Skip to content

[MonologBridge] Fix support for monolog 3.0 #52222

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
Oct 29, 2023

Conversation

louismariegaborit
Copy link
Contributor

@louismariegaborit louismariegaborit commented Oct 21, 2023

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

handle function in HandlerInterface supports only LogRecord argument in monolog 3.0.

@xabbuh
Copy link
Member

xabbuh commented Oct 21, 2023

Can you add a test to prevent future regressions?

@louismariegaborit
Copy link
Contributor Author

Can you add a test to prevent future regressions?

I think it's difficult in this PR to add test because mock stream functions will cause a refactoring of the command.
I might try doing this in an other PR. What do you think ?

@louismariegaborit louismariegaborit force-pushed the fix_server_log_command branch 2 times, most recently from 8a05567 to 3555d5a Compare October 22, 2023 07:45
@louismariegaborit
Copy link
Contributor Author

I don't know how to fix psalm error because in monolog 2.x, LogRecord is an interface and psalm runs with this version.
Anyone have an idea?

channel: $record['channel'],
context: $record['context']->getContext(),
datetime: $record['datetime'],
message: $record['message'],
Copy link
Member

Choose a reason for hiding this comment

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

let's not use named arguments if possible

Copy link
Contributor Author

@louismariegaborit louismariegaborit Oct 29, 2023

Choose a reason for hiding this comment

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

Ok I did the change. But why don't use named arguments ?
Named arguments are already used for the LogRecord.

@stof
Copy link
Member

stof commented Oct 25, 2023

Don't care about Psalm in this case. This is an expected error due to supporting several versions of Monolog. Our psalm setup only reports new issues for each PR, so this won't both future PRs.

@derrabus derrabus force-pushed the fix_server_log_command branch from e5afa36 to 3aacc28 Compare October 29, 2023 23:04
@derrabus
Copy link
Member

Good catch, thanks @louismariegaborit.

@derrabus derrabus merged commit 0196872 into symfony:6.3 Oct 29, 2023
@louismariegaborit louismariegaborit deleted the fix_server_log_command branch October 30, 2023 02:36
@DavidPrevot DavidPrevot mentioned this pull request Jan 10, 2024
nicolas-grekas added a commit that referenced this pull request Jan 29, 2024
…ouismariegaborit)

This PR was merged into the 6.3 branch.

Discussion
----------

[MonologBridge] Fix context data and display extra data

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | ->
| License       | MIT

Fix the compatibility with Monolog 3. In the PR #52222 we had the compatibility with Monolog 3 but context parameter is incorrect and extra parameter is missing.

No test can be added because testing this command is not possible at this time. Another PR (#53518) was opened to add test on this command.

Commits
-------

8687ae0 Fix context data and display extra data
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.

6 participants