Implement parameter conversion and named parameters

[MAILPOET-6142]

mailpoet/lib/Doctrine/WPDB/ConvertParameters.php

@@ -0,0 +1,84 @@
+<?php declare (strict_types = 1);
+
+namespace MailPoet\Doctrine\WPDB;
+
+use MailPoet\Doctrine\WPDB\Exceptions\MissingParameterException;
+use MailPoetVendor\Doctrine\DBAL\ParameterType;
+use MailPoetVendor\Doctrine\DBAL\SQL\Parser\Visitor;
+
+class ConvertParameters implements Visitor {
+  private const PARAM_TYPE_MAP = [
+    ParameterType::STRING => '%s',
+    ParameterType::INTEGER => '%d',
+    ParameterType::ASCII => '%s',
+    ParameterType::BINARY => '%s',
+    ParameterType::BOOLEAN => '%d',
+    ParameterType::NULL => '%s',
+    ParameterType::LARGE_OBJECT => '%s',
+  ];
+
+  /** @var list<string> */
+  private array $buffer = [];
+
+  /** @var array<array-key, array{0: string, 1: mixed, 2: int}> */
+  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';
+  }
+}

mailpoet/lib/Doctrine/WPDB/Exceptions/MissingParameterException.php

@@ -0,0 +1,8 @@
+<?php declare(strict_types = 1);
+
+namespace MailPoet\Doctrine\WPDB\Exceptions;
+
+use Exception;
+
+class MissingParameterException extends Exception {
+}

mailpoet/lib/Doctrine/WPDB/Statement.php

@@ -6,9 +6,11 @@ use MailPoet\Doctrine\WPDB\Exceptions\NotSupportedException;
 use MailPoetVendor\Doctrine\DBAL\Driver\Result;
 use MailPoetVendor\Doctrine\DBAL\Driver\Statement as StatementInterface;
 use MailPoetVendor\Doctrine\DBAL\ParameterType;
+use MailPoetVendor\Doctrine\DBAL\SQL\Parser;
 
 class Statement implements StatementInterface {
   private Connection $connection;
+  private Parser $parser;
   private string $sql;
   private array $params = [];
 
@@ -17,6 +19,7 @@ class Statement implements StatementInterface {
     string $sql
   ) {
     $this->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).
+    // Convert '?' parameters to WPDB format (sprintf-like: '%s', '%d', ...),
-    // Note that this doesn't parse the SQL query properly and doesn't support named parameters.
+    // and add support for named parameters that are not supported by mysqli.
-    $sql = $this->sql;
+    $visitor = new ConvertParameters($this->params);
-    $values = [];
+    $this->parser->parse($this->sql, $visitor);
-    foreach ($this->params as [$param, $value, $type]) {
+    $sql = $visitor->getSQL();
-      $replacement = $type === ParameterType::INTEGER || ParameterType::BOOLEAN ? '%d' : '%s';
+    $values = $visitor->getValues();
-      $pos = strpos($this->sql, '?');
-      $sql = substr_replace($this->sql, $replacement, $pos, 1);
-      $values[$param] = $value;
-    }
 
     global $wpdb;
     $query = count($values) > 0 ? $wpdb->prepare($sql, $values) : $sql;

mailpoet/tests/unit/Doctrine/WPDB/ConvertParametersTest.php

@@ -0,0 +1,173 @@
+<?php declare(strict_types = 1);
+
+namespace MailPoet\Doctrine\WPDB;
+
+use DateTimeImmutable;
+use DateTimeZone;
+use MailPoet\Doctrine\WPDB\Exceptions\MissingParameterException;
+use MailPoetUnitTest;
+use MailPoetVendor\Doctrine\DBAL\ParameterType;
+use stdClass;
+
+class ConvertParametersTest extends MailPoetUnitTest {
+  public function testPositionalParameters(): void {
+    $params = new ConvertParameters([
+      1 => [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());
+  }
+}

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,
+    ];
+  }
 }