Changing the position of the emptied variable

[dfrnks]

When use fetch or fetchColumn and don't pass for all registers if while they don't close the cursor and don't save in the cache

Q	A
Type	bug
BC Break	no
Fixed issues

Summary

[greg0ire]

When use fetch or fetchColumn and don't pass for all registers if while they don't close the cursor and don't save in the cache

I do not understand. Can you please elaborate?

[dfrnks]

When use fetch or fetchColumn and don't pass for all registers if while they don't close the cursor and don't save in the cache

I do not understand. Can you please elaborate?

When this query is made

    $qb = $db
        ->createQueryBuilder()
        ->select('*')
        ->from("base_config");

    $stmt   = $db->executeCacheQuery($qb, [], [], new QueryCacheProfile(3600));
    $config = $stmt->fetch();
    $stmt->closeCursor();

It is not saved in the cache, because it has not been passed in all registers and the variable $emptied in lib/Doctrine/DBAL/Cache/ResultCacheStatement.php is never true when we call closeCursor.

It will only be true if you pass all registers with while for example and then will save in the cache when we call closeCursor.

But is not the case of this query, this query only need the first register. And otherwise it will not influence if someone needs to pass all the registers, it will save in the cache too.

[greg0ire]

Pardon my ignorance but… what is a register? What are you referring to?

[dfrnks]

Pardon my ignorance but… what is a register? What are you referring to?

Sorry, I mean, without this pull request you need to do like this for save the cache using the fetch method

    while ($row = $stmt->fetch()) {
        $data[] = $row;
    }
   $stmt->closeCursor();

You need get all rows.

If you get just the first $row, they don't save the cache.

    $row = $stmt->fetch();
    $stmt->closeCursor();

And I think don't matter if you get one row or all rows to save the cache, it necessary save in the cache always.

[SenseException]

👍

@greg0ire I haven't checked all tests in the repository, but is it possible that we don't have a test that checks the cache behaviour with parameters?

[SenseException]

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

[dfrnks]

Not really, a second query he took from the cache

[SenseException]

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.

[dfrnks]

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

[SenseException]

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.

[morozov]

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

[greg0ire]

@SenseException what do you mean by "with parameters"?

[SenseException]

@greg0ire Every Connection::executeQuery() with a QueryCacheProfile has an empty array for $params. Depending on an omitted key in QueryCacheProfile the $params are also used for the cache key creation and I wonder if this caching is covered by an integration test. This has nothing to do with this PR, but maybe needs to be covered by another PR. Or I haven't found an existing test covering this case yet.

[morozov]

This is likely resolved by #4048.

[morozov]

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

[morozov]

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.

[dfrnks]

With I understanding, my pull request isn't necessary anymore because they are resolved the other way in #4048 right? @morozov

[morozov]

@dfrnks I believe so. You can check your case using the 2.11.x branch.