-
-
Notifications
You must be signed in to change notification settings - Fork 452
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
Allow userland middlewares #1472
Conversation
b4a7efa
to
13a1ce7
Compare
13a1ce7
to
2771b57
Compare
You can run |
interface ConnectionNameAwareInterface | ||
{ | ||
public function setConnectionName(string $name): void; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use case for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this PR with the replacement of DebugStack
by middlewares in mind. In the symfony profiler, queries are grouped by connection. I faced a choice:
- either I inject in the middleware a different service (kind of debug data holder) by connection
- either the middleware is aware of the connection and set queries in an unique debug data holder tied with their connection
It's not a big deal to duplicate an abstract middleware for each connection with a tag, but duplicating too the debug data holder seemed to me more complicated; it's why I chose the second possibility.
This is how I plan to create the debug data holder:
class DebugDataHolder
{
private array $data = [];
public function addQuery(string $connexionName, Query $query): void
{
$this->data[$connexionName][] = [
'sql' => $query->getSql(),
'params' => $query->getParams(),
'types' => $query->getTypes(),
'executionMS' => static fn() => $query->getDuration(), // stop() may not be called at this point
];
}
public function getData(string $connexionName): array
{
if (!isset($this->data[$connexionName])) {
return [];
}
foreach ($this->data[$connexionName] as &$data) {
if (is_callable($data['executionMS'])) {
$data['executionMS'] = $data['executionMS']();
}
}
return $this->data[$connexionName];
}
}
2771b57
to
6bef524
Compare
Thank you, I missed that cs failed |
6bef524
to
bca0e58
Compare
if (interface_exists(Middleware::class)) { | ||
$container | ||
->getDefinition('doctrine.dbal.logger') | ||
->replaceArgument(0, null); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this moved out of loadMiddlewares
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved nothing out of loadMiddlewares
, loadMiddleware
has just been added in this PR. What am I missing ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useMiddlewaresIfAvailable was renamed to loadMiddlewares. This condition was there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sorry, it's a subjective choice, in my mind, loadMiddleware
should only register middlewares, not disable what is replaced by a middleware. But I can move it back in loadMiddleware
if you think it's better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Point is to guarantee logger argument is replaced only when middlewares are registered. After this split, nothing guarantees it anymore and one can call these separately. Perhaps old name was better then, since useMiddlewares
doesn't imply it only loads them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests guarantee that 🙂 But I understand the need from a code point of view, I moved it back into useMiddlewaresIfAvailable
(I renamed the method too).
if (interface_exists(Middleware::class)) { | ||
$container | ||
->getDefinition('doctrine.dbal.logger') | ||
->replaceArgument(0, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This disables logging even when it's enabled, right? Basically it will block release of 2.6 once we merge this, until you finish follow-up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It disables logging but the logging is now done by the logging middleware, see Resources/config/middlewares.xml
in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's just abstract middleware
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but abstract middlewares are registered for each connection by MiddlewarePass
🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. And which part disables middleware when $config['logging'] is false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I missed this case, to allow to handle this case, I did some other changes:
- Middleware can now be registered only for chosen connections (with a tag attribute)
- An attribute
AsMiddleware
has been added
bd84248
to
171f816
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, I think there are only minor comments
5bd9712
to
ed22040
Compare
@ostrolucky thank you, all comments are addressed. |
ed22040
to
9b60db7
Compare
LGTM, but would be nice if community properly tested this. I didn't try it in a project |
…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
@l-vo sorry to comment about this more than 1 year later, is there documentation on this somewhere? |
@greg0ire yes you're right, I forgot to document the configuration. I'm going to do that soon. |
Great! Thanks in advance! |
Related to the needs to replace SQLLogger by Middlewares, this implementations allows to easily inject middleware via a
doctrine.middleware
tag.