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.

Sign up for GitHub

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

Changing the position of the emptied variable #3997

Closed
wants to merge 14 commits into from
+23 −4
Closed

Changing the position of the emptied variable #3997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4f51944
Changing the position of the emptied variable
dfrnks May 6, 2020
a711cce
correcting unit test and code correction
dfrnks May 6, 2020
4e54fce
Remove space
dfrnks May 6, 2020
90121e3
New unit test. test ArrayIterator
dfrnks May 7, 2020
6ebd9a1
New unit test. test ArrayIterator
dfrnks May 7, 2020
c7ba8bb
Change the unit test
dfrnks May 7, 2020
03937e3
CS fixed
dfrnks May 7, 2020
8f70206
/** @scrutinizer ignore-call */
dfrnks May 7, 2020
038b00d
Update tests/Doctrine/Tests/DBAL/Functional/ResultCacheTest.php
dfrnks May 8, 2020
039d351
Remove use ArrayIterator
dfrnks May 8, 2020
70ecd18
Change the return of executeCacheQuery
dfrnks May 8, 2020
40a452b
Fixed CS
dfrnks May 8, 2020
3cc7fe2
Add @throws DBALException
dfrnks May 8, 2020
0ca34b8
Remove @throws DBALException
dfrnks May 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/Doctrine/DBAL/Cache/ResultCacheStatement.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,8 @@ public function fetch($fetchMode = null, $cursorOrientation = PDO::FETCH_ORI_NEX

$row = $this->statement->fetch(FetchMode::ASSOCIATIVE);

$this->emptied = true;

if ($row) {
$this->data[] = $row;

Expand All @@ -158,8 +160,6 @@ public function fetch($fetchMode = null, $cursorOrientation = PDO::FETCH_ORI_NEX
throw new InvalidArgumentException('Invalid fetch-style given for caching result.');
}

$this->emptied = true;

return false;
}

Expand Down
3 changes: 2 additions & 1 deletion lib/Doctrine/DBAL/Connection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,9 +931,10 @@ public function executeQuery($query, array $params = [], $types = [], ?QueryCach
* @param int[]|string[] $types The types the previous parameters are in.
* @param QueryCacheProfile $qcp The query cache profile.
*
* @return ResultStatement
* @return ResultCacheStatement|ArrayStatement
Copy link
Member

Choose a reason for hiding this comment

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

Please revert. The type hint should refer to an interface, not to a specific implementation.

*
* @throws CacheException
* @throws DBALException
*/
public function executeCacheQuery($query, $params, $types, QueryCacheProfile $qcp)
{
Expand Down
20 changes: 19 additions & 1 deletion tests/Doctrine/Tests/DBAL/Functional/ResultCacheTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ public function testDontFinishNoCache() : void

$this->hydrateStmt($stmt, FetchMode::NUMERIC);

self::assertCount(2, $this->sqlLogger->queries);
self::assertCount(1, $this->sqlLogger->queries);
Copy link
Member

Choose a reason for hiding this comment

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

Wait, isn't this test supposed to have 2 queries executed?

Copy link
Author

Choose a reason for hiding this comment

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

Not really, a second query he took from the cache

Copy link
Member

Choose a reason for hiding this comment

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

It previously expected 2 queries in the test and I want to know what's the reason for this test. I'll need help from @beberlei, who implemented this test in #69 a long time ago.

Copy link
Author

Choose a reason for hiding this comment

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

The test was incorrect because, just will save the query in $this->sqlLogger->queries when the query are execute in the database.

dbal/lib/Doctrine/DBAL/Connection.php

Lines 894 to 897 in 4c03ed8

$logger = $this->_config->getSQLLogger();
if ($logger) {
$logger->startQuery($query, $params, $types);
}

When the query are in the cache, they return before

dbal/lib/Doctrine/DBAL/Connection.php

Lines 886 to 897 in 4c03ed8

public function executeQuery($query, array $params = [], $types = [], ?QueryCacheProfile $qcp = null)
{
if ($qcp !== null) {
return $this->executeCacheQuery($query, $params, $types, $qcp);
}
$connection = $this->getWrappedConnection();
$logger = $this->_config->getSQLLogger();
if ($logger) {
$logger->startQuery($query, $params, $types);
}

The reason are the cache doesn't work before, when we use fetch

Copy link
Member

Choose a reason for hiding this comment

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

I agree, which is why I approved if at first, but if there's another reason why @beberlei expected 2 in the past, I want to know and would like to be on the save side.

Copy link
Member

Choose a reason for hiding this comment

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

This was recently changed in #4048. It's an improvement, not a bugfix, so this change shouldn't be made in 2.10.x..

}

public function testFetchAllAndFinishSavesCache() : void
Expand Down Expand Up @@ -194,6 +194,24 @@ public function testFetchAllColumn() : void
self::assertEquals([1], $stmt->fetchAll(FetchMode::COLUMN));
}

public function testGetIterator() : void
{
$query = $this->connection->getDatabasePlatform()
->getDummySelectSQL('1');

$qcp = new QueryCacheProfile(0, 0, new ArrayCache());

$stmt = $this->connection->executeCacheQuery($query, [], [], $qcp);
$stmt->getIterator();
$stmt->closeCursor();

$stmt = $this->connection->executeCacheQuery($query, [], [], $qcp);

$iterator = $stmt->getIterator();

self::assertCount(1, $iterator);
Copy link
Member

Choose a reason for hiding this comment

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

This is not a valid behavior to expect. The fact that the iterator internally calls fetchAll() is a bug (see #2718) and shouldn't be relied upon to expect that the statement is cached just because an iterator over its results was instantiated.

}

/**
* @param array<int, array<int, int|string>> $expectedResult
*/
Expand Down