Allow userland middlewares

[l-vo]

Related to the needs to replace SQLLogger by Middlewares, this implementations allows to easily inject middleware via a doctrine.middleware tag.

[greg0ire]

You can run vendor/bin/phpcbf to fix most CS issues :)

[greg0ire]

What's the use case for this?

[l-vo]

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];
    }
}

[l-vo]

You can run vendor/bin/phpcbf to fix most CS issues :)

Thank you, I missed that cs failed

[ostrolucky]

Why was this moved out of loadMiddlewares?

[l-vo]

I moved nothing out of loadMiddlewares, loadMiddleware has just been added in this PR. What am I missing ?

[ostrolucky]

useMiddlewaresIfAvailable was renamed to loadMiddlewares. This condition was there.

[l-vo]

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.

[ostrolucky]

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.

[l-vo]

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).

[ostrolucky]

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

[l-vo]

It disables logging but the logging is now done by the logging middleware, see Resources/config/middlewares.xml in this PR.

[ostrolucky]

that's just abstract middleware

[l-vo]

Yes but abstract middlewares are registered for each connection by MiddlewarePass 🙂

[ostrolucky]

I see. And which part disables middleware when $config['logging'] is false?

[l-vo]

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

[ostrolucky]

Nice work, I think there are only minor comments

[l-vo]

@ostrolucky thank you, all comments are addressed.

[ostrolucky]

LGTM, but would be nice if community properly tested this. I didn't try it in a project

[greg0ire]

@l-vo sorry to comment about this more than 1 year later, is there documentation on this somewhere?

[l-vo]

@greg0ire yes you're right, I forgot to document the configuration. I'm going to do that soon.

[greg0ire]

Great! Thanks in advance!

[l-vo]

@greg0ire done in #1653 :)