Fix lexer edge cases and string handling - #505
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change updates MySQL token handling, lexer parsing, LIKE pattern translation, and database query validation. It adds coverage for binary data, escaped newlines, vertical tabs, versioned comments, token ranges, LIKE patterns, and invalid query text. ChangesMySQL compatibility updates
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~30 minutes Merge Risk: ⚪ Minimal · up to This updates MySQL-compatible lexing and binary string handling, fixes token ranges and version-comment parsing, and adds invalid-query validation. The supplied coverage and current implementation summaries show no remaining merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant SQLInput as SQL input
participant WP_MySQL_Lexer
participant WP_MySQL_Token
SQLInput->>WP_MySQL_Lexer: tokenize comments, whitespace, and function syntax
WP_MySQL_Lexer->>WP_MySQL_Token: emit token bytes and ranges
WP_MySQL_Token->>WP_MySQL_Token: unescape token value
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Lexer benchmarkChanges to lexer-related files were detected and triggered a benchmark:
Note: Hosted runners are noisy, and absolute numbers vary. Treat the results with caution and verify them locally. To reproduce locally: |
f695a70 to
4ade8df
Compare
| } | ||
| ++$total; | ||
| $tokens = ( new WP_MySQL_Lexer( $query ) )->remaining_tokens(); | ||
| $tokens = ( new WP_MySQL_Lexer( $query, 80038 ) )->remaining_tokens(); |
There was a problem hiding this comment.
The only thing I found worth mentionning, that could maybe be removed. Since WP_MySQL_Lexer seems to default to 80038 when it is not defined.
There was a problem hiding this comment.
It does currently default to 80038, but this test checks an exact set of expected failures that depends on the MySQL version. Passing it explicitly keeps the test stable if the lexer’s default changes. I’ve added a comment explaining why it is pinned.
The final backslash-stripping step used preg_replace() with the "u" (UTF-8) modifier. Invalid UTF-8 made it return null, causing a TypeError because get_value() must return a string. MySQL literals can legitimately contain non-UTF-8 bytes in binary or single-byte charset payloads. Switch the modifier to "s" (DOTALL). Byte-wise unescaping preserves valid UTF-8 and raw bytes, and also handles a backslash followed by a newline. Legacy multibyte charsets such as Big5 and GBK remain unsupported. Remove 14 obsolete WordPress test failure expectations. Reclassify Tests_DB_Charset::test_invalid_characters_in_query as an expected assertion failure: invalid text is accepted instead of crashing at this step. Charset enforcement remains separate from token decoding.
When resolving a function keyword (SYM_FN), the lexer peeks for a following "("
and, under SQL_MODE_IGNORE_SPACE, skips intervening whitespace first. It skipped
by advancing bytes_already_read and never restored it. When no "(" followed, the
keyword was emitted as an IDENTIFIER whose length — derived from
bytes_already_read in produce() — now covered the trailing whitespace, so the
extracted value was e.g. "COUNT " instead of "COUNT". Under this ANSI-style mode
a column or table named after a function would resolve to the wrong identifier.
Peek with a local index instead of mutating bytes_already_read, so the token's
byte range ends at the keyword and the next scan consumes the whitespace.
Recognize six-digit version numbers followed by whitespace starting with MySQL 8.1. Preserve five-digit parsing for older server versions and when the sixth digit is not followed by whitespace. Keep fewer than five leading digits as SQL content. Accept all MySQL ASCII whitespace, including vertical tabs, in the PHP and native lexers. Test version boundaries, whitespace, fallback parsing, and unversioned numeric content. Keep the corpus expectations pinned to MySQL 8.0.38. https://dev.mysql.com/blog-archive/are-you-ready-for-mysql-10/ https://github.com/mysql/mysql-server/blob/mysql-8.1.0/sql/sql_lex.cc
Use wpdb's charset checks to reject invalid text before executing a query, while preserving binary data and prevalidated queries. Keep Query Monitor entries aligned when validation performs charset lookups. Remove the two obsolete failure expectations for the existing WordPress invalid-text tests. Document that full charset enforcement belongs in MySQL on SQLite so it protects all callers, including direct driver use.
Remove UTF-8 validation from LIKE-to-GLOB pattern conversion so raw bytes do not turn the pattern into NULL. Use DOTALL to also unescape backslashes followed by newlines. Cover byte preservation, malformed UTF-8, Unicode, wildcards, escapes, and NULL in helper and query tests. Document the remaining GLOB limitation: distinct invalid UTF-8 sequences can match as equal.
4ade8df to
8cd2a07
Compare
Summary
This PR fixes lexer edge cases and related string handling:
IGNORE_SPACE.wpdb::query()charset checks, retaining its binary-data and prevalidated-query exemptions.LIKE BINARYpatterns: Preserve raw bytes instead of producingNULLduring GLOB conversion, and handle escaped newlines.The lexer fixes cover both PHP packages and the native extension.
Why
The token decoder's UTF-8 regex modifier caused an accidental
TypeErrorfor legitimate binary data as well as malformed text. Decoding must preserve bytes; charset enforcement needs column types and SQL mode. Mirroring WordPress's checks restores its expected validation behavior.Regression tests cover decoding, token ranges, version comments, and pattern conversion. CI expectations remove the
test_strip_invalid_textcases fixed by decoding and the two invalid-query tests fixed by WordPress validation.Remaining limitations
LIKE BINARYfix preserves pattern bytes but does not provide fully byte-correct matching.