Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.The Connection class removes trailing semicolons from SQL Statements, but also supports a setting to allow semicolons in statements. For some reason, SQL Server requires that a MERGE statement end in a semicolon. A good universal approach would be to change:
else {
$this->expandArguments($query, $args);
// To protect against SQL injection, Drupal only supports executing one
// statement at a time. Thus, the presence of a SQL delimiter (the
// semicolon) is not allowed unless the option is set. Allowing
// semicolons should only be needed for special cases like defining a
// function or stored procedure in SQL. Trim any trailing delimiter to
// minimize false positives.
$query = rtrim($query, "; \t\n\r\0\x0B");
if (strpos($query, ';') !== FALSE && empty($options['allow_delimiter_in_query'])) {
throw new \InvalidArgumentException('; is not supported in SQL strings. Use only one statement at a time.');
}
$stmt = $this->prepareQuery($query);
$stmt->execute($args, $options);
}
To:
else {
$this->expandArguments($query, $args);
// To protect against SQL injection, Drupal only supports executing one
// statement at a time. Thus, the presence of a SQL delimiter (the
// semicolon) is not allowed unless the option is set. Allowing
// semicolons should only be needed for special cases like defining a
// function or stored procedure in SQL. Trim any trailing delimiter to
// minimize false positives unless delimiter is allowed.
$whitespace = " \t\n\r\0\x0B";
if (empty($options['allow_delimiter_in_query'])) {
$whitespace .= ';';
}
$query = rtrim($query, $whitespace);
if (strpos($query, ';') !== FALSE && empty($options['allow_delimiter_in_query'])) {
throw new \InvalidArgumentException('; is not supported in SQL strings. Use only one statement at a time.');
}
$stmt = $this->prepareQuery($query);
$stmt->execute($args, $options);
}
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | 3133798-18.patch | 3.04 KB | beakerboy |
| #15 | interdiff_3133798-13-15.txt | 709 bytes | beakerboy |
| #15 | 3133798-15.patch | 3.01 KB | beakerboy |
Comments
Comment #2
beakerboyComment #3
beakerboyComment #4
beakerboyComment #5
beakerboyComment #6
beakerboyI have the logic backwards, and it needs a test.
Comment #7
beakerboyComment #8
ravi.shankar commentedAdded needs tests tag.
Comment #9
beakerboyPatch and test for this issue.
Comment #10
beakerboyFix coding standard issues
Comment #11
daffie commentedPatch looks good and needs some work:
Why do you set the methods: "execute" and "setAttribute" to be replaced by a test double and then do not do that?
I am missing the assertion that the generated query is equal to the expected query.
This line can be rewritten to: "$mock_pdo = $this->getMockBuilder(StubPDO::class)".
@Beakerboy: Could you add an interdiff.txt with your patch file. I makes reviewing your patch a lot easier.
Comment #12
beakerboy@daffie
1)The connection class calls these two methods during the execution path of the test.
I guess I could replace the constructor of Connection with a stub so it does nothing. That would avoid calling setAttribute(). I can’t stub out the constructor because that’s how the StubPDO object gets in. Should I rewrite the constructor to just inject StubPDO and not set any attributes? I don’t see a way around the execute() function, since Connection::query() calls it immediately after calling PDO::prepare(). Let me know if there’s a better way.2)The mock object performs the assertion. For example, if you change the dataProvider the test will return the following:
Again, let me know if there’s a better way.
3) Can do, but all other tests in this class use this format so I was following precedent.
Comment #13
beakerboyUsing ::class keyword. Awaiting advice on other two suggestions. Should there be a follow up to use the ::class keyword throughout this class?
Comment #14
daffie commentedTested with the patch on my local machine.
For my own comment #11.1: the methods "execute" and "setAttribute" need to be mocked and the default mocking is enough for this test.
For my own comment #11.2: AFAIK every test needs an assertion or the PHPUnit will mark the test as risky. In this case that does not happen. Evidently I was wrong.
Could we add a comment before this code with something like: "When the method query() is called, the method prepare() should be called with the expected query." Or something else that explains how the testing works.
@Beakerboy: Thank you for adding the interdiff.txt file. I do not think that there should be a followup for only this class, maybe one for all test classes. You can create one if you like.
Comment #15
beakerboyAdded a comment to the test...although one of the points of a “fluent interface“ is that the code itself should explain what’s going on.
Comment #16
daffie commented@Beakerboy: I am used to look for something like
$this->assert*to see what the test is testing. This test does not have that. Helping other coders understand how a non standard test work is to me almost always welcome. Thank you for adding it.All my points are addressed.
All code changes look good to me.
For me it is RTBC.
Comment #17
alexpottWe need to fix this in 9.1.x first.
Comment #18
beakerboyPatch for 9.1.x
Comment #19
daffie commentedPatch is the same as the one for 8.9.
Back to RTBC.
Comment #21
alexpottCommitted and pushed c9c0fc1e59 to 9.1.x and 79495e89d7 to 9.0.x. Thanks!
Committed 635b422 and pushed to 8.9.x. Thanks!
Comment #24
krzysztof domańskiThe
$trim_charscontains two ordinary spaces.$trim_chars = " \t\n\r\0\x0B";This was before the last commit so it is not the result of a previous change. Looks like there should be one ordinary space and one non-breaking space. I created #3143618: Replace space (U+0020) with No-Break Space (U+00A0) to minimize false positives.