refactor(update): improved update process #3906
Conversation
📝 WalkthroughWalkthroughAdds a migration framework and migration-driven update flow: new migration contracts, builders, dialects, operations, registry/tracker/executor, many concrete migrations and tests, Update/UpdateRunner integrated with dry‑run reporting, plus small API changes (public properties) and routing rewrites for /update. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Controller / CLI
participant Registry as MigrationRegistry
participant Executor as MigrationExecutor
participant Migration as Migration
participant Recorder as OperationRecorder
participant FS as Filesystem
participant DB as Database
UI->>Registry: getPendingMigrations(currentVersion)
Registry-->>UI: pendingMigrations[]
UI->>Executor: setDryRun(flag)
UI->>Executor: executeMigrations(pendingMigrations)
loop per migration
Executor->>Migration: up(OperationRecorder)
Migration->>Recorder: record operations (addSql/addConfig/file/... )
Recorder-->>Executor: operations[]
alt dry-run
Executor->>Executor: collect operations -> MigrationResult (dry)
else apply
Executor->>DB: execute SQL ops
Executor->>FS: perform file ops
Executor->>Executor: execute permission/config ops
DB-->>Executor: sql results
Executor->>Registry: (n/a) -- // registry used for discovery only
Executor->>MigrationTracker: recordMigration(version, time, checksum)
MigrationTracker-->>Executor: ack
end
Executor-->>UI: MigrationResult
end
UI->>Executor: generateDryRunReport()
Executor-->>UI: report
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/AbstractMigration.php`:
- Around line 194-204: The updateLanguageCode method currently interpolates
$oldCode and $newCode directly into the SQL string causing an SQL injection
risk; change the implementation to produce a parameterized SQL statement (e.g.
"UPDATE %s SET %s = ? WHERE %s = ?") and update callers to bind $newCode and
$oldCode as parameters when executing, or alternatively escape/quote the values
using the project's DB quoting utility before interpolation; locate
updateLanguageCode and ensure you also keep using $this->table($table) for the
table name and $column identifier safely (or validate/quote the column) when
switching to parameterized or quoted values.
- Around line 170-173: The CREATE INDEX SQL currently includes "IF NOT EXISTS"
which breaks on MySQL/MariaDB; update the method in AbstractMigration that
returns sprintf('CREATE INDEX IF NOT EXISTS %s ON %s (%s)', $indexName,
$tableName, $columnList) to emit a MySQL-safe path: when the connection/driver
indicates MySQL or MariaDB (use your existing DB connection/driver probe in
AbstractMigration), either return "CREATE INDEX %s ON %s (%s)" or, better,
perform an existence check against information_schema.STATISTICS for $tableName
and $indexName and only run the CREATE when the index is absent; modify the code
that constructs/executes the SQL so the driver branch for MySQL/MariaDB uses the
existence-check logic instead of the "IF NOT EXISTS" form.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationExecutor.php`:
- Around line 67-101: The catch block in MigrationExecutor currently only
catches \Exception, so fatal engine errors (TypeError, Error) from calls like
$migration->up() or $operation->execute() can escape and leave migrations
untracked; change the catch to catch \Throwable $e and keep the existing
handling (set $result->setSuccess(false) and
$result->setErrorMessage($e->getMessage())) so all throwables are recorded, and
verify tracking logic around $result and $this->tracker->recordMigration remains
unchanged.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationTracker.php`:
- Around line 186-195: The SQL in MigrationTracker::getLastAppliedVersion() uses
MySQL-style "LIMIT 1" which breaks on SQL Server; update the method to build the
query based on the database driver (e.g., detect via
$this->configuration->getDb() or its driver name): use "SELECT TOP 1 version
FROM <table> ORDER BY id DESC" for SQL Server and keep "SELECT version FROM
<table> ORDER BY id DESC LIMIT 1" for databases that support LIMIT; implement a
small conditional that selects the appropriate SQL string before executing the
query and leave the rest of the logic (query, fetchObject, return) unchanged.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigRenameOperation.php`:
- Around line 53-56: The execute() method is ignoring the boolean result from
Configuration::rename() and always returning true; change execute() in
ConfigRenameOperation to return the actual rename outcome by calling
$this->configuration->rename($this->oldKey, $this->newKey) and returning its
boolean result so failures are propagated instead of masking them.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/MysqlDialect.php`:
- Around line 113-118: The createIndex method in MysqlDialect is ignoring the
$ifNotExists flag; update MysqlDialect::createIndex to honor it by performing a
pre-check against information_schema.STATISTICS for the given table and index
name when $ifNotExists is true (query for COUNT(*) WHERE table_schema =
DATABASE() AND table_name = $tableName AND index_name = $indexName) and return
an empty string or no-op if the index already exists; alternatively, if you
prefer not to implement DB access here, make the limitation explicit by throwing
a clear exception or returning a commented SQL string when $ifNotExists is true
(e.g., throw new \RuntimeException('IF NOT EXISTS not supported for CREATE INDEX
in MysqlDialect')) so callers cannot assume idempotency.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/TableBuilder.php`:
- Around line 237-242: The CREATE TABLE builder in TableBuilder currently
inlines indexes from $this->indexes (producing "INDEX"/"UNIQUE INDEX" fragments)
which is MySQL-specific; change the logic so inline index fragments are only
emitted for the MySQL dialect (check the builder's dialect/platform indicator
used in this class) and for all other dialects remove those inline fragments and
instead call or append the dialect-aware statements returned by
buildIndexStatements() (which already generates separate CREATE INDEX
statements). Update the code that iterates $this->indexes (the snippet producing
$parts[] = "$indexType $indexName ($columnList)";) to branch on dialect: produce
inline index text only for MySQL, otherwise skip adding to $parts and ensure
buildIndexStatements() output is merged into the final SQL batch.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Beta.php`:
- Around line 47-59: The ALTER statements use MySQL-specific comma-separated
DROP COLUMN syntax; update Migration320Beta to emit portable DDL by replacing
the combined DROP COLUMN statements with one DROP COLUMN per addSql (i.e. two
separate sprintf('ALTER TABLE %sfaqdata DROP COLUMN %s', ...) calls and likewise
for faqdata_revisions for links_state and links_check_date) and add a platform
check using the connection/platform in Migration320Beta (e.g.
$this->connection->getDatabasePlatform()->getName() or similar) so that for
SQLite you perform the proper table-rebuild via Doctrine's
SchemaManager/SchemaComparator (or skip and implement the safe recreation)
instead of issuing DROP COLUMN, and for PostgreSQL ensure the statements do not
use commas.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Alpha.php`:
- Around line 171-188: The INSERT uses SELECT * from the old faqcategory_order
(which has only category_id and position) into the new faqcategory_order_new
(category_id, parent_id, position), causing a column mismatch; update the
recorder->addSql call that inserts data into faqcategory_order_new to use an
explicit column list and select columns accordingly—e.g., INSERT INTO
%sfaqcategory_order_new (category_id, parent_id, position) SELECT category_id,
NULL AS parent_id, position FROM %sfaqcategory_order—so parent_id is supplied
(NULL) and columns align during the Migration400Alpha migration.
- Around line 162-169: In the isSqlServer() branch inside Migration400Alpha (the
block that calls $recorder->addSql), remove the unsupported "AFTER category_id"
clause from the ALTER TABLE statement so SQL Server uses its default behavior of
appending the new column; update the sprintf argument passed to
$recorder->addSql (the SQL string) to end after "ADD COLUMN parent_id INTEGER
DEFAULT NULL" while keeping the same descriptive message ("Add parent_id column
to faqcategory_order (SQL Server)").
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration409.php`:
- Around line 44-66: In Migration409 (the PostgreSQL-only block using
$recorder->addSql), use the prefixed sequence name everywhere by building it
with $this->tablePrefix (e.g., '%sfaqseo_id_seq') for the ALTER TABLE ... SET
DEFAULT nextval(...) and for setval(...), and guard against NULL when computing
MAX(id) by wrapping it in COALESCE((SELECT MAX(id) FROM %sfaqseo), 0) so setval
never receives NULL; update the sprintf calls for nextval and setval to
reference the same prefixed sequence variable and to include the
COALESCE-wrapped MAX(id) expression.
In `@phpmyfaq/src/phpMyFAQ/Setup/Update.php`:
- Around line 201-237: applyUpdates() currently runs runPostMigrationTasks(),
optimizeTables(), executeQueries() and updateVersion() even when migrations
failed; change the control flow so that after executing migrations (via
migrationExecutor->executeMigrations and storing results in
$this->migrationResults) you check $this->allMigrationsSucceeded() and only then
run runPostMigrationTasks(), optimizeTables(), executeQueries(), and
updateVersion(); keep existing dry-run behavior (collectDryRunQueries when
$this->dryRun) and ensure updateVersion() is invoked only when not dry-run and
migrations succeeded.
In `@phpmyfaq/src/phpMyFAQ/Setup/UpdateRunner.php`:
- Around line 76-79: In taskUpdateDatabase() the Update object's version is
being set to System::getVersion() which is the target version, but
getPendingMigrations() needs the current installed version; change the
assignment so the Update instance uses the current version (the same value used
in the dry-run path, $fromVersion) — i.e., set $update->version = $fromVersion
instead of System::getVersion() when preparing the real update so
getPendingMigrations() sees the correct starting version.
🟡 Minor comments (16)
phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/SqlServerDialect.php-91-100 (1)
91-100: UsequoteIdentifier()for table and index names in SQL statements.The
$tableNameand$indexNameparameters are interpolated directly without escaping increateTablePrefix()(lines 95-96, 99) andcreateIndex()(lines 123-124, 127). While table names in this migration system are application-controlled (low risk), follow best practices by using the existingquoteIdentifier()method for identifiers, and escaping string values in WHERE clauses.For the
IF NOT EXISTSchecks, the table name needs string escaping (e.g.,str_replace("'", "''", $tableName)) since it's compared as a value, not an identifier. For theCREATE TABLEandCREATE INDEXstatements, usequoteIdentifier()for proper SQL Server escaping.Note: This same pattern exists across all dialect implementations and should be addressed consistently.
phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration407.php-61-105 (1)
61-105: Consider case-insensitive matching for language code updates on case-sensitive databases.The WHERE clauses match only exact lowercase
fr-caandpt-brformats. PostgreSQL and SQLite are case-sensitive, so any rows containing uppercase variants (fr-CA,fr_CA,pt-BR,pt_BR) would remain unnormalized. While no such variants were found in the current codebase, defensive matching usingLOWER()would ensure completeness.💡 Suggested approach (apply similarly to pt-br)
- "UPDATE %s%s SET %s='fr_ca' WHERE %s='fr-ca'", + "UPDATE %s%s SET %s='fr_ca' WHERE LOWER(%s)='fr-ca'",phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/AlterTableBuilder.php-183-216 (1)
183-216: Guard against empty combined builds.
If no alterations are added,buildCombined()returnsALTER TABLE <name>which is invalid SQL. Consider failing fast.🛠️ Proposed guard for empty combined builds
public function buildCombined(): string { + if ($this->alterations === []) { + throw new \LogicException('No alterations defined for combined ALTER TABLE statement.'); + } $parts = [];tests/phpMyFAQ/Setup/Migration/Versions/Migration320AlphaTest.php-67-81 (1)
67-81: Test assertion is ineffective - callback always returns true.The callback collects keys but always returns
true, meaning the test passes regardless of which config keys are added. It doesn't verify thatsecurity.enableSignInWithMicrosoft(or similar) is actually set.🔧 Suggested fix to make assertion meaningful
public function testUpAddsEnableSignInWithMicrosoftConfig(): void { $recorder = $this->createMock(OperationRecorder::class); + $capturedKeys = []; $recorder ->expects($this->atLeastOnce()) ->method('addConfig') - ->with($this->callback(function ($key) { - static $keys = []; - $keys[] = $key; - return true; - })); + ->with($this->callback(function ($key) use (&$capturedKeys) { + $capturedKeys[] = $key; + return true; + })); $this->migration->up($recorder); + + $this->assertContains('security.enableSignInWithMicrosoft', $capturedKeys); }phpmyfaq/src/phpMyFAQ/Setup/Migration/AbstractMigration.php-147-152 (1)
147-152:dropColumns()uses MySQL-specific syntax.The multi-column DROP syntax (
ALTER TABLE x DROP COLUMN a, DROP COLUMN b) is MySQL-specific. PostgreSQL, SQLite, and SQL Server require separate ALTER statements for each column.Consider adding dialect-specific handling similar to
addColumn().🔧 Suggested approach
protected function dropColumns(string $table, array $columns): string { $tableName = $this->table($table); + + // SQLite doesn't support DROP COLUMN in older versions + // PostgreSQL and SQL Server require separate statements + if (!$this->isMySql()) { + throw new \RuntimeException('dropColumns() only supports MySQL. Use dropColumn() in a loop for other databases.'); + } + $dropParts = array_map(static fn($col) => "DROP COLUMN $col", $columns); return sprintf('ALTER TABLE %s %s', $tableName, implode(', ', $dropParts)); }tests/phpMyFAQ/Setup/Migration/Versions/Migration320AlphaTest.php-83-101 (1)
83-101: Test always passes - callback never returns false.The callback checks for
faqbackupbut returnstrueregardless of whether it's found. The test won't fail if the backup table SQL is missing.🔧 Suggested fix
public function testUpAddsSqlForBackupTable(): void { $recorder = $this->createMock(OperationRecorder::class); + $foundBackupSql = false; $recorder ->expects($this->atLeastOnce()) ->method('addSql') - ->with($this->callback(function ($sql) { - static $sqlStatements = []; - $sqlStatements[] = $sql; - // Check that at least one SQL contains backup table creation + ->with($this->callback(function ($sql) use (&$foundBackupSql) { if (str_contains($sql, 'faqbackup')) { - return true; + $foundBackupSql = true; } return true; })); $this->migration->up($recorder); + + $this->assertTrue($foundBackupSql, 'Expected SQL statement for faqbackup table creation'); }tests/phpMyFAQ/Setup/Migration/Operations/DirectoryCopyOperationTest.php-27-33 (1)
27-33: Description assertions can fail whenPMF_ROOT_DIRis defined.
getDescription()shortens paths whenPMF_ROOT_DIRis set. The test hardcodes full paths, which can become relative and fail in environments where the constant is defined. Consider deriving expected substrings the same way.🧩 Suggested fix
- $this->assertStringContainsString('Copy directory:', $operation->getDescription()); - $this->assertStringContainsString('/source/dir', $operation->getDescription()); - $this->assertStringContainsString('/dest/dir', $operation->getDescription()); + $description = $operation->getDescription(); + $source = defined('PMF_ROOT_DIR') ? str_replace(PMF_ROOT_DIR, '', '/source/dir') : '/source/dir'; + $destination = defined('PMF_ROOT_DIR') ? str_replace(PMF_ROOT_DIR, '', '/dest/dir') : '/dest/dir'; + + $this->assertStringContainsString('Copy directory:', $description); + $this->assertStringContainsString($source, $description); + $this->assertStringContainsString($destination, $description);phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php-322-358 (1)
322-358: SQL Server indexes lack idempotency checks.Similar to the earlier index creation issue, the SQL Server chat message indexes don't include
IF NOT EXISTSchecks. Consider wrapping these in conditional statements for idempotency:IF NOT EXISTS (SELECT * FROM sys.indexes WHERE name = 'idx_chat_sender') CREATE INDEX idx_chat_sender ON ...phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Alpha.php-104-107 (1)
104-107: Comment mentions ReCAPTCHA v3 but config keys are for v2.The comment says "Google ReCAPTCHAv3 support" but the configuration keys are named
googleReCaptchaV2*. Please update the comment to match the actual implementation.- // Google ReCAPTCHAv3 support + // Google ReCAPTCHA v2 support $recorder->addConfig('security.enableGoogleReCaptchaV2', false);phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Alpha.php-98-99 (1)
98-99: No-op config rename detected.This
renameConfigcall renamessecurity.loginWithEmailAddressto itself, which is effectively a no-op. This appears to be either a mistake or a placeholder that was not completed.- // Update options - $recorder->renameConfig('security.loginWithEmailAddress', 'security.loginWithEmailAddress'); + // TODO: If this was intended to rename from a different key, update accordingly + // $recorder->renameConfig('security.oldConfigName', 'security.loginWithEmailAddress');phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Alpha.php-77-84 (1)
77-84: Consider addingIF NOT EXISTSto faqbackup table creation.The
CREATE TABLEstatement forfaqbackuplacks an existence check, which could cause the migration to fail if the table already exists.Suggested fix
- $recorder->addSql(sprintf('CREATE TABLE %sfaqbackup ( + $recorder->addSql(sprintf('CREATE TABLE IF NOT EXISTS %sfaqbackup ( id INT NOT NULL,Note: For SQL Server compatibility, you may need dialect-specific handling similar to other table creations in this PR.
phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php-81-84 (1)
81-84: Index creation may fail on re-run without idempotency check.The
CREATE INDEXstatement lacks an existence check, which could cause the migration to fail if executed multiple times or if the index already exists. MySQL supportsCREATE INDEX IF NOT EXISTSsyntax (MySQL 8.0.29+), or you could use a conditional check similar to the SQL Server pattern.Suggested fix for MySQL
- $recorder->addSql( - sprintf('CREATE INDEX idx_hash ON %sfaqadminlog (hash)', $this->tablePrefix), - 'Create hash index on faqadminlog', - ); + if ($this->isMySql()) { + $recorder->addSql( + sprintf( + 'CREATE INDEX IF NOT EXISTS idx_hash ON %sfaqadminlog (hash)', + $this->tablePrefix + ), + 'Create hash index on faqadminlog (MySQL)', + ); + } elseif ($this->isSqlServer()) { + $recorder->addSql( + sprintf( + "IF NOT EXISTS (SELECT * FROM sys.indexes WHERE name = 'idx_hash') " . + "CREATE INDEX idx_hash ON %sfaqadminlog (hash)", + $this->tablePrefix + ), + 'Create hash index on faqadminlog (SQL Server)', + ); + } else { + $recorder->addSql( + sprintf('CREATE INDEX IF NOT EXISTS idx_hash ON %sfaqadminlog (hash)', $this->tablePrefix), + 'Create hash index on faqadminlog', + ); + }phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Beta.php-97-101 (1)
97-101: Replace deprecated SQL ServerTEXTtype withNVARCHAR(MAX).The
TEXTdata type has been deprecated in SQL Server since version 2005. UseNVARCHAR(MAX)instead for Unicode support and alignment with modern SQL Server standards.Suggested fix
} elseif ($this->isSqlServer()) { $recorder->addSql( - sprintf('ALTER TABLE %sfaqconfig ALTER COLUMN config_value TEXT', $this->tablePrefix), + sprintf('ALTER TABLE %sfaqconfig ALTER COLUMN config_value NVARCHAR(MAX)', $this->tablePrefix), 'Change faqconfig.config_value to TEXT (SQL Server)', ); }phpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationTracker.php-99-102 (1)
99-102: Add null check before accessing$row->cnt.If
fetchObject()returnsnull(e.g., query failure), accessing$row->cntwill cause a fatal error.🐛 Proposed fix
$result = $this->configuration->getDb()->query($query); $row = $this->configuration->getDb()->fetchObject($result); - return (int) $row->cnt > 0; + return $row !== null && (int) $row->cnt > 0; }phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/PermissionGrantOperation.php-60-74 (1)
60-74: CheckaddRight()return value before using it.
addRight()returns0when the permission already exists or the insert fails. Passing0tograntUserRight()causes it to fail silently while the operation returnstrue.🐛 Proposed fix
public function execute(): bool { try { $user = new User($this->configuration); $rightData = [ 'name' => $this->permissionName, 'description' => $this->permissionDescription, ]; $rightId = $user->perm->addRight($rightData); + if ($rightId <= 0) { + return false; + } $user->perm->grantUserRight($this->userId, $rightId); return true; } catch (\Exception) { return false; } }tests/phpMyFAQ/Setup/Migration/AbstractMigrationTest.php-26-28 (1)
26-28: Suppress PHPMD unused-parameter warning in the no-opup()test stub.PHPMD flags the unused
$recorderparameter. If PHPMD is enforced on tests, add a suppression to keep CI clean.🧹 Suggested suppression
+ /** + * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) + */ public function up(OperationRecorder $recorder): void { // Empty for testing }
🧹 Nitpick comments (30)
phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration323.php (2)
56-89: SQLite table rebuild uses MySQL-styleINT(1)syntax.SQLite doesn't recognize the
(1)length specifier forINT. While SQLite is permissive and will accept this syntax, it's ignored. For consistency and clarity, useINTEGERinstead.Also,
INSERT INTO ... SELECT *assumes column ordering matches between old and new tables. Consider explicitly listing columns to avoid subtle bugs if schemas diverge.♻️ Suggested fix for SQLite syntax
ip VARCHAR(64) NULL, account_status VARCHAR(50) NULL, last_login VARCHAR(14) NULL, auth_source VARCHAR(100) NULL, member_since VARCHAR(14) NULL, remember_me VARCHAR(150) NULL, - success INT(1) NULL DEFAULT 1, - is_superadmin INT(1) NULL DEFAULT 0, - login_attempts INT(1) NULL DEFAULT 0, + success INTEGER NULL DEFAULT 1, + is_superadmin INTEGER NULL DEFAULT 0, + login_attempts INTEGER NULL DEFAULT 0, refresh_token TEXT NULL DEFAULT NULL,
42-96: Consider a fallback or warning for unrecognized database types.If
up()is called with an unsupported database type, no operations are recorded and the migration silently does nothing. Consider logging a warning or throwing an exception for unsupported databases to make debugging easier.♻️ Add fallback handling
} elseif ($this->isSqlServer()) { $recorder->addSql( sprintf('ALTER TABLE %sfaquser ALTER COLUMN ip VARCHAR(64)', $this->tablePrefix), 'Increase faquser.ip column size (SQL Server)', ); + } else { + throw new \RuntimeException('Unsupported database type for migration 3.2.3'); } }phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration405.php (1)
65-118: Consider adding a fallback for unrecognized database types.Similar to Migration323, if the database type is not recognized, the column size increase will silently not happen. Adding explicit handling would improve debuggability.
phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/SqlServerDialect.php (1)
81-84: Consider returning date-only value forcurrentDate().
currentDate()currently returnsGETDATE(), which includes time components. For a date-only default,CAST(GETDATE() AS DATE)would be more precise. However, this may be acceptable depending on how the value is used.♻️ Optional: Use date-only function
public function currentDate(): string { - return 'GETDATE()'; + return 'CAST(GETDATE() AS DATE)'; }phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Alpha2.php (1)
42-86: Consider using the QueryBuilder/TableBuilder for DDL.
Now that dialect-aware builders exist, using them here would reduce per-dialect SQL duplication and keep type mappings centralized for future migrations.phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/TableBuilder.php (3)
104-108: Potential SQL injection in default value handling.The default value is directly interpolated into the SQL string without escaping. If user-controlled data ever reaches this method, it could result in SQL injection.
$defaultVal = $default !== null ? "'$default'" : null;Consider using a proper escaping mechanism or parameterized approach for default values.
♻️ Suggested improvement
public function varchar(string $name, int $length, bool $nullable = true, ?string $default = null): self { - $defaultVal = $default !== null ? "'$default'" : null; + $defaultVal = $default !== null ? "'" . addslashes($default) . "'" : null; return $this->addColumn($name, $this->dialect->varchar($length), $nullable, $defaultVal); }
148-152: Same SQL injection concern as varchar().The
char()method has the same issue with unescaped default value interpolation.♻️ Suggested improvement
public function char(string $name, int $length, bool $nullable = true, ?string $default = null): self { - $defaultVal = $default !== null ? "'$default'" : null; + $defaultVal = $default !== null ? "'" . addslashes($default) . "'" : null; return $this->addColumn($name, $this->dialect->char($length), $nullable, $defaultVal); }
261-273: Potential duplicate index creation.
build()already adds inline indexes (lines 238-242), andbuildIndexStatements()generates separate CREATE INDEX statements for the same indexes. Callers using both methods will attempt to create duplicate indexes.Consider documenting the intended usage pattern or providing a flag to exclude inline indexes from
build()whenbuildIndexStatements()will be used.phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Beta2.php (1)
42-58: Consider using inheritedaddColumn()helper for consistency.The
AbstractMigrationclass providesaddColumn()which already handles SQLite vs other databases. Using it would reduce code duplication and ensure consistent behavior.♻️ Suggested refactor
public function up(OperationRecorder $recorder): void { // WebAuthn support $recorder->addConfig('security.enableWebAuthnSupport', false); - if ($this->isSqlite()) { - $recorder->addSql( - sprintf('ALTER TABLE %sfaquser ADD COLUMN webauthnkeys TEXT NULL DEFAULT NULL', $this->tablePrefix), - 'Add WebAuthn keys column to faquser (SQLite)', - ); - } else { - $recorder->addSql( - sprintf('ALTER TABLE %sfaquser ADD webauthnkeys TEXT NULL DEFAULT NULL', $this->tablePrefix), - 'Add WebAuthn keys column to faquser', - ); - } + $recorder->addSql( + $this->addColumn('faquser', 'webauthnkeys', $this->textType() . ' NULL DEFAULT NULL'), + 'Add WebAuthn keys column to faquser', + ); }tests/phpMyFAQ/Setup/Migration/Operations/FileCopyOperationTest.php (1)
10-69: Missing test forexecute()method.The
OperationInterfacerequires anexecute(): boolmethod, but there's no test verifying the copy operation behavior. Consider adding tests for:
- Successful file copy
- Behavior when source doesn't exist and
onlyIfExistsis true- Behavior when source doesn't exist and
onlyIfExistsis false🧪 Suggested test additions
public function testExecuteSuccessfulCopy(): void { $this->filesystem ->expects($this->once()) ->method('copy') ->with('/source/file.txt', '/dest/file.txt') ->willReturn(true); $operation = new FileCopyOperation($this->filesystem, '/source/file.txt', '/dest/file.txt'); $this->assertTrue($operation->execute()); } public function testExecuteWithOnlyIfExistsAndMissingSource(): void { $this->filesystem ->expects($this->once()) ->method('exists') ->with('/source/file.txt') ->willReturn(false); $this->filesystem ->expects($this->never()) ->method('copy'); $operation = new FileCopyOperation($this->filesystem, '/source/file.txt', '/dest/file.txt', true); $this->assertTrue($operation->execute()); }tests/phpMyFAQ/Setup/UpdateTest.php (1)
99-107: Optional: remove ReflectionClass intestSetDryRun.
SincedryRunis now public, direct assertions are simpler and less brittle.♻️ Simplified test
public function testSetDryRun(): void { $this->update->dryRun = true; - $reflection = new \ReflectionClass($this->update); - $property = $reflection->getProperty('dryRun'); - $this->assertTrue($property->getValue($this->update)); + $this->assertTrue($this->update->dryRun); $this->update->dryRun = false; - $this->assertFalse($property->getValue($this->update)); + $this->assertFalse($this->update->dryRun); }phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigUpdateOperation.php (1)
70-84: Consider extractingformatValueto a shared helper to avoid duplication.
This mirrorsConfigAddOperation::formatValue; a shared trait/helper would reduce drift.tests/phpMyFAQ/Setup/Migration/Operations/ConfigRenameOperationTest.php (1)
1-19: Consider adding the license header for consistency.Other PHP files in the codebase include the Mozilla Public License header. While test files sometimes omit this, adding it would maintain consistency across the project.
tests/phpMyFAQ/Setup/Migration/QueryBuilder/DialectFactoryTest.php (1)
1-10: Consider adding the license header for consistency.Similar to other test files, consider adding the standard MPL license header for project consistency.
tests/phpMyFAQ/Setup/Migration/Operations/SqlOperationTest.php (1)
1-10: Consider adding the license header for consistency.For project-wide consistency, consider adding the standard MPL license header.
phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigAddOperation.php (1)
54-58: Consider adding exception handling to align with other operation types.The
execute()method returnstruewithout handling potential exceptions from$configuration->add(). Other operation types includingSqlOperation,FileCopyOperation,DirectoryCopyOperation, andPermissionGrantOperationuse try-catch to catch exceptions and returnfalse. For consistency,ConfigAddOperation(and similarly structuredConfigRenameOperation,ConfigUpdateOperation,ConfigDeleteOperation) should follow the same pattern.♻️ Proposed fix for consistent error handling
public function execute(): bool { - $this->configuration->add($this->key, $this->value); - return true; + try { + $this->configuration->add($this->key, $this->value); + return true; + } catch (\Exception) { + return false; + } }tests/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/SqlServerDialectTest.php (2)
5-9: Unnecessary attribute and missing test coverage.The
#[AllowMockObjectsWithoutExpectations]attribute is unnecessary since no mock objects are created in this test class.Additionally, there's no test for the
date()method, which returns'DATE'according to theSqlServerDialectimplementation.Suggested addition
public function testDate(): void { $this->assertEquals('DATE', $this->dialect->date()); }
92-96: Consider testingaddColumnwith the$afterparameter.The test only verifies
addColumnwithout the optional$afterparameter. Since SQL Server doesn't support column positioning (as confirmed bysupportsColumnPositioning()returningfalse), it would be valuable to add a test confirming that the$afterparameter is correctly ignored.Suggested test addition
public function testAddColumnIgnoresAfterParameter(): void { $result = $this->dialect->addColumn('test_table', 'new_column', 'NVARCHAR(255)', 'existing_column'); // SQL Server doesn't support AFTER clause, should be ignored $this->assertEquals('ALTER TABLE test_table ADD new_column NVARCHAR(255)', $result); }phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php (1)
86-107: Consider clarifying theidcolumn generation strategy forfaqcustompages.The
faqcustompagestable uses a composite primary key(id, lang)but theidcolumn is not auto-increment. This appears intentional for multilingual content where the sameidwith differentlangvalues represents translations of the same page. However, this design requires the application to manually generate and manageidvalues.Consider adding a brief comment in the migration description or code to clarify this design decision for future maintainers.
phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Alpha.php (1)
53-58: SQLite column type uses non-standardINT(1)syntax.SQLite uses
INTEGERas its integer affinity type. While SQLite is permissive and will acceptINT(1), usingINTEGERis more idiomatic and consistent with other SQLite migrations in this PR (e.g.,Migration420AlphausesINTEGER).Suggested fix
$recorder->addSql(sprintf( 'ALTER TABLE %sfaquserdata - ADD COLUMN twofactor_enabled INT(1) NULL DEFAULT 0, + ADD COLUMN twofactor_enabled INTEGER NULL DEFAULT 0, ADD COLUMN secret VARCHAR(128) NULL DEFAULT NULL', $this->tablePrefix, ), 'Add 2FA columns to faquserdata (SQLite)');phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/FileCopyOperation.php (1)
56-68: Consider logging or indicating when a file copy is skipped.When
onlyIfExistsistrueand the source file doesn't exist, the operation silently returnstrue. While this is the intended behavior, it may make debugging difficult if files are unexpectedly missing. Consider whether the caller should be informed that the operation was skipped versus successfully completed.Additionally, if the destination directory doesn't exist,
$this->filesystem->copy()may fail. Consider ensuring the destination directory exists before attempting the copy.Suggested enhancement
public function execute(): bool { if ($this->onlyIfExists && !file_exists($this->source)) { - return true; // Skip if a source doesn't exist and onlyIfExists is true + // Source doesn't exist but onlyIfExists allows skipping + return true; } try { + // Ensure destination directory exists + $destDir = dirname($this->destination); + if (!is_dir($destDir)) { + $this->filesystem->mkdir($destDir, 0755, true); + } $this->filesystem->copy($this->source, $this->destination); return true; } catch (\Exception) { return false; } }phpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationTracker.php (1)
84-85: Consider error handling for table creation failure.The
ensureTableExists()method executes the CREATE TABLE query but does not verify success or handle potential failures. If the query fails silently, subsequent operations will fail with confusing errors.♻️ Suggested improvement
- $this->configuration->getDb()->query($createTableSql); + $result = $this->configuration->getDb()->query($createTableSql); + if ($result === false) { + throw new \RuntimeException('Failed to create migrations tracking table'); + } }tests/phpMyFAQ/Setup/Migration/MigrationExecutorTest.php (1)
301-313: Intentional empty callback - suppress the static analysis warning.The unused
$recorderparameter is expected since this mock simulates an empty migration with no operations. Consider adding an annotation or brief comment to clarify intent and silence the PHPMD warning.♻️ Suggested improvement
private function createMockMigration(string $version, string $description): MigrationInterface { $migration = $this->createMock(MigrationInterface::class); $migration->method('getVersion')->willReturn($version); $migration->method('getDescription')->willReturn($description); $migration ->method('up') - ->willReturnCallback(function (OperationRecorder $recorder) { - // Empty migration - no operations + ->willReturnCallback(function (OperationRecorder $recorder): void { + // Intentionally empty - simulates a migration with no operations + unset($recorder); }); return $migration; }tests/phpMyFAQ/Setup/Migration/MigrationTrackerTest.php (1)
17-26: Missing test coverage forensureTableExists()method.The test suite doesn't include tests for the
ensureTableExists()method, which contains database-specific logic. Consider adding tests to verify proper DDL generation for each supported database type.Would you like me to generate test cases for
ensureTableExists()covering the different database types?phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/DirectoryCopyOperation.php (2)
56-68: Consider logging or exposing failure details.Similar to
SqlOperation, theexecute()method silently catches exceptions. For filesystem operations, knowing the specific error (permission denied, disk full, etc.) is valuable for troubleshooting.
81-88: Minor:shortenPathcould handle edge cases better.If
PMF_ROOT_DIRends without a separator and the path starts right after the prefix, the result may have a leading slash or no separator. This is cosmetic for display purposes but could be cleaner.♻️ Optional improvement
private function shortenPath(string $path): string { // Remove common prefixes to shorten the path for display if (defined('PMF_ROOT_DIR')) { - $path = str_replace(PMF_ROOT_DIR, '', $path); + $path = str_replace(PMF_ROOT_DIR . DIRECTORY_SEPARATOR, '', $path); + $path = str_replace(PMF_ROOT_DIR, '', $path); } - return $path; + return ltrim($path, DIRECTORY_SEPARATOR); }phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/SqlOperation.php (1)
84-92: Add error logging to failed database operations.The
execute()method silently swallows exceptions and returns false, making it difficult to debug failed migrations. Capture the exception message and log it for visibility into what went wrong.♻️ Suggested improvement
public function execute(): bool { try { $this->configuration->getDb()->query($this->query); return true; - } catch (Exception) { + } catch (Exception $e) { + error_log('SQL migration failed: ' . $e->getMessage()); return false; } }phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/OperationRecorder.php (1)
159-164: Avoid empty entries ingetSqlQueries().
If a custom operation reports typesqlbut isn’tSqlOperation, the current mapping inserts''into the list. Filtering byinstanceofavoids silent empties.♻️ Suggested refactor
- return array_map(static fn(OperationInterface $op) => $op instanceof SqlOperation - ? $op->getQuery() - : '', $this->getOperationsByType('sql')); + $sqlOps = array_filter( + $this->operations, + static fn(OperationInterface $op) => $op instanceof SqlOperation, + ); + return array_map(static fn(SqlOperation $op) => $op->getQuery(), $sqlOps);phpmyfaq/src/phpMyFAQ/Setup/Update.php (2)
343-355: Preserve exception context when rethrowing.
Re-wrapping loses the original stack trace; rethrow the same exception or chain it as the previous.♻️ Suggested change
- } catch (Exception $exception) { - throw new Exception($exception->getMessage()); + } catch (Exception $exception) { + throw $exception; }
44-86: Simplify trivial property hooks.
PHP 8.4 property hooks are correctly supported percomposer.json(php >= 8.4.0). Inside a hook,$this->propertyaccesses the backing storage directly, preventing recursion. However, these hooks are trivial pass-throughs (setters that simply assign, getters that simply return) and add no value. Consider removing them unless they will later enforce validation, transformation, or lazy initialization.💡 Example simplification
- public string $version { - set { - $this->version = $value; - } - } + public string $version = '';
| } | ||
|
|
||
| return sprintf('CREATE INDEX IF NOT EXISTS %s ON %s (%s)', $indexName, $tableName, $columnList); | ||
| } |
There was a problem hiding this comment.
MySQL does not support IF NOT EXISTS for CREATE INDEX.
The generated SQL CREATE INDEX IF NOT EXISTS is valid for PostgreSQL and SQLite but will fail on MySQL/MariaDB. Consider adding MySQL-specific handling.
🐛 Suggested fix
protected function createIndex(string $table, string $indexName, string|array $columns): string
{
$tableName = $this->table($table);
$columnList = is_array($columns) ? implode(', ', $columns) : $columns;
if ($this->isSqlServer()) {
return sprintf(
"IF NOT EXISTS (SELECT * FROM sys.indexes WHERE name = '%s') " . 'CREATE INDEX %s ON %s (%s)',
$indexName,
$indexName,
$tableName,
$columnList,
);
}
+ if ($this->isMySql()) {
+ // MySQL doesn't support IF NOT EXISTS for CREATE INDEX
+ // Use a procedure or handle duplicates at application level
+ return sprintf('CREATE INDEX %s ON %s (%s)', $indexName, $tableName, $columnList);
+ }
+
return sprintf('CREATE INDEX IF NOT EXISTS %s ON %s (%s)', $indexName, $tableName, $columnList);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } | |
| return sprintf('CREATE INDEX IF NOT EXISTS %s ON %s (%s)', $indexName, $tableName, $columnList); | |
| } | |
| protected function createIndex(string $table, string $indexName, string|array $columns): string | |
| { | |
| $tableName = $this->table($table); | |
| $columnList = is_array($columns) ? implode(', ', $columns) : $columns; | |
| if ($this->isSqlServer()) { | |
| return sprintf( | |
| "IF NOT EXISTS (SELECT * FROM sys.indexes WHERE name = '%s') " . 'CREATE INDEX %s ON %s (%s)', | |
| $indexName, | |
| $indexName, | |
| $tableName, | |
| $columnList, | |
| ); | |
| } | |
| if ($this->isMySql()) { | |
| // MySQL doesn't support IF NOT EXISTS for CREATE INDEX | |
| // Use a procedure or handle duplicates at application level | |
| return sprintf('CREATE INDEX %s ON %s (%s)', $indexName, $tableName, $columnList); | |
| } | |
| return sprintf('CREATE INDEX IF NOT EXISTS %s ON %s (%s)', $indexName, $tableName, $columnList); | |
| } |
🤖 Prompt for AI Agents
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/AbstractMigration.php` around lines 170
- 173, The CREATE INDEX SQL currently includes "IF NOT EXISTS" which breaks on
MySQL/MariaDB; update the method in AbstractMigration that returns
sprintf('CREATE INDEX IF NOT EXISTS %s ON %s (%s)', $indexName, $tableName,
$columnList) to emit a MySQL-safe path: when the connection/driver indicates
MySQL or MariaDB (use your existing DB connection/driver probe in
AbstractMigration), either return "CREATE INDEX %s ON %s (%s)" or, better,
perform an existence check against information_schema.STATISTICS for $tableName
and $indexName and only run the CREATE when the index is absent; modify the code
that constructs/executes the SQL so the driver branch for MySQL/MariaDB uses the
existence-check logic instead of the "IF NOT EXISTS" form.
phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigRenameOperation.php
Outdated
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Beta.php
Outdated
Show resolved
Hide resolved
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @thorsten. * #3906 (comment) The following files were modified: * `phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/SetupController.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/AbstractMigration.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationExecutor.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationInterface.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationRegistry.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationResult.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationTracker.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigAddOperation.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigDeleteOperation.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigRenameOperation.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigUpdateOperation.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/DirectoryCopyOperation.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/FileCopyOperation.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/OperationInterface.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/OperationRecorder.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/PermissionGrantOperation.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/SqlOperation.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/AlterTableBuilder.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/MysqlDialect.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/PostgresDialect.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/SqlServerDialect.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/SqliteDialect.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/DialectFactory.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/DialectInterface.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/QueryBuilder.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/TableBuilder.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Alpha.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Beta.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Beta2.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320RC.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration323.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Alpha.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Alpha2.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Alpha3.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Beta2.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration405.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration407.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration409.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration410Alpha.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration410Alpha2.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration410Alpha3.php` * `phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php` * `phpmyfaq/src/phpMyFAQ/Setup/Update.php` * `phpmyfaq/src/phpMyFAQ/Setup/UpdateRunner.php`
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationExecutor.php`:
- Around line 299-306: In truncateQuery, preg_replace can return null which will
cause strlen to throw a TypeError; after calling preg_replace on trim($query)
assign its result to a local variable (e.g., $sanitized) and guard it with a
fallback to an empty string (use the null coalescing operator or an explicit
null check) before calling strlen/substr; then use that sanitized string for the
length check and truncation so strlen/substr always receive a string.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/AlterTableBuilder.php`:
- Around line 65-74: The addVarchar method builds a default SQL literal without
escaping embedded quotes, which can break the generated SQL; update addVarchar
to escape single quotes in $default (e.g. replace ' with '' per SQL string
literal rules) before wrapping it in single quotes and passing it to addColumn,
so the defaultVal passed to addColumn('ADD', $name,
$this->dialect->varchar($length), $nullable, $defaultVal, $after) is a safe,
escaped SQL string literal (handle null unchanged).
- Around line 144-179: The build() method in AlterTableBuilder accesses
$this->tableName without ensuring it was set by table(), which causes an
uninitialized property error; add an explicit validation at the start of build()
that checks $this->tableName (or empty string) and throws a clear exception
(e.g., InvalidArgumentException or RuntimeException) with a message telling the
caller to call table() first, so callers get a descriptive failure instead of an
uninitialized property error.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/TableBuilder.php`:
- Around line 104-108: The varchar() and char() methods build SQL default
literals without escaping single quotes, breaking DDL for values like O'Reilly;
update TableBuilder::varchar(string $name, int $length, bool $nullable = true,
?string $default = null) and TableBuilder::char(...) to escape any single quotes
in $default before wrapping it in quotes (e.g., $escaped = str_replace("'",
"''", $default)) and then pass the quoted escaped value to addColumn so the
generated SQL uses proper SQL-standard escaped single quotes.
- Around line 237-245: The build() method in TableBuilder already inlines
indexes for MySQL, so buildIndexStatements() must skip emitting separate CREATE
INDEX statements when the dialect is MySQL; modify the buildIndexStatements()
method to detect MySQL (use the same check as build(): $isMysql =
in_array($this->dialect->getType(), ['mysqli', 'pdo_mysql'], true)) and return
an empty array or no statements when $isMysql is true, otherwise proceed to
generate index statements from $this->indexes as before; update references to
buildIndexStatements() callers if they expect an array to ensure compatibility.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Beta.php`:
- Around line 122-126: The private method rebuildTableWithoutColumns currently
declares an unused parameter $columnsToRemove which misleads callers; remove the
parameter from rebuildTableWithoutColumns (and its type OperationRecorder
reference remains) and update every call site that passes $columnsToRemove to
call rebuildTableWithoutColumns($recorder, $tableName) for the specific faqdata
and faqdata_revisions flows; alternatively, if you prefer to keep the signature,
document the parameter as intentionally ignored and add a suppress/annotation
comment explaining that the method uses hardcoded schema definitions for faqdata
and faqdata_revisions due to SQLite limitations—use the function name
rebuildTableWithoutColumns and OperationRecorder to locate the method and adjust
all callers accordingly.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration323.php`:
- Around line 90-94: In Migration323 update the SQL Server branch where
$recorder->addSql(...) is called: modify the ALTER TABLE command string produced
by sprintf in the isSqlServer() block so the ALTER COLUMN includes explicit
nullability (add " NULL") to match the existing nullable faquser.ip column and
SQL Server syntax requirements; ensure you keep the existing table prefix
sprintf usage and the same $recorder->addSql call and description text.
- Around line 54-89: The SQLite branch in Migration323 (inside the isSqlite()
block) uses "INSERT INTO %sfaquser_new SELECT * FROM %sfaquser" which is
brittle; change the recorder->addSql INSERT to use an explicit column list that
matches the CREATE TABLE definition to guarantee correct column mapping. Update
the INSERT to specify the columns: user_id, login, session_id,
session_timestamp, ip, account_status, last_login, auth_source, member_since,
remember_me, success, is_superadmin, login_attempts, refresh_token,
access_token, code_verifier, jwt in that order for both the INSERT INTO column
list and the SELECT list from the old %sfaquser; leave the CREATE, DROP and
RENAME recorder->addSql calls unchanged.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Alpha.php`:
- Around line 162-169: In Migration400Alpha, the SQL Server branch (checked by
isSqlServer()) uses an invalid T-SQL form "ADD COLUMN"; update the
$recorder->addSql call that builds the ALTER TABLE for %sfaqcategory_order (uses
$this->tablePrefix) to remove the "COLUMN" keyword so the statement reads ALTER
TABLE ... ADD parent_id INTEGER DEFAULT NULL; keep the same descriptive message
string ('Add parent_id column to faqcategory_order (SQL Server)') and no other
changes.
In `@phpmyfaq/src/phpMyFAQ/Setup/Update.php`:
- Around line 67-77: The getters for MigrationRegistry and MigrationTracker
create infinite recursion because they return the same property names inside
their get accessors; fix by either removing the custom get hooks and declaring
the properties with a private setter (e.g., make migrationRegistry and
migrationTracker writable only inside the class via a private set) or introduce
distinct backing fields (e.g., rename internal storage to _migrationRegistry and
_migrationTracker) and have the get accessors return those backing fields;
update any construction/assignment sites that set these properties (where
MigrationRegistry, migrationRegistry, MigrationTracker, migrationTracker, and
MigrationExecutor are referenced) to use the chosen approach so getters no
longer call themselves.
In `@phpmyfaq/src/phpMyFAQ/Setup/UpdateRunner.php`:
- Around line 223-230: In truncateString, preg_replace can return null on PCRE
error which then causes strlen/substr to emit warnings; ensure the result of
preg_replace is converted to a safe string (e.g. fall back to an empty string)
before calling strlen/substr and keep the existing trim and whitespace collapse
logic; update the truncateString function to check or coalesce the preg_replace
return (using a null coalescing or explicit check) so strlen/substr operate on a
valid string.
♻️ Duplicate comments (1)
phpmyfaq/src/phpMyFAQ/Setup/UpdateRunner.php (1)
408-437: Version parameter mismatch has been resolved.The previous review correctly identified that
taskUpdateDatabasewas passing the target version instead of the installed version. This fix now uses$this->installedVersion(captured duringtaskUpdateCheck), which aligns with the dry-run path and ensuresgetPendingMigrations()identifies the correct migrations to apply.
🧹 Nitpick comments (8)
phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/AlterTableBuilder.php (1)
184-222: Consider validating dialect compatibility for MySQL-only method.The docblock states this is "MySQL only", but the method doesn't enforce this. If used with PostgreSQL/SQLite/SQLServer, the generated
MODIFYsyntax (line 212) will produce invalid SQL. Consider adding a runtime check or documenting the consequence more prominently.Proposed dialect validation
public function buildCombined(): string { if ($this->alterations === []) { throw new LogicException('No alterations defined for combined ALTER TABLE statement.'); } + + if (!isset($this->tableName)) { + throw new LogicException('Table name must be set before building ALTER TABLE statements.'); + } $parts = [];phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Alpha.php (2)
93-110: Consider adding a constraint to prevent duplicate bookmarks.The
faqbookmarkstable lacks a PRIMARY KEY or UNIQUE constraint on(userid, faqid). Without this, the same user could bookmark the same FAQ multiple times, which is likely unintended behavior.♻️ Suggested fix
// Bookmarks support if ($this->isMySql()) { $recorder->addSql( sprintf( - 'CREATE TABLE %sfaqbookmarks (userid int(11) DEFAULT NULL, faqid int(11) DEFAULT NULL)', + 'CREATE TABLE %sfaqbookmarks (userid int(11) NOT NULL, faqid int(11) NOT NULL, PRIMARY KEY (userid, faqid))', $this->tablePrefix, ), 'Create bookmarks table (MySQL)', ); } else { $recorder->addSql( sprintf( - 'CREATE TABLE %sfaqbookmarks (userid INTEGER DEFAULT NULL, faqid INTEGER DEFAULT NULL)', + 'CREATE TABLE %sfaqbookmarks (userid INTEGER NOT NULL, faqid INTEGER NOT NULL, PRIMARY KEY (userid, faqid))', $this->tablePrefix, ), 'Create bookmarks table', ); }
147-148: Multi-line string contains embedded newline and whitespace.The
botIgnoreListconfig value includes a literal newline and leading whitespace between the two lines, which may cause bot matching issues or display problems. Consider using string concatenation or a single line.♻️ Suggested fix
// Blocked statistics browsers - $recorder->addConfig('main.botIgnoreList', 'nustcrape,webpost,GoogleBot,msnbot,crawler,scooter, - bravobrian,archiver,w3c,controler,wget,bot,spider,Yahoo! Slurp,htdig,gsa-crawler,AirControler,Uptime-Kuma'); + $recorder->addConfig( + 'main.botIgnoreList', + 'nustcrape,webpost,GoogleBot,msnbot,crawler,scooter,bravobrian,archiver,w3c,controler,wget,bot,spider,Yahoo! Slurp,htdig,gsa-crawler,AirControler,Uptime-Kuma' + );phpmyfaq/src/phpMyFAQ/Setup/UpdateRunner.php (2)
98-221: Consider extracting operation-type display blocks into helper methods.This method handles 8 different operation types, making it ~120 lines. Extracting each operation-type display (e.g.,
displaySqlOperations,displayConfigOperations,displayFileOperations) would improve readability and make individual sections testable.
260-261: Properties declared after methods.Per PER Coding Style 3.0, properties should be declared before methods. Both
$versionand$installedVersionare positioned after helper methods. Consider moving them to the top of the class with other class members.tests/phpMyFAQ/Setup/Migration/Versions/Migration320AlphaTest.php (1)
67-85: Test assertion is ineffective - callback always returns true.The test
testUpAddsSqlForBackupTableclaims to verify that SQL for a backup table is generated, but the callback unconditionally returnstrueon line 81, making the assertion meaningless. The$sqlStatementsarray is populated but never used for verification.Consider refactoring to actually assert the presence of backup table SQL:
♻️ Proposed fix
public function testUpAddsSqlForBackupTable(): void { $recorder = $this->createMock(OperationRecorder::class); + $capturedStatements = []; $recorder ->expects($this->atLeastOnce()) ->method('addSql') - ->with($this->callback(function ($sql) { - static $sqlStatements = []; - $sqlStatements[] = $sql; - // Check that at least one SQL contains backup table creation - if (str_contains($sql, 'faqbackup')) { - return true; - } - return true; - })); + ->willReturnCallback(function ($sql) use (&$capturedStatements) { + $capturedStatements[] = $sql; + }); $this->migration->up($recorder); + + $hasBackupSql = array_any($capturedStatements, fn($sql) => str_contains($sql, 'faqbackup')); + $this->assertTrue($hasBackupSql, 'Expected at least one SQL statement to reference faqbackup table'); }phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Beta.php (1)
109-114: UseVARCHAR(MAX)instead of deprecatedTEXTtype in SQL Server.The
TEXTdata type is deprecated in SQL Server and will be removed in a future version. Microsoft recommends usingVARCHAR(MAX)instead.Suggested fix
} elseif ($this->isSqlServer()) { $recorder->addSql( - sprintf('ALTER TABLE %sfaqconfig ALTER COLUMN config_value TEXT', $this->tablePrefix), + sprintf('ALTER TABLE %sfaqconfig ALTER COLUMN config_value VARCHAR(MAX)', $this->tablePrefix), 'Change faqconfig.config_value to TEXT (SQL Server)', ); }phpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationTracker.php (1)
145-171: Consider preserving NULL execution times (schema allows NULL).Casting to
intturns NULL into0, which can conflate “unknown” with “zero.” If you want that distinction, returnint|nullhere (or alternatively make the column NOT NULL with a default of 0).♻️ Proposed tweak
- * `@return` array<int, array{version: string, applied_at: string, execution_time_ms: int, checksum: string|null, description: string|null}> + * `@return` array<int, array{version: string, applied_at: string, execution_time_ms: int|null, checksum: string|null, description: string|null}> @@ - 'execution_time_ms' => (int) $row->execution_time_ms, + 'execution_time_ms' => $row->execution_time_ms !== null ? (int) $row->execution_time_ms : null,
phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/AlterTableBuilder.php
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/AlterTableBuilder.php
Show resolved
Hide resolved
| } elseif ($this->isSqlite()) { | ||
| // SQLite requires table rebuild | ||
| $recorder->addSql(sprintf( | ||
| 'CREATE TABLE %sfaquser_new ( | ||
| user_id INTEGER NOT NULL, | ||
| login VARCHAR(128) NOT NULL, | ||
| session_id VARCHAR(150) NULL, | ||
| session_timestamp INTEGER NULL, | ||
| ip VARCHAR(64) NULL, | ||
| account_status VARCHAR(50) NULL, | ||
| last_login VARCHAR(14) NULL, | ||
| auth_source VARCHAR(100) NULL, | ||
| member_since VARCHAR(14) NULL, | ||
| remember_me VARCHAR(150) NULL, | ||
| success INTEGER NULL DEFAULT 1, | ||
| is_superadmin INTEGER NULL DEFAULT 0, | ||
| login_attempts INTEGER NULL DEFAULT 0, | ||
| refresh_token TEXT NULL DEFAULT NULL, | ||
| access_token TEXT NULL DEFAULT NULL, | ||
| code_verifier VARCHAR(255) NULL DEFAULT NULL, | ||
| jwt TEXT NULL DEFAULT NULL, | ||
| PRIMARY KEY (user_id))', | ||
| $this->tablePrefix, | ||
| ), 'Create new faquser table with larger IP column (SQLite)'); | ||
|
|
||
| $recorder->addSql( | ||
| sprintf('INSERT INTO %sfaquser_new SELECT * FROM %sfaquser', $this->tablePrefix, $this->tablePrefix), | ||
| 'Copy data to new faquser table (SQLite)', | ||
| ); | ||
|
|
||
| $recorder->addSql(sprintf('DROP TABLE %sfaquser', $this->tablePrefix), 'Drop old faquser table (SQLite)'); | ||
|
|
||
| $recorder->addSql( | ||
| sprintf('ALTER TABLE %sfaquser_new RENAME TO %sfaquser', $this->tablePrefix, $this->tablePrefix), | ||
| 'Rename new faquser table (SQLite)', | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for faquser table definition in setup files and migrations
fd -t f \( -name "*.sql" -o -name "*.php" \) | head -20Repository: thorsten/phpMyFAQ
Length of output: 291
🏁 Script executed:
# Find all references to faquser table creation
rg -n "CREATE TABLE.*faquser" --type php --type sqlRepository: thorsten/phpMyFAQ
Length of output: 4639
🏁 Script executed:
# Search for existing indexes on faquser
rg -n "CREATE.*INDEX.*faquser|faquser.*INDEX" --type php --type sqlRepository: thorsten/phpMyFAQ
Length of output: 43
🏁 Script executed:
# List migration files to understand the sequence
fd -t f "Migration.*\.php$" phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/ | sortRepository: thorsten/phpMyFAQ
Length of output: 1144
🏁 Script executed:
# Check the current/initial faquser schema definition
find . -name "*.sql" -type f -exec grep -l "faquser" {} \;Repository: thorsten/phpMyFAQ
Length of output: 43
🏁 Script executed:
# Look at Migration323 in context and check SQL Server branch
cat -n phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration323.php | sed -n '1,150p'Repository: thorsten/phpMyFAQ
Length of output: 4385
🏁 Script executed:
# Read the actual faquser schema from Sqlite3.php
sed -n '308,327p' phpmyfaq/src/phpMyFAQ/Instance/Database/Sqlite3.phpRepository: thorsten/phpMyFAQ
Length of output: 945
🏁 Script executed:
# Read the actual faquser schema from Sqlsrv.php
sed -n '308,327p' phpmyfaq/src/phpMyFAQ/Instance/Database/Sqlsrv.phpRepository: thorsten/phpMyFAQ
Length of output: 956
🏁 Script executed:
# Check for PRIMARY KEY or constraints on faquser in setup/schema
rg "PRIMARY KEY|UNIQUE|FOREIGN KEY|INDEX" phpmyfaq/src/phpMyFAQ/Instance/Database/ -A 2 -B 2 | grep -A 3 -B 3 "faquser"Repository: thorsten/phpMyFAQ
Length of output: 16775
🏁 Script executed:
# Check if there are any triggers defined for faquser
rg "CREATE TRIGGER|faquser.*trigger" --type php --type sqlRepository: thorsten/phpMyFAQ
Length of output: 43
🏁 Script executed:
# Check when webauthnkeys was added to the schema
rg "webauthnkeys" phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/ --type phpRepository: thorsten/phpMyFAQ
Length of output: 425
🏁 Script executed:
# Check if there are any other recent migrations that modified faquser
rg "faquser" phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/ --type php -lRepository: thorsten/phpMyFAQ
Length of output: 262
🏁 Script executed:
# Read the previous migration (320RC) to see when webauthnkeys was added
cat -n phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320RC.php | head -150Repository: thorsten/phpMyFAQ
Length of output: 1564
🏁 Script executed:
# Check initial ip column definition in Migration320Alpha
rg "ip" phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Alpha.php -A 5 -B 5 | grep -A 5 -B 5 "faquser"Repository: thorsten/phpMyFAQ
Length of output: 43
🏁 Script executed:
# Check the full PdoSqlsrv.php and Sqlsrv.php for exact ip column definition
sed -n '307,327p' phpmyfaq/src/phpMyFAQ/Instance/Database/Sqlsrv.phpRepository: thorsten/phpMyFAQ
Length of output: 1010
Fix SQL Server ALTER COLUMN syntax and data type mismatch.
The SQL Server branch (line 92) uses incorrect syntax and wrong data type. The ALTER COLUMN statement must explicitly specify NULL and use NVARCHAR to match the existing schema:
- $recorder->addSql(
- sprintf('ALTER TABLE %sfaquser ALTER COLUMN ip VARCHAR(64)', $this->tablePrefix),
- 'Increase faquser.ip column size (SQL Server)',
- );
+ $recorder->addSql(
+ sprintf('ALTER TABLE %sfaquser ALTER COLUMN ip NVARCHAR(64) NULL', $this->tablePrefix),
+ 'Increase faquser.ip column size (SQL Server)',
+ );The SQLite branch correctly hardcodes the schema for version 3.2.3. However, using explicit column lists in the INSERT statement is a defensive best practice to prevent data mapping errors if columns are added in the future.
Safer SQLite insert with explicit columns
- $recorder->addSql(
- sprintf('INSERT INTO %sfaquser_new SELECT * FROM %sfaquser', $this->tablePrefix, $this->tablePrefix),
- 'Copy data to new faquser table (SQLite)',
- );
+ $recorder->addSql(
+ sprintf(
+ 'INSERT INTO %sfaquser_new (user_id, login, session_id, session_timestamp, ip, account_status, last_login, auth_source, member_since, remember_me, success, is_superadmin, login_attempts, refresh_token, access_token, code_verifier, jwt) ' .
+ 'SELECT user_id, login, session_id, session_timestamp, ip, account_status, last_login, auth_source, member_since, remember_me, success, is_superadmin, login_attempts, refresh_token, access_token, code_verifier, jwt FROM %sfaquser',
+ $this->tablePrefix,
+ $this->tablePrefix,
+ ),
+ 'Copy data to new faquser table (SQLite)',
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } elseif ($this->isSqlite()) { | |
| // SQLite requires table rebuild | |
| $recorder->addSql(sprintf( | |
| 'CREATE TABLE %sfaquser_new ( | |
| user_id INTEGER NOT NULL, | |
| login VARCHAR(128) NOT NULL, | |
| session_id VARCHAR(150) NULL, | |
| session_timestamp INTEGER NULL, | |
| ip VARCHAR(64) NULL, | |
| account_status VARCHAR(50) NULL, | |
| last_login VARCHAR(14) NULL, | |
| auth_source VARCHAR(100) NULL, | |
| member_since VARCHAR(14) NULL, | |
| remember_me VARCHAR(150) NULL, | |
| success INTEGER NULL DEFAULT 1, | |
| is_superadmin INTEGER NULL DEFAULT 0, | |
| login_attempts INTEGER NULL DEFAULT 0, | |
| refresh_token TEXT NULL DEFAULT NULL, | |
| access_token TEXT NULL DEFAULT NULL, | |
| code_verifier VARCHAR(255) NULL DEFAULT NULL, | |
| jwt TEXT NULL DEFAULT NULL, | |
| PRIMARY KEY (user_id))', | |
| $this->tablePrefix, | |
| ), 'Create new faquser table with larger IP column (SQLite)'); | |
| $recorder->addSql( | |
| sprintf('INSERT INTO %sfaquser_new SELECT * FROM %sfaquser', $this->tablePrefix, $this->tablePrefix), | |
| 'Copy data to new faquser table (SQLite)', | |
| ); | |
| $recorder->addSql(sprintf('DROP TABLE %sfaquser', $this->tablePrefix), 'Drop old faquser table (SQLite)'); | |
| $recorder->addSql( | |
| sprintf('ALTER TABLE %sfaquser_new RENAME TO %sfaquser', $this->tablePrefix, $this->tablePrefix), | |
| 'Rename new faquser table (SQLite)', | |
| ); | |
| } elseif ($this->isSqlite()) { | |
| // SQLite requires table rebuild | |
| $recorder->addSql(sprintf( | |
| 'CREATE TABLE %sfaquser_new ( | |
| user_id INTEGER NOT NULL, | |
| login VARCHAR(128) NOT NULL, | |
| session_id VARCHAR(150) NULL, | |
| session_timestamp INTEGER NULL, | |
| ip VARCHAR(64) NULL, | |
| account_status VARCHAR(50) NULL, | |
| last_login VARCHAR(14) NULL, | |
| auth_source VARCHAR(100) NULL, | |
| member_since VARCHAR(14) NULL, | |
| remember_me VARCHAR(150) NULL, | |
| success INTEGER NULL DEFAULT 1, | |
| is_superadmin INTEGER NULL DEFAULT 0, | |
| login_attempts INTEGER NULL DEFAULT 0, | |
| refresh_token TEXT NULL DEFAULT NULL, | |
| access_token TEXT NULL DEFAULT NULL, | |
| code_verifier VARCHAR(255) NULL DEFAULT NULL, | |
| jwt TEXT NULL DEFAULT NULL, | |
| PRIMARY KEY (user_id))', | |
| $this->tablePrefix, | |
| ), 'Create new faquser table with larger IP column (SQLite)'); | |
| $recorder->addSql( | |
| sprintf( | |
| 'INSERT INTO %sfaquser_new (user_id, login, session_id, session_timestamp, ip, account_status, last_login, auth_source, member_since, remember_me, success, is_superadmin, login_attempts, refresh_token, access_token, code_verifier, jwt) ' . | |
| 'SELECT user_id, login, session_id, session_timestamp, ip, account_status, last_login, auth_source, member_since, remember_me, success, is_superadmin, login_attempts, refresh_token, access_token, code_verifier, jwt FROM %sfaquser', | |
| $this->tablePrefix, | |
| $this->tablePrefix, | |
| ), | |
| 'Copy data to new faquser table (SQLite)', | |
| ); | |
| $recorder->addSql(sprintf('DROP TABLE %sfaquser', $this->tablePrefix), 'Drop old faquser table (SQLite)'); | |
| $recorder->addSql( | |
| sprintf('ALTER TABLE %sfaquser_new RENAME TO %sfaquser', $this->tablePrefix, $this->tablePrefix), | |
| 'Rename new faquser table (SQLite)', | |
| ); |
🤖 Prompt for AI Agents
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration323.php` around lines
54 - 89, The SQLite branch in Migration323 (inside the isSqlite() block) uses
"INSERT INTO %sfaquser_new SELECT * FROM %sfaquser" which is brittle; change the
recorder->addSql INSERT to use an explicit column list that matches the CREATE
TABLE definition to guarantee correct column mapping. Update the INSERT to
specify the columns: user_id, login, session_id, session_timestamp, ip,
account_status, last_login, auth_source, member_since, remember_me, success,
is_superadmin, login_attempts, refresh_token, access_token, code_verifier, jwt
in that order for both the INSERT INTO column list and the SELECT list from the
old %sfaquser; leave the CREATE, DROP and RENAME recorder->addSql calls
unchanged.
| } elseif ($this->isSqlServer()) { | ||
| $recorder->addSql( | ||
| sprintf('ALTER TABLE %sfaquser ALTER COLUMN ip VARCHAR(64)', $this->tablePrefix), | ||
| 'Increase faquser.ip column size (SQL Server)', | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration323.php | sed -n '85,100p'Repository: thorsten/phpMyFAQ
Length of output: 633
🏁 Script executed:
rg -i "faquser.*ip" --type php | head -20Repository: thorsten/phpMyFAQ
Length of output: 1084
🏁 Script executed:
fd . -name "*.php" | xargs rg -l "CREATE TABLE.*faquser" | head -5Repository: thorsten/phpMyFAQ
Length of output: 579
🌐 Web query:
SQL Server ALTER COLUMN nullability requirement NOT NULL NULL syntax
💡 Result:
In SQL Server, changing a column’s nullability is done with ALTER TABLE ... ALTER COLUMN, and you must restate the column’s data type in the statement.
Make a column nullable
ALTER TABLE dbo.YourTable
ALTER COLUMN YourColumn <data_type> NULL;SQL Server allows NULL in ALTER COLUMN to change a NOT NULL column to allow nulls (except where disallowed by constraints such as a primary key). [1]
Make a column NOT NULL
ALTER TABLE dbo.YourTable
ALTER COLUMN YourColumn <data_type> NOT NULL;This will only succeed if the column currently contains no NULL values (you must update/fix them first). [1] A common pattern is: update NULLs → then alter to NOT NULL. [2]
Example
UPDATE dbo.YourTable
SET YourColumn = 'some_value'
WHERE YourColumn IS NULL;
ALTER TABLE dbo.YourTable
ALTER COLUMN YourColumn NVARCHAR(20) NOT NULL;[1]
Sources:
[1] ALTER TABLE / ALTER COLUMN reference showing NULL | NOT NULL behavior and examples: (documentation.help)
[2] Example workflow and syntax (must specify datatype) for NULL → NOT NULL: (atlassian.com)
Citations:
- 1: https://documentation.help/tsqlref/ts_aa-az_3ied.htm?utm_source=openai
- 2: https://www.atlassian.com/data/databases/how-to-alter-a-column-from-null-to-not-null-in-sql-server?utm_source=openai
SQL Server ALTER COLUMN requires explicit nullability.
The statement at line 92 omits NULL/NOT NULL, which SQL Server requires in ALTER COLUMN statements. The MySQL migration for the same column explicitly specifies NULL DEFAULT NULL, confirming the column is nullable. Add NULL to match both the existing column definition and SQL Server syntax requirements:
- sprintf('ALTER TABLE %sfaquser ALTER COLUMN ip VARCHAR(64)', $this->tablePrefix),
+ sprintf('ALTER TABLE %sfaquser ALTER COLUMN ip VARCHAR(64) NULL', $this->tablePrefix),🤖 Prompt for AI Agents
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration323.php` around lines
90 - 94, In Migration323 update the SQL Server branch where
$recorder->addSql(...) is called: modify the ALTER TABLE command string produced
by sprintf in the isSqlServer() block so the ALTER COLUMN includes explicit
nullability (add " NULL") to match the existing nullable faquser.ip column and
SQL Server syntax requirements; ensure you keep the existing table prefix
sprintf usage and the same $recorder->addSql call and description text.
| } elseif ($this->isSqlServer()) { | ||
| $recorder->addSql( | ||
| sprintf( | ||
| 'ALTER TABLE %sfaqcategory_order ADD COLUMN parent_id INTEGER DEFAULT NULL', | ||
| $this->tablePrefix, | ||
| ), | ||
| 'Add parent_id column to faqcategory_order (SQL Server)', | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
SQL Server ALTER TABLE ADD COLUMN syntax T-SQL
💡 Result:
SQL Server ALTER TABLE ... ADD column (T‑SQL) syntax
Add one column
ALTER TABLE schema_name.table_name
ADD column_name data_type [NULL | NOT NULL] [CONSTRAINT constraint_name] [DEFAULT default_value];[1]
Add multiple columns
ALTER TABLE schema_name.table_name
ADD
column1 data_type [NULL | NOT NULL] [DEFAULT ...],
column2 data_type [NULL | NOT NULL] [DEFAULT ...];[1]
Example (add a nullable column)
ALTER TABLE dbo.Products
ADD brand_id smallint NULL;[3]
Example (add a NOT NULL column with a default)
ALTER TABLE dbo.Products
ADD status varchar(20) NOT NULL CONSTRAINT DF_Products_status DEFAULT ('active');[1]
Note: SQL Server appends new columns to the end of the table definition (you can’t add “after” an existing column like some other DBs). [2]
Sources: Microsoft Learn ALTER TABLE (Transact-SQL) [1], SQLServerTutorial [2], PopSQL [3].
Fix SQL Server syntax: remove COLUMN keyword from ALTER TABLE ADD statement.
SQL Server T-SQL requires ALTER TABLE ... ADD column_name data_type, not ADD COLUMN. The current code will cause a syntax error on SQL Server migrations. Change line 165 from:
ADD COLUMN parent_id INTEGER DEFAULT NULL
to:
ADD parent_id INTEGER DEFAULT NULL
🤖 Prompt for AI Agents
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Alpha.php` around
lines 162 - 169, In Migration400Alpha, the SQL Server branch (checked by
isSqlServer()) uses an invalid T-SQL form "ADD COLUMN"; update the
$recorder->addSql call that builds the ALTER TABLE for %sfaqcategory_order (uses
$this->tablePrefix) to remove the "COLUMN" keyword so the statement reads ALTER
TABLE ... ADD parent_id INTEGER DEFAULT NULL; keep the same descriptive message
string ('Add parent_id column to faqcategory_order (SQL Server)') and no other
changes.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
phpmyfaq/src/phpMyFAQ/Setup/Update.php (1)
359-389: Hash chain can break on partial migration runs.
If an entry already has a hash,previousHashremains unchanged, so later un‑hashed entries get a null previous hash. Keep the chain by updatingpreviousHashwhen an existing hash is encountered.🐛 Suggested fix
- foreach ($entries as $entity) { - if ($entity->getHash() !== null) { - continue; - } + foreach ($entries as $entity) { + if ($entity->getHash() !== null) { + $previousHash = $entity->getHash(); + continue; + }
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/TableBuilder.php`:
- Around line 216-237: The build logic in TableBuilder emits an explicit
"PRIMARY KEY (...)" even when a column used AUTO_INCREMENT via
SqliteDialect::autoIncrement(), causing duplicate PRIMARY KEY clauses for
SQLite; update the TableBuilder->build() logic to detect when the dialect bakes
PRIMARY KEY into autoIncrement (e.g. by checking for any $this->columns with
type 'AUTO_INCREMENT' and/or $this->dialect being SqliteDialect) and in that
case skip adding the explicit PRIMARY KEY from $this->primaryKey (or else
validate and throw if both are set), ensuring you reference the TableBuilder
class, the $this->columns entries with 'AUTO_INCREMENT', the $this->primaryKey
field, and SqliteDialect::autoIncrement() when making the change.
In `@phpmyfaq/src/phpMyFAQ/Setup/UpdateRunner.php`:
- Around line 69-90: The runDryRun method assumes Update::applyUpdates always
throws on failure but applyUpdates can return false; update runDryRun to capture
the boolean result of $update->applyUpdates(), check if it is === false and in
that case call $symfonyStyle->error(...) with a clear message and return
Command::FAILURE; only when applyUpdates returns truthy should you proceed to
call $update->getDryRunResults(), displayDryRunReport and return
Command::SUCCESS, while keeping the existing try/catch around applyUpdates to
handle thrown exceptions as before.
In `@phpmyfaq/src/phpMyFAQ/Translation/Provider/DeepLTranslationProvider.php`:
- Around line 112-114: The catch block in DeepLTranslationProvider around the
toArray() call currently catches Exception and TransportExceptionInterface but
omits DecodingExceptionInterface (which toArray() can throw); update the catch
to also handle DecodingExceptionInterface (e.g., catch
Exception|TransportExceptionInterface|DecodingExceptionInterface $e) or add a
dedicated catch for DecodingExceptionInterface and rethrow the same
ApiException('DeepL API error: ' . $e->getMessage()), matching the handling used
in doTranslate to prevent unhandled decoding errors.
In `@tests/phpMyFAQ/Setup/Migration/QueryBuilder/AlterTableBuilderTest.php`:
- Line 8: The two `use` declarations are combined on one line; split them so
each `use` statement is on its own line (e.g., separate `use
PHPUnit\Framework\TestCase;` and `use RuntimeException;`) to conform to PER
Coding Style 3.0 and ensure `TestCase` and `RuntimeException` imports are each
declared on a single line.
🧹 Nitpick comments (7)
tests/phpMyFAQ/ApplicationTest.php (2)
53-62: Test name and reflection usage mismatch with new implementation.The test is named
testSetUrlMatcher, implying it tests a setter method, but it now tests direct property assignment. Additionally, ifurlMatcheris a public property (allowing direct assignment), using reflection to read it is unnecessary.Consider renaming to reflect the actual behavior and simplifying the assertion:
♻️ Suggested simplification
- public function testSetUrlMatcher(): void + public function testUrlMatcherPropertyAssignment(): void { $urlMatcher = $this->createStub(UrlMatcher::class); $this->application->urlMatcher = $urlMatcher; - $reflection = new ReflectionClass(Application::class); - $property = $reflection->getProperty('urlMatcher'); - - $this->assertSame($urlMatcher, $property->getValue($this->application)); + $this->assertSame($urlMatcher, $this->application->urlMatcher); }
64-73: Same naming and reflection concerns as the previous test.The test name
testSetControllerResolverno longer reflects the actual behavior being tested. The same simplification applies here.♻️ Suggested simplification
- public function testSetControllerResolver(): void + public function testControllerResolverPropertyAssignment(): void { $controllerResolver = $this->createStub(ControllerResolver::class); $this->application->controllerResolver = $controllerResolver; - $reflection = new ReflectionClass(Application::class); - $property = $reflection->getProperty('controllerResolver'); - - $this->assertSame($controllerResolver, $property->getValue($this->application)); + $this->assertSame($controllerResolver, $this->application->controllerResolver); }phpmyfaq/src/phpMyFAQ/Controller/Api/OpenQuestionController.php (1)
120-120: Note: Route naming convention.The route name
api_open_questionsuses underscores. Per coding guidelines, API routes should followapi.{resource}.{action}with dots (e.g.,api.open_questions.list). This is pre-existing and not introduced by this PR, so it can be addressed separately if desired.phpmyfaq/src/phpMyFAQ/Routing/RouteCacheManager.php (1)
84-88: TOCTOU race condition still present.The
file_exists()check beforeunlink()is defensive, but a race condition remains—another process could delete the file between the check andunlink(). Sinceglob()already returns only existing files, consider suppressing the potential warning directly:♻️ Suggested simplification
foreach ($cacheFiles as $file) { - if (!file_exists($file)) { - continue; - } - - unlink($file); + `@unlink`($file); }phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PageController.php (1)
170-176: Simplify required-field validation to avoid double negation.The
if (!(!isset(...) || ...)) continue;pattern is hard to read. A direct early-return check is clearer and equivalent.♻️ Proposed refactor
foreach ($requiredFields as $field) { - if (!(!isset($data->$field) || $data->$field === '')) { - continue; - } - - return $this->json(['error' => "Missing required field: $field"], Response::HTTP_BAD_REQUEST); + if (!isset($data->$field) || $data->$field === '') { + return $this->json(['error' => "Missing required field: $field"], Response::HTTP_BAD_REQUEST); + } }phpmyfaq/src/Bootstrap.php (1)
96-97: Minor: Redundant condition check.The
str_contains($requestUri, '/update')check already covers/update/paths, making the second condition redundant. However, this doesn't affect correctness.Note that
str_containsmay match unintended paths (e.g.,/content/update/file). If stricter matching is needed, consider using a regex or checking path prefix explicitly.♻️ Optional: Simplify and tighten the condition
-$isSetupContext = str_contains($requestUri, '/setup/') || str_contains($requestUri, '/api/setup/') || - str_contains($requestUri, '/update') || str_contains($requestUri, '/update/'); +$isSetupContext = str_contains($requestUri, '/setup/') || str_contains($requestUri, '/api/setup/') || + preg_match('#^/update(?:/|$)#', $requestUri);phpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationRegistry.php (1)
88-94: Guard against non‑migration classes during instantiation.
A typo in the registry list would currently instantiate arbitrary classes. Consider enforcing the interface to fail fast.♻️ Suggested guard
- foreach ($this->migrationClasses as $version => $className) { - if (!class_exists($className)) { + foreach ($this->migrationClasses as $version => $className) { + if (!class_exists($className) || !is_subclass_of($className, MigrationInterface::class)) { continue; }
phpmyfaq/src/phpMyFAQ/Translation/Provider/DeepLTranslationProvider.php
Outdated
Show resolved
Hide resolved
tests/phpMyFAQ/Setup/Migration/QueryBuilder/AlterTableBuilderTest.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
phpmyfaq/src/phpMyFAQ/Controller/Administration/FaqController.php (1)
242-263: Route change and POST data source look correct.The route now accepts both GET (initial load) and POST (revision selection form submission), and
$request->requestcorrectly retrieves POST body data.However, there's an inconsistency between lines 328 and 333:
- Line 328:
sprintf('%s 1.%d', ..., $selectedRevisionId)— uses$selectedRevisionIddirectly- Line 333:
$selectedRevisionId ?? $faqData['revision_id']— has fallbackOn GET requests,
$selectedRevisionIdwill benull(no POST data), causing line 328 to render "Revision 1.0" instead of the actual current revision.Proposed fix for consistency
- 'currentRevision' => sprintf('%s 1.%d', Translation::get(key: 'msgRevision'), $selectedRevisionId), + 'currentRevision' => sprintf('%s 1.%d', Translation::get(key: 'msgRevision'), $selectedRevisionId ?? $faqData['revision_id']),phpmyfaq/src/phpMyFAQ/Setup/UpdateRunner.php (1)
289-318: Exception handling returnsSUCCESSafter failure, masking errors.When an exception is caught on line 313, the error is displayed but
Command::SUCCESSis returned on line 317. This allows the update process to continue with an empty$installedVersion, which will causetaskUpdateDatabaseto use an incorrect (empty) version string for migrations.🐛 Proposed fix
} catch (Exception|TransportExceptionInterface|DecodingExceptionInterface $exception) { $symfonyStyle->error(message: 'Error during update check: ' . $exception->getMessage()); + return Command::FAILURE; } return Command::SUCCESS;
🧹 Nitpick comments (3)
tests/phpMyFAQ/Setup/Migration/QueryBuilder/AlterTableBuilderTest.php (1)
11-12: Consider removing the unused#[AllowMockObjectsWithoutExpectations]attribute.This test class doesn't create any mock objects, so this attribute has no effect. Removing it would reduce noise.
Suggested change
-#[AllowMockObjectsWithoutExpectations] class AlterTableBuilderTest extends TestCasephpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/TableBuilder.php (1)
212-215: Add a guard for callingbuild()beforetable().
Without a table name, the typed property is uninitialized and PHP throws. A clear exception improves failure mode (and consider mirroring this inbuildIndexStatements()).♻️ Proposed guard
public function build(): string { + if (!isset($this->tableName)) { + throw new \LogicException('Table name not set. Call table() before build().'); + } $parts = [];tests/phpMyFAQ/Setup/Migration/QueryBuilder/TableBuilderTest.php (1)
359-372: Consider adding a non‑SQLite regression test for autoIncrement + primaryKey.
This would catch cases where explicit PKs are inadvertently suppressed for MySQL/Postgres. Align expected output with each dialect’sautoIncrement()behavior.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.