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.

Sign up for GitHub

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

Jump to bottom

Allow to use dbal logging middleware instead of deprecated SQLLogger #1456

Merged
merged 1 commit into from
Jan 5, 2022
Merged

Allow to use dbal logging middleware instead of deprecated SQLLogger #1456

merged 1 commit into from
Jan 5, 2022

Conversation

l-vo
Copy link
Contributor

@l-vo l-vo commented Dec 29, 2021 •
edited
Loading

Fixes #1429

@ostrolucky ostrolucky changed the base branch from 2.5.x to 2.6.x December 29, 2021 14:11
@l-vo l-vo force-pushed the allow_to_use_logging_middleware branch 3 times, most recently from 5563641 to 2ac02d3 Compare December 29, 2021 14:40
@stof
Copy link
Member

stof commented Dec 29, 2021

To me, there is no reason for having a configuration for that. The core loggers should be wired as middles rather than loggers on newer DBAL versions without any config change.

@l-vo l-vo changed the title Allow to use dbal logging middleware instead of deprecated SQLLogger WIP: Allow to use dbal logging middleware instead of deprecated SQLLogger Dec 30, 2021
@l-vo l-vo force-pushed the allow_to_use_logging_middleware branch 5 times, most recently from d35d639 to 75adbe9 Compare January 1, 2022 22:20
@ostrolucky
Copy link
Member

like @stof said, this should work without change in configuration

@l-vo l-vo force-pushed the allow_to_use_logging_middleware branch from 75adbe9 to f5bb700 Compare January 2, 2022 13:17
@l-vo l-vo changed the title WIP: Allow to use dbal logging middleware instead of deprecated SQLLogger Allow to use dbal logging middleware instead of deprecated SQLLogger Jan 2, 2022
@l-vo
Copy link
Contributor Author

l-vo commented Jan 2, 2022

@stof @ostrolucky thank you for your feedbacks, the point is the logged message formats are not the same; for a basic query, with SQLLogger:

[2022-01-02T12:47:18.370397+00:00] doctrine.DEBUG: SELECT t0.id AS id_1, t0.name AS name_2, t0.price AS price_3, t0.version AS version_4 FROM product t0 [] []

and with logging middleware:

[2022-01-01T13:45:40.827235+00:00] doctrine.DEBUG: Executing query: SELECT t0.id AS id_1, t0.name AS name_2, t0.price AS price_3, t0.version AS version_4 FROM product t0 {"sql":"SELECT t0.id AS id_1, t0.name AS name_2, t0.price AS price_3, t0.version AS version_4 FROM product t0"} []

It might be a problem if one do parsing of these logs for some reason. It's why I suggest an opt-in for this change.

@ostrolucky
Copy link
Member

I see. That change is fine, we don't provide BC promise for log format.

@l-vo
Copy link
Contributor Author

l-vo commented Jan 2, 2022

Ok, so I'm going to remove the opt-in.

@l-vo l-vo changed the title Allow to use dbal logging middleware instead of deprecated SQLLogger WIP: Allow to use dbal logging middleware instead of deprecated SQLLogger Jan 2, 2022
@ostrolucky ostrolucky added this to the 2.6.0 milestone Jan 2, 2022
@l-vo
Allow to use dbal logging middleware
c416af5 
instead of deprecated SQLLogger
@l-vo l-vo force-pushed the allow_to_use_logging_middleware branch from f5bb700 to c416af5 Compare January 2, 2022 13:51
@l-vo l-vo changed the title WIP: Allow to use dbal logging middleware instead of deprecated SQLLogger Allow to use dbal logging middleware instead of deprecated SQLLogger Jan 2, 2022
@ostrolucky
Copy link
Member

Does this solve the deprecations we currently have? See https://github.com/doctrine/DoctrineBundle/runs/4681778064?check_suite_focus=true I am thinking if it wouldn't be better to remove definition of doctrine.dbal.logger completely, instead of only its arguments.

@l-vo
Copy link
Contributor Author

l-vo commented Jan 2, 2022 •
edited
Loading

No, it doesn't, removing these deprecations needs two steps, the second (#1431) is about the profiler and involve to modify Doctrine Bridge (IMHO). I can work on it after this one. We can't remove doctrine.dbal.logger now because it also does time measurements (stopwatch) for the profiler.

dmaicher
dmaicher approved these changes Jan 5, 2022
View reviewed changes
Copy link
Contributor

@dmaicher dmaicher left a comment

Choose a reason for hiding this comment

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

Seems good to me. Tested it on my project and only noticed the different log message format. Apart from that all works well.

Next step would be to replace the data collection part for the profiler then? Like DebugStack and BacktraceLogger?

@ostrolucky
Copy link
Member

fair enough, let's merge it then

@ostrolucky ostrolucky merged commit d688e91 into doctrine:2.6.x Jan 5, 2022
@dmaicher dmaicher mentioned this pull request Jan 10, 2022
Closed
@l-vo l-vo deleted the allow_to_use_logging_middleware branch January 28, 2022 21:03
@l-vo l-vo mentioned this pull request Feb 20, 2022
Merged
This was referenced Mar 2, 2022
Closed
Merged
fabpot added a commit to symfony/symfony that referenced this pull request Mar 25, 2022
@fabpot
bug #45491 [DoctrineBridge] Allow to use a middleware instead of Dbal…
1b2c9b5 
…Logger (l-vo)

This PR was merged into the 5.4 branch.

Discussion
----------

[DoctrineBridge] Allow to use a middleware instead of DbalLogger

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       |  Step toward the fix of doctrine/DoctrineBundle#1431  (mentioned too in #44313 and #44495)
| License       | MIT
| Doc PR        |

The SqlLogger that is used in doctrine bridge and doctrine bundle has been deprecated and replaced by a system of Middleware.

A work has started on Doctrine bundle with doctrine/DoctrineBundle#1456 and doctrine/DoctrineBundle#1472

This PR suggest to add a middleware thats covers what was previously done by `DbalLogger` and `DebugStack`.

Another PR will follow in DoctrineBundle for the integration.

Commits
-------

20d0806 Allow to use a middleware instead of DbalLogger
@77web 77web mentioned this pull request Mar 30, 2022
Closed
@r-martins r-martins mentioned this pull request Mar 31, 2022
Closed
@r-martins
Copy link

r-martins commented Mar 31, 2022 •
edited
Loading

It does not really fix the deprecation warnings, right? It just fixes if we add a middleware class. Am I right?
If yes, how do we do that?

@l-vo
Copy link
Contributor Author

l-vo commented Mar 31, 2022

@r-martins Yes, this is a step to fix them since it involves changes on Symfony too. The changes on Symfony are now merged but a final step is still missing in DoctrineBundle. I'm working on it.

@greg0ire greg0ire mentioned this pull request Apr 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmaicher dmaicher dmaicher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.6.0
Development

Successfully merging this pull request may close these issues.

Wire a middleware for logging
5 participants
@l-vo @stof @ostrolucky @r-martins @dmaicher