Fix wrong type assertion and annotation#3543
Fix wrong type assertion and annotation#3543nicolas-grekas wants to merge 2 commits intodoctrine:masterfrom nicolas-grekas:assert-pdo
Conversation
|
Hmm, does phpstan not like that? |
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
| * @param string $cacheKey | ||
| * @param string $realKey | ||
| * @param int $lifetime | ||
| * @param ResultStatement|PDOStatement $stmt |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
This should be removed and the type hint added back, isn't it?
There was a problem hiding this comment.
I agree here, it looks like we're missing an assert on the caller and not here.
|
I've fixed the PHPStan failures that occurred due to allowing PDO classes in |
| * @param int $lifetime | ||
| */ | ||
| public function __construct(ResultStatement $stmt, Cache $resultCache, $cacheKey, $realKey, $lifetime) | ||
| public function __construct($stmt, Cache $resultCache, $cacheKey, $realKey, $lifetime) |
There was a problem hiding this comment.
Even when passing the PDO instance we modify the statement class, so I'd say we shouldn't remove this type hint here.
There was a problem hiding this comment.
PHPStan was complaining about it, that's why I had to modify it.
| * @param string $cacheKey | ||
| * @param string $realKey | ||
| * @param int $lifetime | ||
| * @param ResultStatement|PDOStatement $stmt |
There was a problem hiding this comment.
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:'); |
There was a problem hiding this comment.
Perhaps we need more tests covering the basic DBAL features for when an instance is used?
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( |
There was a problem hiding this comment.
| $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.
| * Executes an SQL statement, returning a result set as a Statement object. | ||
| * | ||
| * @return \Doctrine\DBAL\Driver\Statement | ||
| * @return DriverStatement|PDOStatement |
There was a problem hiding this comment.
The Interface containing query() only expects a return type of Doctrine\DBAL\Driver\Statement. Should PDOStatement be added to the interface too?
|
Closing as no longer relevant. |
This issue introduced in #3443 breaks Symfony's test suite.