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.

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

Provide abstract middleware classes #5035

Merged
merged 1 commit into from Nov 27, 2021

Conversation

derrabus
Copy link
Member

Q A
Type improvement
BC Break no
Fixed issues N/A

Summary

When writing a middleware, I have to start with a lot of delegating boilerplate code. As a DX improvement, I'd like to propose the introduction of abstract wrapper classes for driver, connection, statement and result. This allows a developer to only override the methods they want to hook into, resulting in smaller and focussed middleware classes.

I have already modified the portability and logging middlewares to use those abstract classes, so we can see the effect that those classes have.

Using those abstract classes of course remains optional. The contract is still defined by the interfaces, so a middleware can still be implemented without the abstract classes.

Although the new classes are already covered by the tests of the portability and logging middlewares, I intend to add dedicated tests for them, once we agree that we want to introduce those abstract classes.

@derrabus derrabus added this to the 3.3.0 milestone Nov 27, 2021
@derrabus derrabus closed this Nov 27, 2021
@derrabus derrabus reopened this Nov 27, 2021
@derrabus
Copy link
Member Author

AppVeyor is drunk.

Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

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

I'm all for it.

@derrabus derrabus force-pushed the improvement/abstract-middlewares branch from 09a087d to ba8ecfe Compare November 27, 2021 16:37
@derrabus derrabus merged commit 8d00257 into doctrine:3.3.x Nov 27, 2021
@derrabus derrabus deleted the improvement/abstract-middlewares branch November 27, 2021 16:46
@@ -47,7 +46,7 @@ public function bindParam($param, &$variable, $type = ParameterType::STRING, $le
$this->params[$param] = &$variable;
$this->types[$param] = $type;

return $this->statement->bindParam($param, $variable, $type, ...array_slice(func_get_args(), 3));
return parent::bindParam($param, $variable, $type, ...array_slice(func_get_args(), 3));
Copy link
Member

Choose a reason for hiding this comment

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

why this array_slice magic instead of passing $length to the parent method ? the parent method won't do the same magic to pass extra parameters (or omit the length) to the wrapped statement anyway.

abstract class AbstractDriverMiddleware implements VersionAwarePlatformDriver
{
/** @var Driver */
private $wrappedDriver;
Copy link
Contributor

@mvorisek mvorisek Jul 14, 2022

Choose a reason for hiding this comment

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

Shouldn't this property be protected or getter to be added?

Otherwise how are supposed the public/middleware methods to be overriden?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it shouldn't.

You'll likely need to override the constructor anyway, so you can create your own private property for the wrapped driver if you really need it. Delegating a call to the wrapped driver can be done my calling the parent method, for example:

public function connect(array $params)
{
$this->logger->info('Connecting with parameters {params}', ['params' => $this->maskPassword($params)]);
return new Connection(
parent::connect($params),
$this->logger
);
}

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants