From c42c37e8b19b4ad334d8e2b9b1b4188d63df2c0b Mon Sep 17 00:00:00 2001 From: Jan Jakes Date: Sat, 13 Jul 2024 14:15:26 +0200 Subject: [PATCH] Implement parameter conversion and named parameters [MAILPOET-6142] --- .../lib/Doctrine/WPDB/ConvertParameters.php | 84 +++++++++ .../Exceptions/MissingParameterException.php | 8 + mailpoet/lib/Doctrine/WPDB/Statement.php | 19 +- .../Doctrine/WPDB/ConvertParametersTest.php | 173 ++++++++++++++++++ .../unit/Doctrine/WPDB/StatementTest.php | 57 ++++++ 5 files changed, 331 insertions(+), 10 deletions(-) create mode 100644 mailpoet/lib/Doctrine/WPDB/ConvertParameters.php create mode 100644 mailpoet/lib/Doctrine/WPDB/Exceptions/MissingParameterException.php create mode 100644 mailpoet/tests/unit/Doctrine/WPDB/ConvertParametersTest.php diff --git a/mailpoet/lib/Doctrine/WPDB/ConvertParameters.php b/mailpoet/lib/Doctrine/WPDB/ConvertParameters.php new file mode 100644 index 0000000000..6f936a311f --- /dev/null +++ b/mailpoet/lib/Doctrine/WPDB/ConvertParameters.php @@ -0,0 +1,84 @@ + '%s', + ParameterType::INTEGER => '%d', + ParameterType::ASCII => '%s', + ParameterType::BINARY => '%s', + ParameterType::BOOLEAN => '%d', + ParameterType::NULL => '%s', + ParameterType::LARGE_OBJECT => '%s', + ]; + + /** @var list */ + private array $buffer = []; + + /** @var array */ + private array $params; + + private array $values = []; + + private int $cursor = 1; + + public function __construct( + array $params + ) { + $this->params = $params; + } + + public function acceptPositionalParameter(string $sql): void { + $position = $this->cursor++; + $this->acceptParameter($position); + } + + public function acceptNamedParameter(string $sql): void { + $this->acceptParameter(trim($sql, ':')); + } + + public function acceptOther(string $sql): void { + $this->buffer[] = $sql; + } + + public function getSQL(): string { + return implode('', $this->buffer); + } + + public function getValues(): array { + return $this->values; + } + + /** @param array-key $key */ + private function acceptParameter($key): void { + if (!array_key_exists($key, $this->params)) { + throw new MissingParameterException(sprintf("Parameter '%s' was defined in the query, but not provided.", $key)); + } + [, $value, $type] = $this->params[$key]; + + // WPDB doesn't support NULL values. We need to handle them explicitly. + if ($value === null) { + $this->buffer[] = 'NULL'; + return; + } + + // WPDB doesn't accept non-scalar values. We need to cast them (PDO-like behavior). + if (!is_scalar($value)) { + if ($type === ParameterType::INTEGER) { + $value = (int)$value; // @phpstan-ignore-line -- cast may fail and that's OK + } elseif ($type === ParameterType::BOOLEAN) { + $value = (bool)$value; + } else { + $value = (string)$value; // @phpstan-ignore-line -- cast may fail and that's OK + } + } + + $this->values[] = $value; + $this->buffer[] = self::PARAM_TYPE_MAP[$type] ?? '%s'; + } +} diff --git a/mailpoet/lib/Doctrine/WPDB/Exceptions/MissingParameterException.php b/mailpoet/lib/Doctrine/WPDB/Exceptions/MissingParameterException.php new file mode 100644 index 0000000000..6297e2de62 --- /dev/null +++ b/mailpoet/lib/Doctrine/WPDB/Exceptions/MissingParameterException.php @@ -0,0 +1,8 @@ +connection = $connection; + $this->parser = new Parser(false); $this->sql = $sql; } @@ -51,16 +54,12 @@ class Statement implements StatementInterface { ); } - // Convert "?" placeholders to sprintf-like format expected by WPDB (basic implementation). - // Note that this doesn't parse the SQL query properly and doesn't support named parameters. - $sql = $this->sql; - $values = []; - foreach ($this->params as [$param, $value, $type]) { - $replacement = $type === ParameterType::INTEGER || ParameterType::BOOLEAN ? '%d' : '%s'; - $pos = strpos($this->sql, '?'); - $sql = substr_replace($this->sql, $replacement, $pos, 1); - $values[$param] = $value; - } + // Convert '?' parameters to WPDB format (sprintf-like: '%s', '%d', ...), + // and add support for named parameters that are not supported by mysqli. + $visitor = new ConvertParameters($this->params); + $this->parser->parse($this->sql, $visitor); + $sql = $visitor->getSQL(); + $values = $visitor->getValues(); global $wpdb; $query = count($values) > 0 ? $wpdb->prepare($sql, $values) : $sql; diff --git a/mailpoet/tests/unit/Doctrine/WPDB/ConvertParametersTest.php b/mailpoet/tests/unit/Doctrine/WPDB/ConvertParametersTest.php new file mode 100644 index 0000000000..bea5e2c287 --- /dev/null +++ b/mailpoet/tests/unit/Doctrine/WPDB/ConvertParametersTest.php @@ -0,0 +1,173 @@ + [1, 123, ParameterType::INTEGER], + 2 => [2, 'aaa', ParameterType::STRING], + 3 => [3, true, ParameterType::BOOLEAN], + ]); + + $params->acceptOther('SELECT * FROM test_table WHERE id = '); + $params->acceptPositionalParameter('?'); + $params->acceptOther(' AND value = '); + $params->acceptPositionalParameter('?'); + $params->acceptOther(' AND isDeleted = '); + $params->acceptPositionalParameter('?'); + + $this->assertSame( + 'SELECT * FROM test_table WHERE id = %d AND value = %s AND isDeleted = %d', + $params->getSQL() + ); + $this->assertSame([123, 'aaa', true], $params->getValues()); + } + + public function testNamedParameters(): void { + $params = new ConvertParameters([ + 'id' => ['id', 123, ParameterType::INTEGER], + 'value' => ['value', 'aaa', ParameterType::STRING], + 'isDeleted' => ['isDeleted', true, ParameterType::BOOLEAN], + ]); + + $params->acceptOther('SELECT * FROM test_table WHERE id = '); + $params->acceptNamedParameter(':id'); + $params->acceptOther(' AND value = '); + $params->acceptNamedParameter(':value'); + $params->acceptOther(' AND isDeleted = '); + $params->acceptNamedParameter(':isDeleted'); + + $this->assertSame( + 'SELECT * FROM test_table WHERE id = %d AND value = %s AND isDeleted = %d', + $params->getSQL() + ); + $this->assertSame([123, 'aaa', true], $params->getValues()); + } + + public function testRepeatedNamedParameters(): void { + $params = new ConvertParameters([ + 'value' => ['value', 'aaa', ParameterType::STRING], + ]); + + $params->acceptOther('SELECT * FROM test_table WHERE value1 = '); + $params->acceptNamedParameter(':value'); + $params->acceptOther(' AND value2 = '); + $params->acceptNamedParameter(':value'); + + $this->assertSame( + 'SELECT * FROM test_table WHERE value1 = %s AND value2 = %s', + $params->getSQL() + ); + $this->assertSame(['aaa', 'aaa'], $params->getValues()); + } + + public function testMixedParameters(): void { + $params = new ConvertParameters([ + 1 => [1, 123, ParameterType::INTEGER], + 2 => [2, 'aaa', ParameterType::STRING], + 3 => [3, true, ParameterType::BOOLEAN], + 'named1' => ['named1', 'bbb', ParameterType::STRING], + 'named2' => ['named2', 'ccc', ParameterType::ASCII], + ]); + + $params->acceptOther('SELECT * FROM test_table WHERE id = '); + $params->acceptPositionalParameter('?'); + $params->acceptOther(' AND named1 = '); + $params->acceptNamedParameter(':named1'); + $params->acceptOther(' AND value = '); + $params->acceptPositionalParameter('?'); + $params->acceptOther(' AND named2 = '); + $params->acceptNamedParameter(':named2'); + $params->acceptOther(' AND isDeleted = '); + $params->acceptPositionalParameter('?'); + + $this->assertSame( + 'SELECT * FROM test_table WHERE id = %d AND named1 = %s AND value = %s AND named2 = %s AND isDeleted = %d', + $params->getSQL() + ); + $this->assertSame([123, 'bbb', 'aaa', 'ccc', true], $params->getValues()); + } + + public function testMissingPositionalParameter(): void { + $params = new ConvertParameters([ + 1 => [1, 123, ParameterType::INTEGER], + ]); + + $params->acceptOther('SELECT * FROM test_table WHERE id = '); + $params->acceptPositionalParameter('?'); + $params->acceptOther(' AND value = '); + + $this->expectException(MissingParameterException::class); + $this->expectExceptionMessage("Parameter '2' was defined in the query, but not provided."); + + $params->acceptPositionalParameter('?'); + } + + public function testMissingNamedParameter(): void { + $params = new ConvertParameters([ + 'id' => ['id', 123, ParameterType::INTEGER], + ]); + + $params->acceptOther('SELECT * FROM test_table WHERE id = '); + $params->acceptNamedParameter(':id'); + $params->acceptOther(' AND value = '); + + $this->expectException(MissingParameterException::class); + $this->expectExceptionMessage("Parameter 'value' was defined in the query, but not provided."); + + $params->acceptNamedParameter(':value'); + } + + public function testNullValues(): void { + $params = new ConvertParameters([ + 1 => [1, null, ParameterType::STRING], + 'named' => ['named', null, ParameterType::INTEGER], + ]); + + $params->acceptOther('SELECT * FROM test_table WHERE id = '); + $params->acceptPositionalParameter('?'); + $params->acceptOther(' AND value = '); + $params->acceptNamedParameter(':named'); + + $this->assertSame( + 'SELECT * FROM test_table WHERE id = NULL AND value = NULL', + $params->getSQL() + ); + $this->assertSame([], $params->getValues()); + } + + public function testNonScalarValues(): void { + $dateTime = new class('2021-01-01 12:34:56', new DateTimeZone('UTC')) extends DateTimeImmutable { + public function __toString(): string { + return $this->format(DateTimeImmutable::W3C); + } + }; + + $params = new ConvertParameters([ + 1 => [1, $dateTime, ParameterType::STRING], + 2 => [2, new stdClass(), ParameterType::BOOLEAN], + 3 => [3, ['abc'], ParameterType::INTEGER], + ]); + + $params->acceptOther('SELECT * FROM test_table WHERE datetime = '); + $params->acceptPositionalParameter('?'); + $params->acceptOther(' AND boolean = '); + $params->acceptPositionalParameter('?'); + $params->acceptOther(' AND integer = '); + $params->acceptPositionalParameter('?'); + + $this->assertSame( + 'SELECT * FROM test_table WHERE datetime = %s AND boolean = %d AND integer = %d', + $params->getSQL() + ); + $this->assertSame(['2021-01-01T12:34:56+00:00', true, 1], $params->getValues()); + } +} diff --git a/mailpoet/tests/unit/Doctrine/WPDB/StatementTest.php b/mailpoet/tests/unit/Doctrine/WPDB/StatementTest.php index 1d935dd8e4..ac3ab716e9 100644 --- a/mailpoet/tests/unit/Doctrine/WPDB/StatementTest.php +++ b/mailpoet/tests/unit/Doctrine/WPDB/StatementTest.php @@ -63,4 +63,61 @@ class StatementTest extends MailPoetUnitTest { $statement->bindValue(1, 'abc'); $statement->execute(); } + + /** + * @dataProvider parameterReplacementProvider + */ + public function testParameterReplacement(string $inputSql, string $outputSql, int $parameterCount): void { + $wpdb = $this->getMockBuilder(stdClass::class)->addMethods(['prepare'])->getMock(); + $wpdb->expects($this->once()) + ->method('prepare') + ->with($outputSql) + ->willReturn(''); + + $GLOBALS['wpdb'] = $wpdb; + $connection = $this->createMock(Connection::class); + $statement = new Statement($connection, $inputSql); + for ($i = 1; $i <= $parameterCount; $i++) { + $statement->bindValue($i, 'abc'); + } + $statement->execute(); + } + + public function parameterReplacementProvider(): iterable { + yield 'simple' => [ + 'SELECT * FROM test_table WHERE value = ?', + 'SELECT * FROM test_table WHERE value = %s', + 1, + ]; + + yield 'with ? in string' => [ + "SELECT * FROM test_table WHERE value = ? AND name = 'a?c'", + "SELECT * FROM test_table WHERE value = %s AND name = 'a?c'", + 1, + ]; + + yield 'with ? in string and multiple parameters' => [ + "SELECT * FROM test_table WHERE value = ? AND name = 'a?c' AND id = ?", + "SELECT * FROM test_table WHERE value = %s AND name = 'a?c' AND id = %s", + 2, + ]; + + yield 'with JOIN' => [ + 'SELECT * FROM test_table JOIN other_table ON test_table.id = other_table.id WHERE value = ?', + 'SELECT * FROM test_table JOIN other_table ON test_table.id = other_table.id WHERE value = %s', + 1, + ]; + + yield 'with subquery' => [ + "SELECT * FROM test_table WHERE value = ? AND name = (SELECT name FROM other_table WHERE id = ?)", + 'SELECT * FROM test_table WHERE value = %s AND name = (SELECT name FROM other_table WHERE id = %s)', + 2, + ]; + + yield 'complex' => [ + "SELECT CONCAT(key, '?') FROM test_table WHERE value = ? AND name = 'a?c' AND id = ? AND (SELECT name FROM other_table WHERE id = ?)", + "SELECT CONCAT(key, '?') FROM test_table WHERE value = %s AND name = 'a?c' AND id = %s AND (SELECT name FROM other_table WHERE id = %s)", + 3, + ]; + } }