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

New assertion breaks QueryBuilder with dummy driver #4250

Closed
Bilge opened this issue Sep 3, 2020 · 7 comments
Closed

New assertion breaks QueryBuilder with dummy driver #4250

Bilge opened this issue Sep 3, 2020 · 7 comments

Comments

@Bilge
Copy link
Contributor

Bilge commented Sep 3, 2020

I use the Doctrine QueryBuilder with a foreign database library. Since the QueryBuilder does not work without a driver, I create a dummy driver so I can just use the QueryBuilder. This worked until a BC break in a minor revision that added an assertion that disagrees with the type hint on the backing field.

BC Break Report

Q A
BC Break yes
Version 2.10.3

Summary

When upgrading from 2.10.2. to 2.10.3 my code breaks due to this change: e48861e

assert($this->_conn !== null);

Previous behaviour

Previously $this->_conn was allowed to be null. The type hint for $_conn still reads as nullable, but for some reason this assertion was added that disagrees with this.

/**
* The wrapped driver connection.
*
* @var \Doctrine\DBAL\Driver\Connection|null
*/
protected $_conn;

Current behavior

Since the assertions forces $_conn to be non-null, it breaks cases where $_conn is null.

How to reproduce

<?php
declare(strict_types=1);

use Doctrine\DBAL\Driver\AbstractPostgreSQLDriver;

final class DummyPostgresDriver extends AbstractPostgreSQLDriver
{
    public function connect(array $params, $username = null, $password = null, array $driverOptions = []): void
    {
        // Intentionally empty.
    }

    public function getName(): string
    {
        return __CLASS__;
    }
}

$qb = (new Doctrine\DBAL\Connection([], new DummyPostgresDriver))->createQueryBuilder();
$qb->setMaxResults(50); // Only occurs when max results is defined.
$qb->getSql(); // This eventually calls Connection::getWrappedConnection(), triggering the assertion to fail.
@greg0ire
Copy link
Member

greg0ire commented Sep 3, 2020

Before the assertion was added, there was a contract for implementations though:

* @return DriverConnection

The contract is enforced now, any implementation should already be compliant. You can work this around by disabling assertions, or you could implement the contract by returning a dummy implementation of DriverConnection.

@Bilge
Copy link
Contributor Author

Bilge commented Sep 3, 2020

The class itself is already a DriverConnection. I could inject it into itself but that seems silly.

@greg0ire
Copy link
Member

greg0ire commented Sep 3, 2020

Doesn't having a dummy driver feel silly already? Anyway, I don't think we are going to revert this to humor this use case, sorry. Feels a bit too much like xkcd 1172

@Bilge
Copy link
Contributor Author

Bilge commented Sep 3, 2020

Doesn't having a QueryBuilder tied to an active connection feel silly? I'm trying to work around these artificial constraints.

@greg0ire
Copy link
Member

greg0ire commented Sep 3, 2020

It sure does. Good luck with your issue.

@greg0ire
Copy link
Member

greg0ire commented Sep 3, 2020

Closing as per #4094 (comment)

@greg0ire greg0ire closed this as completed Sep 3, 2020
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

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

No branches or pull requests

2 participants