Fix SQL mode compat issues by removing the additional probe request#333
Conversation
github-actions
Bot
added
command:db
command:db-import
command:db-query
scope:testing
labels
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesThe PR changes MySQL/MariaDB SQL-mode handling. SQL mode compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WP_CLI
participant MySQL_Client
participant MySQL_Server
WP_CLI->>MySQL_Client: Add SQL-mode compatibility to --init-command
MySQL_Client->>MySQL_Server: Initialize session SQL mode
MySQL_Client->>MySQL_Server: Execute query or import SQL
MySQL_Server-->>WP_CLI: Return command result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/DB_Command.php (1)
489-574: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider documenting the SQL-mode behavior change in the command help.
The PR intentionally removes SQL-mode adaptation from
wp db query, and the objectives call this out as a breaking change on strict-mode servers (justifying the 3.0.0 bump). The command's own## OPTIONS/description doesn't mention this, so users hitting new strict-mode failures via--help/generated docs won't get a hint about the change.📝 Suggested doc note
* Use the `--skip-column-names` MySQL argument to exclude the headers * from a SELECT query. Pipe the output to remove the ASCII table * entirely. * + * Unlike `wp db import`, this command does not adapt the session SQL mode for + * WordPress compatibility; it runs under the server's own SQL modes, just like + * the `mysql` client itself. + * * ## OPTIONS🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/DB_Command.php` around lines 489 - 574, Update the `DB_Command` documentation for `wp db query` to explicitly state that the command no longer adapts SQL modes, including that queries may fail on servers enforcing strict SQL modes. Place the note in the command description or `## OPTIONS` help so it appears in `--help` and generated documentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/DB_Command.php`:
- Around line 931-938: Extract the duplicated SQL-mode compatibility handling
from import() and run_query() into a shared helper that merges the compatibility
statement with a caller-provided mysql_args['init-command'] instead of silently
discarding either value. When both are present, preserve both commands and emit
a user-visible warning about the conflict; retain existing behavior when only
the compatibility statement or only the user command is provided.
---
Nitpick comments:
In `@src/DB_Command.php`:
- Around line 489-574: Update the `DB_Command` documentation for `wp db query`
to explicitly state that the command no longer adapts SQL modes, including that
queries may fail on servers enforcing strict SQL modes. Place the note in the
command description or `## OPTIONS` help so it appears in `--help` and generated
documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ed0878e6-0a0f-47cf-9a4d-dd3a21891761
📒 Files selected for processing (3)
features/db-import.featurefeatures/db-query.featuresrc/DB_Command.php
| // Adapt the session SQL mode to be WordPress-compatible via --init-command, | ||
| // so it runs on connect before any SQL is read. This covers both file and | ||
| // STDIN imports, and needs no separate probe connection. | ||
| $sql_mode_compat = $this->get_sql_mode_compat_statement( $assoc_args ); | ||
| if ( '' !== $sql_mode_compat && ! isset( $mysql_args['init-command'] ) ) { | ||
| $mysql_args['init-command'] = $sql_mode_compat; | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Custom --init-command silently disables WordPress SQL-mode compatibility, with no warning, and the logic is duplicated in two places.
In both import() and run_query(), if the caller already passes their own --init-command (e.g. to set time_zone or other session vars), $sql_mode_compat is computed but then dropped entirely — no merge, no warning. This reintroduces the exact strict-mode import-failure risk the PR is meant to fix, silently, for anyone combining --init-command with wp db import/internal run_query() callers. The same 4-line block is copy-pasted in both places.
♻️ Proposed fix: shared helper + user-visible warning on conflict
+ /**
+ * Apply WordPress SQL-mode compatibility as an `--init-command`, unless the
+ * caller already supplied their own `--init-command` (in which case warn
+ * instead of silently dropping the compatibility statement).
+ *
+ * `@param` array $mysql_args Mysql args array, passed by reference.
+ * `@param` array $assoc_args The associative argument array passed to the command.
+ */
+ private function apply_sql_mode_compat_init_command( &$mysql_args, $assoc_args ) {
+ $sql_mode_compat = $this->get_sql_mode_compat_statement( $assoc_args );
+ if ( '' === $sql_mode_compat ) {
+ return;
+ }
+ if ( isset( $mysql_args['init-command'] ) ) {
+ WP_CLI::warning( 'A custom --init-command was provided; skipping automatic WordPress SQL-mode compatibility. Include the compatibility statement in your --init-command, or pass --skip-sql-mode-compat to silence this warning.' );
+ return;
+ }
+ $mysql_args['init-command'] = $sql_mode_compat;
+ }Then at each call site:
- // Adapt the session SQL mode to be WordPress-compatible via --init-command,
- // so it runs on connect before any SQL is read. This covers both file and
- // STDIN imports, and needs no separate probe connection.
- $sql_mode_compat = $this->get_sql_mode_compat_statement( $assoc_args );
- if ( '' !== $sql_mode_compat && ! isset( $mysql_args['init-command'] ) ) {
- $mysql_args['init-command'] = $sql_mode_compat;
- }
+ // Adapt the session SQL mode to be WordPress-compatible via --init-command,
+ // so it runs on connect before any SQL is read. This covers both file and
+ // STDIN imports, and needs no separate probe connection.
+ $this->apply_sql_mode_compat_init_command( $mysql_args, $assoc_args );(and identically at the run_query() call site.)
Also applies to: 1942-1948
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/DB_Command.php` around lines 931 - 938, Extract the duplicated SQL-mode
compatibility handling from import() and run_query() into a shared helper that
merges the compatibility statement with a caller-provided
mysql_args['init-command'] instead of silently discarding either value. When
both are present, preserve both commands and emit a user-visible warning about
the conflict; retain existing behavior when only the compatibility statement or
only the user command is provided.
|
20 of 30 Behat jobs fail. The split is clean along one axis:
The failures are deterministic, not flaky. Failing scenariosTwo pre-existing scenarios fail, both with
MechanismBoth scenarios run DDL through The new Run: https://github.com/wp-cli/db-command/actions/runs/29804346388 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
schlessera
changed the title
`wp db query`, `wp db import` and the internal `run_query()` adapted the session SQL mode to match WordPress. Building that adaptation opened a second MySQL connection to read `@@SESSION.sql_mode`, and that probe was built without the caller's connection options (`--host`, `--defaults`, SSL/TLS, sockets), so it connected to the wrong server and aborted the command before the real query ran. This is the root cause of #311, and of #237, #278, #288 and #218. - `wp db query` no longer adapts the SQL mode. It runs under the server's own modes, like the `mysql` client, so connection options are honored. This is a behavior change and targets the next major version. - `wp db import` and `run_query()` keep WordPress compatibility, but apply it via the client's `--init-command` on the same connection rather than a probe. This also covers dumps streamed from STDIN (#171). - Add a `--skip-sql-mode-compat` flag to `wp db import` to opt out. The incompatible modes are stripped from `@@SESSION.sql_mode` in a single statement, with each mode comma-wrapped so a substring like `ANSI` cannot corrupt `ANSI_QUOTES`.
Applying the compatibility statement via --init-command collided with init commands the caller already had: an explicit --init-command made the compat step drop out, and an init command from an option file (--defaults) was silently overridden by the CLI value. Prepend the compatibility statement to the executed batch instead, for both file imports and run_query(), so it composes with any caller init command. STDIN imports have no executed batch, so they keep using --init-command, which is what lets them get compatibility at all.
Replace the removed SQL-mode probe with a single mechanism shared by `wp db query`, `wp db import` and the internal `run_query()`: the WordPress-compatible session SQL mode is applied through the MySQL client's `--init-command`, on the same connection, before any SQL runs. This keeps `wp db query` adapting the SQL mode as it did before, so DDL against WordPress's zero-date schema (`ALTER TABLE wp_blogs ...`, `CREATE TABLE ... AS SELECT` from a WordPress table) keeps working on strict servers, while still removing the second probe connection that ignored the caller's connection options and caused #311. The STDIN import path (#171) is covered too, since `--init-command` applies whether or not there is an executed batch. When the caller supplies their own `--init-command`, the compat statement is composed with it into a single multi-statement `--init-command` (compat first) instead of being dropped. `--init-command-add` is not used: it is rejected by the mysql 5.6/5.7/8.0 and mariadb clients, whereas a single multi-statement `--init-command` runs both statements on every CI-targeted client (mysql 5.6/5.7/8.0/8.4, mariadb 11.4). Add `--skip-sql-mode-compat` to `wp db query` as well, so a query can opt into the server's own SQL modes.
2566fc9 to
ab66c6e
Compare
|
🦸 👍 |
2 participants
Problem
wp db query,wp db importand the internalrun_query()all adapted the session SQL mode to match WordPress (stripping modes likeSTRICT_TRANS_TABLESandNO_ZERO_DATE, the waywpdbdoes). To build that adaptation,get_sql_mode_query()calledget_current_sql_modes(), which opened a second MySQL connection to runSELECT @@SESSION.sql_mode.That probe connection was built with
$args = [], discarding the caller's connection options. So when you passed a custom--host,--defaults, SSL/TLS options or a socket, the probe connected to the wrong place (or failed outright) and aborted the whole command withFailed to get current SQL modes, before your actual query ever ran.This is the root cause of #311 and its duplicates #237, #278, #288 and #218 (each one is a different connection option the probe failed to forward, patched one at a time).
Approach
The probe is removed. Every path that needs WordPress SQL-mode compatibility now applies it through the MySQL client's
--init-commandon the same connection, so it runs right after connect and before any SQL is read — no second connection, so the connection options in play no longer matter.wp db query,wp db importandrun_query()all adapt the session SQL mode via--init-command. Behavior is unchanged from before for the default case: statements still run under WordPress-compatible modes, so DDL against WordPress's zero-date schema (ALTER TABLE wp_blogs …,CREATE TABLE … AS SELECTfrom a WordPress table, importing a headerless dump) keeps working on strict servers. This is not a breaking change.--init-commandapplies on connect regardless, so a dump streamed from STDIN gets the same treatment as a file import.--skip-sql-mode-compatflag onwp db queryandwp db import, to run under the server's own SQL modes.Why compatibility matters
WordPress schema declares datetime columns as
DEFAULT '0000-00-00 00:00:00', so real dumps and tables carry zero-date values. On MySQL 5.7+/8.0 (defaultsql_modeincludesNO_ZERO_DATE/STRICT_TRANS_TABLES), a raw statement against that schema without a relaxed session mode fails withERROR 1067 Invalid default value.mysqldump/wp db exportdumps carry their ownSET SQL_MODE='NO_AUTO_VALUE_ON_ZERO'header, but headerless dumps (compact exports, hand-written SQL, some third-party tools) and ad-hoc DDL throughwp db querystill need the session mode relaxed — which is what--init-commandnow provides, by default.Implementation notes
apply_sql_mode_compat_init_command(), is used byquery(),import()andrun_query(), so the logic lives in one place.@@SESSION.sql_modein a single expression. Each mode is comma-wrapped, e.g.REPLACE(CONCAT(',', @@SESSION.sql_mode, ','), ',ANSI,', ','), so a mode name that is a substring of another (ANSIinsideANSI_QUOTES) is never corrupted.--init-command. If the caller already set their own--init-command(via the CLI or an option file), the compat statement is composed with it into a single multi-statement--init-command— compat first, the caller's command second — rather than dropping either one. Compat first means it is always applied; a caller that re-setssql_modethemselves still wins, since the lastSETtakes effect. This fixes the previous behavior, where a caller-provided--init-commandon a STDIN import silently discarded the compat statement.--init-commandand not--init-command-add. MySQL 8.4 redefines--init-commandas a single statement and adds--init-command-addfor additional ones, but--init-command-addis rejected by mysql 5.6/5.7/8.0 and by the MariaDB client, so it is not portable. A single--init-commandcarrying two;-separated statements was verified to execute both on every CI-targeted client: mysql 5.6, 5.7, 8.0, 8.4 and mariadb 11.4.--init-commandis likewise available across all of them;get_mysql_binary_path()selects themariadbbinary on MariaDB, which also supports it.Testing
New/updated Behat scenarios in
db-query.featureanddb-import.feature:wp db queryadapts the SQL mode via--init-commandby default, with no probe;--skip-sql-mode-compatdisables it.wp db importapplies compat via--init-commandby default;--skip-sql-mode-compatdisables it.wp db importof a headerless zero-date dump succeeds, from both a file and STDIN (SQL mode compat logic is missing for SQL from STDIN #171).wp db importkeeps compat when the caller also passes--init-command.The pre-existing
db.feature/db-tables.featurescenarios that run DDL throughwp db query(CREATE TABLE … AS SELECTfromwp_users,ALTER TABLE wp_blogs) pass again on strict servers, which was the regression an earlier revision of this PR introduced.Known limitation
$sql_incompatible_modesdoes not includeNO_ZERO_IN_DATE(it predates this PR). I kept the existing mode set to keep this change scoped to the probe fix. WordPress Core's list does include it, so adding it is a reasonable follow-up.Fixes #311.
Fixes #171.
Summary by CodeRabbit
wp db importandwp db query.--skip-sql-mode-compatto disable automatic SQL-mode adjustments.--defaults) no longer fail.--init-command.