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

Fix wrong type assertion and annotation #3543

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
6 participants
@nicolas-grekas
Copy link
Member

commented May 9, 2019

Q A
Type bug
BC Break no
Fixed issues -

This issue introduced in #3443 breaks Symfony's test suite.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:assert-pdo branch from 24f167a to 72291c3 May 9, 2019

@jwage

This comment has been minimized.

Copy link
Member

commented May 9, 2019

@morozov and I were just talking about this issue in #3487 and how we plan to improve this in 3.0. This is good for master.

@jwage jwage added the BC Fix label May 9, 2019

@jwage

jwage approved these changes May 9, 2019

Show resolved Hide resolved lib/Doctrine/DBAL/Connection.php Outdated

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:assert-pdo branch from 72291c3 to cb0e83b May 9, 2019

@jwage

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Hmm, does phpstan not like that?

nicolas-grekas added a commit to symfony/symfony that referenced this pull request May 9, 2019

minor #31443 [Doctrine\Bridge] fix tests (nicolas-grekas)
This PR was merged into the 4.3 branch.

Discussion
----------

[Doctrine\Bridge] fix tests

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Once merged, one issue will remain, which will be fixed by doctrine/dbal#3543

Commits
-------

10da231 [Doctrine\Bridge] fix tests

@alcaeus alcaeus force-pushed the nicolas-grekas:assert-pdo branch from cb0e83b to 89f6987 May 10, 2019

* @param string $cacheKey
* @param string $realKey
* @param int $lifetime
* @param ResultStatement|PDOStatement $stmt

This comment has been minimized.

Copy link
@alcaeus

alcaeus May 10, 2019

Member

I wanted to add an assert statement to the constructor to check for this, but apparently PHPStan thinks it's redundant. The code in question would be

assert($stmt instanceof ResultStatement || $stmt instanceof PDOStatement);

Do we want to add it and silence the error or leave it as is?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 10, 2019

Author Member

This should be removed and the type hint added back, isn't it?

This comment has been minimized.

Copy link
@lcobucci

lcobucci May 10, 2019

Member

I agree here, it looks like we're missing an assert on the caller and not here.

nicolas-grekas and others added some commits May 9, 2019

@alcaeus alcaeus requested review from jwage, morozov and lcobucci May 10, 2019

@alcaeus

This comment has been minimized.

Copy link
Member

commented May 10, 2019

I've fixed the PHPStan failures that occurred due to allowing PDO classes in Connection. Unfortunately that means adding a bunch of assert calls as for example fetchAll will happily return false depending on the error handling mechanism. I don't think it's feasible to check for false and throw exceptions; at least not in the context of this PR.

*/
public function __construct(ResultStatement $stmt, Cache $resultCache, $cacheKey, $realKey, $lifetime)
public function __construct($stmt, Cache $resultCache, $cacheKey, $realKey, $lifetime)

This comment has been minimized.

Copy link
@lcobucci

lcobucci May 10, 2019

Member

Even when passing the PDO instance we modify the statement class, so I'd say we shouldn't remove this type hint here.

This comment has been minimized.

Copy link
@alcaeus

alcaeus May 10, 2019

Member

PHPStan was complaining about it, that's why I had to modify it.

@jwage

jwage approved these changes May 10, 2019

@lcobucci
Copy link
Member

left a comment

I feel like we're doing unnecessary things here and that we should try an alternative approach (wrapping). I'm working on it atm and we can compare the two PRs...

* @param string $cacheKey
* @param string $realKey
* @param int $lifetime
* @param ResultStatement|PDOStatement $stmt

This comment has been minimized.

Copy link
@lcobucci

lcobucci May 10, 2019

Member

I agree here, it looks like we're missing an assert on the caller and not here.

$conn = DriverManager::getConnection([
'pdo' => new PDO('sqlite::memory:'),
]);
$pdo = new PDO('sqlite::memory:');

This comment has been minimized.

Copy link
@lcobucci

lcobucci May 10, 2019

Member

Perhaps we need more tests covering the basic DBAL features for when an instance is used?

@lcobucci lcobucci referenced this pull request May 10, 2019

Closed

Wrap pdo instances #3544

Tobion added a commit to symfony/symfony that referenced this pull request May 11, 2019

minor #31473 [Messenger] Fix doctrine tests (weaverryan)
This PR was merged into the 4.3 branch.

Discussion
----------

[Messenger] Fix doctrine tests

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | none
| License       | MIT
| Doc PR        | not needed

Not sure why, but when passing in `pdo`, the Doctrine `Connection::_conn` property is a PDO instance and isn't wrapped. In the master branch of `doctrine/dbal`, they now check for this an throw an exception.

@nicolas-grekas has a PR in Doctrine for this (doctrine/dbal#3543), but I don't see any reason we shouldn't just avoid the `pdo` option entirely.

Commits
-------

a7cf3f9 Fixing tests - passing pdo is not wrapped for some reason in dbal
$this->_conn->quote($keyName)
));
$indexArray = $stmt->fetchAll(FetchMode::ASSOCIATIVE);
$stmt = $this->_conn->executeQuery(sprintf(

This comment has been minimized.

Copy link
@SenseException

SenseException May 13, 2019

Contributor
Suggested change
$stmt = $this->_conn->executeQuery(sprintf(
$stmt = $this->_conn->executeQuery(sprintf(

Because of the newline above this, I assume that the spaces aren't needed here.

@@ -986,7 +994,7 @@ public function project($query, array $params, Closure $function)
/**
* Executes an SQL statement, returning a result set as a Statement object.
*
* @return \Doctrine\DBAL\Driver\Statement
* @return DriverStatement|PDOStatement

This comment has been minimized.

Copy link
@SenseException

SenseException May 13, 2019

Contributor

The Interface containing query() only expects a return type of Doctrine\DBAL\Driver\Statement. Should PDOStatement be added to the interface too?

@morozov

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Closing as no longer relevant.

@morozov morozov closed this May 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.