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

ProfilerLogger: Fix errors in DBAL 3.x #69

Merged
merged 9 commits into from
May 9, 2022
Merged

Conversation

Rixafy
Copy link
Contributor

@Rixafy Rixafy commented Apr 29, 2022

When persisting and flushing entity, there was an error

Doctrine\DBAL\ArrayParameters\Exception\MissingPositionalParameter: Positional parameter at index 0 does not have a bound value. in /vendor/doctrine/dbal/src/ArrayParameters/Exception/MissingPositionalParameter.php:19
Stack trace:
#0 /vendor/doctrine/dbal/src/ExpandArrayParameters.php(51): Doctrine\DBAL\ArrayParameters\Exception\MissingPositionalParameter::new(0)
#1 /vendor/doctrine/dbal/src/SQL/Parser.php(89): Doctrine\DBAL\ExpandArrayParameters->acceptPositionalParameter('...')
#2 /vendor/doctrine/dbal/src/SQL/Parser.php(103): Doctrine\DBAL\SQL\Parser::Doctrine\DBAL\SQL\{closure}('...')
#3 /vendor/nettrine/dbal/src/Logger/ProfilerLogger.php(90): Doctrine\DBAL\SQL\Parser->parse('...', Object(Doctrine\DBAL\ExpandArrayParameters))
#4 /vendor/nettrine/dbal/src/Logger/ProfilerLogger.php(49): Nettrine\DBAL\Logger\ProfilerLogger->expandListParameters('...', Array, Array)
#5 /vendor/doctrine/dbal/src/Logging/LoggerChain.php(37): Nettrine\DBAL\Logger\ProfilerLogger->startQuery('...', Array, Array)
#6 /vendor/doctrine/dbal/src/Statement.php(175): Doctrine\DBAL\Logging\LoggerChain->startQuery('...', Array, Array)
#7 /vendor/doctrine/dbal/src/Statement.php(221): Doctrine\DBAL\Statement->execute(NULL)
#8 /vendor/doctrine/orm/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php(278): Doctrine\DBAL\Statement->executeStatement()
#9 /vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php(1129): Doctrine\ORM\Persisters\Entity\BasicEntityPersister->executeInserts()
#10 /vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php(426): Doctrine\ORM\UnitOfWork->executeInserts(Object(Doctrine\ORM\Mapping\ClassMetadata))
#11 /vendor/doctrine/orm/lib/Doctrine/ORM/EntityManager.php(398): Doctrine\ORM\UnitOfWork->commit(NULL)
#12 /vendor/doctrine/orm/lib/Doctrine/ORM/Decorator/EntityManagerDecorator.php(211): Doctrine\ORM\EntityManager->flush(NULL)

So I found that in ExpandArrayParameters::acceptPositionalParameter was $index = 0 and first index of $this->originalParameters started always from 1, not 0

    public function acceptPositionalParameter(string $sql): void
    {
        $index = $this->originalParameterIndex;
		
        if (! array_key_exists($index, $this->originalParameters)) {
            throw MissingPositionalParameter::new($index);
        }

        $this->acceptParameter($index, $this->originalParameters[$index]);

        $this->originalParameterIndex++;
    }

Using array_values should resolve the problem, I have tested it on PHP 8.1 and it works just fine.
Also, after resolving the problem there appeared a new one, null value was passed to non-null argument, so I have added a nullcheck.

@Rixafy
Copy link
Contributor Author

Rixafy commented May 1, 2022

Do not merge, it doesn't work with named arguments, I don't want to make some dirty-fix to check if it's string or integer, and what do to with a case when there are mixed keys, it needs someone who know what is ProfileLogger for and why does parameters need to be filtered.

@Rixafy
Copy link
Contributor Author

Rixafy commented May 1, 2022

So I have checked how SQLParserUtils did it, and I found that there was is_int(key($params)) and it that doesn't result in true, parameters from ExpandArrayParameters were returned in original state.

Only problem with that is, if there will be mixed parameters (mixed = named with positional - ? and :name) and positional will start from 1 and not 0, I have MariaDB and that database doesn't support mixed parameters so I would get error anyway, but maybe proper fix would be looping through $params and checking if there is index 1 and not 0, then decrease all integer indexes by 1, but that would be too much code, especially when I don't even know what is ProfilerLogger for and what exactly is expandListParameters doing, my guess is that it will fill missing $types/$params to equal array size.

@xificurk
Copy link
Contributor

xificurk commented May 2, 2022

Look at e.g. Connection::executeStatement(), there is a conditional parameters expansion:

if ($this->needsArrayParameterConversion($params, $types)) {
    $sql, $params, $types] = $this->expandArrayParameters($sql, $params, $types);
}

I guess the key part that's missing in ProfilerLogger is that with DBAL 3, it should expand the parameters only if the conditions of needsArrayParameterConversion() are met.

@Rixafy
Copy link
Contributor Author

Rixafy commented May 2, 2022

I think it's ready to merge

@xificurk
Copy link
Contributor

xificurk commented May 2, 2022

@Rixafy would you add a test case for this to make sure this does not break in future DBAL version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants