refactor(update): improved update process #3906
Conversation
📝 WalkthroughWalkthroughAdds a full migration framework (registry, executor, tracker, operations, dialect-aware query builders), many concrete migrations (3.2.0 → 4.2.0‑alpha), rewires Update to use migrations with dry‑run/reporting, updates Application/entrypoints to use public properties, and includes extensive tests plus small code-quality tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI / Admin UI"
participant Runner as "UpdateRunner"
participant Updater as "Update"
participant Registry as "MigrationRegistry"
participant Executor as "MigrationExecutor"
participant Migration as "Migration"
participant Recorder as "OperationRecorder"
participant DB as "Database / Filesystem / Config"
CLI->>Runner: runDryRun(fromVersion)
Runner->>Updater: request dry‑run
Updater->>Registry: getPendingMigrations(fromVersion)
Registry-->>Updater: [Migration...]
Updater->>Executor: setDryRun(true)
Updater->>Executor: executeMigrations([migrations])
activate Executor
loop per migration
Executor->>Recorder: create recorder
Executor->>Migration: up(recorder)
Migration->>Recorder: record operations (SQL, config, files, perms)
Recorder-->>Executor: operations list
alt dry‑run
Executor->>Executor: simulate operations -> MigrationResult (dry)
else execute
Executor->>DB: execute operations
DB-->>Executor: success/failure
Executor->>MigrationTracker: recordMigration(...)
end
Executor-->>Updater: MigrationResult
end
deactivate Executor
Updater->>Runner: return formatted report/results
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
🚥 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
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
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.
thorsten
force-pushed
the
feat/rewrite-update-process
branch
from
862402d to
be13aad
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
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 142-152: The dropColumns method in AbstractMigration currently
emits a single MySQL-only ALTER TABLE ... DROP COLUMN a, b statement; change it
to emit one ALTER TABLE ... DROP COLUMN <col> statement per column and join them
into a single string (e.g. implode('; ', $parts)) so non-MySQL backends
(Postgres/SQLite) receive separate ALTER TABLE statements. Use
$this->table($table) for the table name and map each column to sprintf('ALTER
TABLE %s DROP COLUMN %s', $tableName, $col) inside dropColumns to build the
parts before joining.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigAddOperation.php`:
- Around line 54-57: The execute() method in ConfigAddOperation currently calls
$this->configuration->add($this->key, $this->value) but unconditionally returns
true, hiding failures; change execute() to return the boolean result of
Configuration::add() (i.e. return $this->configuration->add($this->key,
$this->value);) so the migration reflects the real success/failure of
Configuration::add() in the ConfigAddOperation::execute() method.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigDeleteOperation.php`:
- Around line 47-51: The execute() method in ConfigDeleteOperation currently
calls $this->configuration->delete($this->key) but ignores its boolean result;
change execute() to capture that return value from
Configuration::delete($this->key) and propagate it (return the boolean) so
failures are reported to the migration executor; locate the execute() method in
ConfigDeleteOperation and replace the unconditional "return true" with returning
the actual result of the delete call (or, if your error strategy requires, throw
a descriptive exception when delete returns false) to ensure failures are not
masked.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/DirectoryCopyOperation.php`:
- Around line 56-67: The execute() method in DirectoryCopyOperation ignores the
boolean return value of $this->filesystem->recursiveCopy and thus reports
success even when it returns false; change execute() (in
DirectoryCopyOperation::execute) to check the result of
$this->filesystem->recursiveCopy($this->source, $this->destination) and return
that boolean (or return false when it returns false) instead of always returning
true, while keeping the existing only-if-exists short-circuit
($this->onlyIfExists && !is_dir($this->source)) and still catching exceptions to
return false on thrown errors.
In
`@phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/PermissionGrantOperation.php`:
- Around line 60-70: The execute() method currently calls
User->perm->addRight(...) and proceeds to grantUserRight even if addRight()
returns a falsy/invalid id; change execute() so it validates $rightId
immediately after calling addRight() (e.g., check for === false or empty/invalid
value), and short‑circuits (return false or throw a descriptive exception and
log an error) without calling $user->perm->grantUserRight when $rightId is
invalid; reference the execute() method in PermissionGrantOperation and the
calls to User->perm->addRight and User->perm->grantUserRight when locating where
to add the guard and error handling.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/SqlOperation.php`:
- Around line 84-91: The execute() method in SqlOperation.php falsely returns
true even when $this->configuration->getDb()->query($this->query) returns false
and currently only catches Exception; change execute() to capture the actual
boolean result of query(), return that result (or false if the call fails), and
broaden the catch to \Throwable to handle non-Exception errors; in short, call
$this->configuration->getDb()->query(...), store its return value, return
(bool)$result, and catch \Throwable to return false on unrecoverable errors.
In
`@phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/SqlServerDialect.php`:
- Around line 118-128: The IF NOT EXISTS check in createIndex(string $indexName,
string $tableName, array $columns, bool $ifNotExists) currently queries
sys.indexes by name only and must be scoped to the target table; change the
predicate to also compare object_id = OBJECT_ID(N'<table>') so the check only
matches indexes on the given $tableName (e.g. WHERE name = '$indexName' AND
object_id = OBJECT_ID(N'$tableName')). Update the conditional branch in
createIndex to include this object_id filter and ensure $tableName is passed as
the full schema-qualified name (or properly quoted) when building the OBJECT_ID
call.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Alpha.php`:
- Around line 77-84: The CREATE TABLE SQL in Migration320Alpha.php uses a
lowercase literal "timestamp" which is incompatible with SQL Server; update the
$recorder->addSql call that creates "faqbackup" to use a dialect-safe column
type by replacing the raw "timestamp" with the migration helper (e.g. use
$this->timestampType(false) or the equivalent DATETIME token for SQL Server) so
the generated SQL is valid across platforms; modify the SQL string inside
$recorder->addSql(...) to interpolate the helper/type instead of the literal
"timestamp" to ensure cross-dialect compatibility.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Alpha3.php`:
- Around line 55-98: Migration uses DATETIME for MySQL but DATE for other
engines causing inconsistent timestamps; update Migration400Alpha3 so the
%sfaqseo "created" column is a timestamp/datetime type on all engines (change
the SQL in the recorder->addSql calls for SQL Server, SQLite and PostgreSQL
branches to use the engine-appropriate datetime type and default (e.g. SQL
Server DATETIME/DATETIME2 with GETDATE(), SQLite DATETIME with datetime('now'),
PostgreSQL TIMESTAMP with CURRENT_TIMESTAMP) so SeoRepository continues to
instantiate equivalent DateTime values across databases.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration405.php`:
- Around line 75-83: The SQL used in Migration405.php is invalid for PostgreSQL:
update the two $recorder->addSql calls that target faqforms.input_label so the
type-change statement uses the correct PostgreSQL syntax (e.g. "ALTER COLUMN
input_label TYPE VARCHAR(500)" or "ALTER COLUMN input_label SET DATA TYPE
VARCHAR(500)") instead of "SET TYPE"; leave the NOT NULL statement ("ALTER
COLUMN input_label SET NOT NULL") as-is. Locate the $recorder->addSql
invocations in Migration405 and replace the faulty ALTER COLUMN clause
accordingly.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration410Alpha3.php`:
- Around line 89-116: Replace the raw SQL "CREATE INDEX IF NOT EXISTS" calls in
Migration410Alpha3 (the blocks using $recorder->addSql to create
idx_faqsearches_searchterm, idx_faqsearches_date_term and
idx_faqsearches_date_term_lang) with the migration helper that handles DB
differences; specifically call the existing AbstractMigration::createIndex (e.g.
$this->createIndex('faqsearches', 'idx_faqsearches_searchterm', 'searchterm'))
or implement explicit MySQL/MariaDB handling before issuing SQL so MySQL is not
given "CREATE INDEX IF NOT EXISTS" raw SQL—update all three index creations
accordingly and remove the IF NOT EXISTS SQL strings passed to
$recorder->addSql.
🟡 Minor comments (13)
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php-349-355 (1)
349-355:⚠️ Potential issue | 🟡 MinorUse strict
in_arrayto avoid loose‑comparison mismatches.
If a plugin name is numeric-like (e.g.,"0"), loose comparison can yield false positives/negatives. Using strict comparison prevents subtle dependency resolution bugs.🔧 Proposed fix
- if (in_array($dependency, $this->loadedPlugins)) { + if (in_array($dependency, $this->loadedPlugins, true)) { continue; }phpmyfaq/src/Bootstrap.php-95-97 (1)
95-97:⚠️ Potential issue | 🟡 MinorAvoid overly broad
/updatematching in setup-context detection.
str_contains($requestUri, '/update')also matches routes like/updatesor/foo/update-profile, which can bypass the installer redirect when config is missing and lead to a hard failure later in bootstrap. Consider matching the path segment precisely.🔧 Suggested fix (path-segment match)
$requestUri = $_SERVER['REQUEST_URI'] ?? ''; -$isSetupContext = str_contains($requestUri, '/setup/') || str_contains($requestUri, '/api/setup/') || - str_contains($requestUri, '/update') || str_contains($requestUri, '/update/'); +$path = parse_url($requestUri, PHP_URL_PATH) ?? ''; +$isSetupContext = + $path === '/setup' || str_starts_with($path, '/setup/') || + $path === '/api/setup' || str_starts_with($path, '/api/setup/') || + $path === '/update' || str_starts_with($path, '/update/');phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Alpha.php-147-148 (1)
147-148:⚠️ Potential issue | 🟡 MinorAvoid embedding newlines in
main.botIgnoreList.The newline/indentation becomes part of the stored value and can interfere with matching if consumers don’t trim tokens.
🧹 Proposed fix
- $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/Migration/Versions/Migration320Alpha.php-53-58 (1)
53-58:⚠️ Potential issue | 🟡 MinorSQLite does not support
INT(1)syntax.SQLite uses
INTEGERfor all integer types. TheINT(1)notation on line 55 is MySQL-specific and may cause issues on SQLite.🐛 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/AbstractMigration.php-186-229 (1)
186-229:⚠️ Potential issue | 🟡 Minor
indexExists()returns empty string for unsupported database types.When an unsupported database type is encountered, returning an empty string could lead to silent failures or unexpected behavior when the caller tries to execute the query.
🛡️ Suggested fix
if ($this->isSqlServer()) { return sprintf( 'SELECT COUNT(*) as idx_count FROM sys.indexes ' . "WHERE name = '%s' AND object_id = OBJECT_ID('%s')", $escapedIndexName, $escapedTableName, ); } - return ''; + throw new \RuntimeException("Unsupported database type for index existence check: {$this->dbType}"); }phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Alpha.php-109-116 (1)
109-116:⚠️ Potential issue | 🟡 Minor
DROP TABLEwithoutIF EXISTSmay fail if tables don't exist.If these tables have already been dropped or never existed, the migration will fail. Consider using
dropTableIfExists()helper.🐛 Suggested fix
// Remove section tables - $recorder->addSql(sprintf('DROP TABLE %sfaqsections', $this->tablePrefix), 'Drop faqsections table'); - $recorder->addSql( - sprintf('DROP TABLE %sfaqsection_category', $this->tablePrefix), - 'Drop faqsection_category table', - ); - $recorder->addSql(sprintf('DROP TABLE %sfaqsection_group', $this->tablePrefix), 'Drop faqsection_group table'); - $recorder->addSql(sprintf('DROP TABLE %sfaqsection_news', $this->tablePrefix), 'Drop faqsection_news table'); + $recorder->addSql($this->dropTableIfExists('faqsections'), 'Drop faqsections table'); + $recorder->addSql($this->dropTableIfExists('faqsection_category'), 'Drop faqsection_category table'); + $recorder->addSql($this->dropTableIfExists('faqsection_group'), 'Drop faqsection_group table'); + $recorder->addSql($this->dropTableIfExists('faqsection_news'), 'Drop faqsection_news table');phpmyfaq/src/phpMyFAQ/Setup/Migration/AbstractMigration.php-119-131 (1)
119-131:⚠️ Potential issue | 🟡 MinorDefault value is not escaped in
addColumn().The
$defaultparameter is directly interpolated into the SQL string without escaping, which could lead to SQL injection if untrusted input is passed.🐛 Suggested fix
protected function addColumn(string $table, string $column, string $type, ?string $default = null): string { $tableName = $this->table($table); - $defaultClause = $default !== null ? " DEFAULT $default" : ''; + $defaultClause = ''; + if ($default !== null) { + // For literal values like numbers, NULL, or function calls, use as-is + // For string values, they should be passed already quoted or as SQL expressions + $defaultClause = " DEFAULT $default"; + } if ($this->isSqlite()) { return sprintf('ALTER TABLE %s ADD COLUMN %s %s%s', $tableName, $column, $type, $defaultClause); }Consider documenting that
$defaultshould be passed as a SQL-safe expression (e.g.,'NULL','0',"'value'") or adding escaping for string defaults.phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration410Alpha.php-44-84 (1)
44-84:⚠️ Potential issue | 🟡 MinorHeredoc indentation will be included in the output.
The heredoc content has leading spaces that will be preserved in the final
robots.txtoutput. This may not be the intended behavior asrobots.txtfiles typically don't have leading whitespace.🐛 Suggested fix - remove indentation or use nowdoc
public function up(OperationRecorder $recorder): void { - $robotsText = <<<EOT - User-agent: Amazonbot - User-agent: anthropic-ai + $robotsText = <<<'EOT' +User-agent: Amazonbot +User-agent: anthropic-ai ... - Sitemap: /sitemap.xml - EOT; +Sitemap: /sitemap.xml +EOT; $recorder->addConfig('seo.contentRobotsText', $robotsText); }Note: PHP 7.3+ closing marker indentation (
EOT;) allows dedenting, but the content lines themselves retain their relative indentation from the opening<<<EOT.phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Alpha.php-98-99 (1)
98-99:⚠️ Potential issue | 🟡 MinorNo-op rename operation - same key for old and new.
renameConfig('security.loginWithEmailAddress', 'security.loginWithEmailAddress')renames the config key to itself, which is a no-op. This appears to be a mistake or placeholder.🐛 Suggested fix
If this was meant to rename to a different key:
- $recorder->renameConfig('security.loginWithEmailAddress', 'security.loginWithEmailAddress'); + $recorder->renameConfig('security.loginWithEmailAddress', 'security.enableEmailLogin');Or remove if not needed:
- // Update options - $recorder->renameConfig('security.loginWithEmailAddress', 'security.loginWithEmailAddress');tests/phpMyFAQ/Setup/Migration/MigrationTrackerTest.php-18-27 (1)
18-27:⚠️ Potential issue | 🟡 MinorMock static
Databasemethods or initialize them in test setup.
MigrationTrackerusesDatabase::getTablePrefix()andDatabase::getType()static methods. These access uninitialized global state:Database::$dbTypehas no default value and is only set viafactory(), andDatabase::$tablePrefixdefaults tonull. Without proper initialization in the test setup, these methods will either cause runtime errors or return inconsistent values, leading to flaky tests.phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php-81-84 (1)
81-84:⚠️ Potential issue | 🟡 MinorCREATE INDEX may fail on MySQL if index already exists.
The
CREATE INDEX idx_hashstatement at line 82 is executed unconditionally for all dialects, but MySQL doesn't supportIF NOT EXISTSfor CREATE INDEX. If this migration is re-run or the index already exists, it will fail on MySQL.Consider wrapping this in a MySQL-specific check or using a try-catch approach in the executor.
Suggested fix with dialect-specific handling
+ // Create hash index on faqadminlog + if ($this->isMySql()) { + // MySQL doesn't support IF NOT EXISTS for CREATE INDEX + // The index will be created only if it doesn't exist (handled by executor) + $recorder->addSql( + sprintf('CREATE INDEX idx_hash ON %sfaqadminlog (hash)', $this->tablePrefix), + 'Create hash index on faqadminlog (MySQL)', + ); + } else { $recorder->addSql( - sprintf('CREATE INDEX idx_hash ON %sfaqadminlog (hash)', $this->tablePrefix), + sprintf('CREATE INDEX IF NOT EXISTS idx_hash ON %sfaqadminlog (hash)', $this->tablePrefix), 'Create hash index on faqadminlog', ); + }tests/phpMyFAQ/Setup/Migration/Versions/Migration320AlphaTest.php-67-85 (1)
67-85:⚠️ Potential issue | 🟡 MinorTest callback always returns
true, making the assertion ineffective.The callback in
testUpAddsSqlForBackupTablechecks if$sqlcontains'faqbackup'but returnstruein both branches (lines 78-81). This means the test passes regardless of whether the backup table SQL is actually present.Proposed fix
$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; + // All SQL statements are valid for this test + return true; })); $this->migration->up($recorder); + + // Alternative: Add a separate assertion to verify backup table SQL existsIf the intent is to verify that at least one SQL statement contains
'faqbackup', consider using a different approach like capturing all calls and asserting afterward, or usingwillReturnCallbackto track the statements.tests/phpMyFAQ/Setup/Migration/MigrationExecutorTest.php-376-388 (1)
376-388:⚠️ Potential issue | 🟡 MinorSilence the PHPMD unused-parameter warning in the callback.
PHPMD flags$recorderas unused; a lightweight assertion avoids the warning without changing behavior.🧩 Suggested tweak
$migration ->method('up') - ->willReturnCallback(function (OperationRecorder $recorder) { - // Empty migration - no operations - }); + ->willReturnCallback(function (OperationRecorder $recorder) { + $this->assertInstanceOf(OperationRecorder::class, $recorder); + // Empty migration - no operations + });
🧹 Nitpick comments (21)
phpmyfaq/src/phpMyFAQ/Setup/UpdateRunner.php (3)
272-273: Consider moving property declarations to the class top.Per PER Coding Style 3.0, property declarations should appear before method definitions in the class body. These properties are currently placed after the
shortenPath()method.♻️ Suggested reorganization
Move lines 272-273 to after the constructor (around line 45):
final class UpdateRunner { + private string $version = ''; + private string $installedVersion = ''; + public function __construct( private readonly Configuration $configuration, private readonly System $system, ) { }And remove them from their current location (lines 272-273).
295-295: Consider adding parentheses for clarity.While
new DateTime()->format()works in PHP 8.0+ due to changed operator precedence, the conventional and clearer style is(new DateTime())->format().♻️ Suggested fix
- $dateLastChecked = new DateTime()->format(DateTimeInterface::ATOM); + $dateLastChecked = (new DateTime())->format(DateTimeInterface::ATOM);
332-332: Fix docblock closing formatting.The docblock closing
*/should be on its own line, not combined with the method signature.♻️ Suggested fix
- */ private function taskDownloadPackage(SymfonyStyle $symfonyStyle): int + */ + private function taskDownloadPackage(SymfonyStyle $symfonyStyle): intphpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Alpha.php (1)
141-141: Consider using a booleanfalsefor config consistency.If this setting is consumed as a boolean (like other flags in this migration), store it as
falserather than the string'false'.♻️ Proposed tweak (if boolean is expected)
- $recorder->addConfig('records.orderStickyFaqsCustom', 'false'); + $recorder->addConfig('records.orderStickyFaqsCustom', false);phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/FileCopyOperation.php (1)
81-87: Prefer prefix-only trimming for display paths.
str_replaceremoves all occurrences mid‑path; usestr_starts_withwithsubstrto safely remove only the leading prefix. The project's minimum PHP 8.4.0 supports both functions.🛠️ Suggested refactor
- if (defined('PMF_ROOT_DIR')) { - $path = str_replace(PMF_ROOT_DIR, '', $path); - } + if (defined('PMF_ROOT_DIR') && str_starts_with($path, PMF_ROOT_DIR)) { + $path = substr($path, strlen(PMF_ROOT_DIR)); + } return $path;phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/PostgresDialect.php (1)
102-122: Silence PHPMD unused-parameter warnings inaddColumn()/dropIndex().
$afterand$tableNameare unused due to PostgreSQL syntax; consider suppressing the warnings to keep static analysis clean.♻️ Suggested suppression
+ /** + * {`@inheritDoc`} + * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) + */ public function addColumn(string $tableName, string $columnName, string $type, ?string $after = null): string { // PostgreSQL doesn't support AFTER clause return "ALTER TABLE $tableName ADD COLUMN $columnName $type"; } @@ + /** + * {`@inheritDoc`} + * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) + */ public function dropIndex(string $indexName, string $tableName): string { return "DROP INDEX $indexName"; }phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/SqliteDialect.php (1)
102-124: Use parameters in the exception (or suppress) to avoid PHPMD noise.
modifyColumn()ignores its parameters due to SQLite limitations; it can include them in the exception message. The other unused parameters can be explicitly suppressed.♻️ Proposed cleanup
+ /** + * {`@inheritDoc`} + * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) + */ public function addColumn(string $tableName, string $columnName, string $type, ?string $after = null): string { // SQLite doesn't support AFTER clause return "ALTER TABLE $tableName ADD COLUMN $columnName $type"; } @@ public function modifyColumn(string $tableName, string $columnName, string $newType): string { // SQLite doesn't support ALTER COLUMN directly // This would require a table rebuild, which is handled separately - throw new \RuntimeException('SQLite does not support modifying columns. Use table rebuild pattern.'); + throw new \RuntimeException(sprintf( + 'SQLite does not support modifying columns (%s.%s -> %s). Use table rebuild pattern.', + $tableName, + $columnName, + $newType, + )); } @@ + /** + * {`@inheritDoc`} + * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) + */ public function dropIndex(string $indexName, string $tableName): string { return "DROP INDEX IF EXISTS $indexName"; }Also applies to: 108-113
phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/DialectFactory.php (1)
41-48: Optional: normalize$dbTypebefore matching.This avoids surprising mismatches if the DB type string ever includes different casing or whitespace.
♻️ Suggested tweak
public static function createForType(string $dbType): DialectInterface { - return match ($dbType) { + $dbType = strtolower(trim($dbType)); + return match ($dbType) { 'mysqli', 'pdo_mysql' => new MysqlDialect(), 'pgsql', 'pdo_pgsql' => new PostgresDialect(), 'sqlite3', 'pdo_sqlite' => new SqliteDialect(), 'sqlsrv', 'pdo_sqlsrv' => new SqlServerDialect(), default => throw new \InvalidArgumentException("Unsupported database type: $dbType"), }; }phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/QueryBuilder.php (1)
65-116: Extract a helper for prefixed table names to reduce repetition.Several methods duplicate the same
$withPrefixlogic. A small helper keeps this DRY and less error‑prone.♻️ Proposed refactor
- public function dropTable(string $tableName, bool $withPrefix = true): string - { - $fullName = $withPrefix ? $this->tablePrefix . $tableName : $tableName; - return "DROP TABLE $fullName"; - } + public function dropTable(string $tableName, bool $withPrefix = true): string + { + $fullName = $this->fullTableName($tableName, $withPrefix); + return "DROP TABLE $fullName"; + } @@ - $fullName = $withPrefix ? $this->tablePrefix . $tableName : $tableName; + $fullName = $this->fullTableName($tableName, $withPrefix); return "DROP TABLE IF EXISTS $fullName"; @@ - $fullTableName = $withPrefix ? $this->tablePrefix . $tableName : $tableName; + $fullTableName = $this->fullTableName($tableName, $withPrefix); return $this->dialect->createIndex($indexName, $fullTableName, (array) $columns, false); @@ - $fullTableName = $withPrefix ? $this->tablePrefix . $tableName : $tableName; + $fullTableName = $this->fullTableName($tableName, $withPrefix); return $this->dialect->createIndex($indexName, $fullTableName, (array) $columns, true); @@ - $fullTableName = $withPrefix ? $this->tablePrefix . $tableName : $tableName; + $fullTableName = $this->fullTableName($tableName, $withPrefix); return $this->dialect->dropIndex($indexName, $fullTableName); } + + private function fullTableName(string $tableName, bool $withPrefix): string + { + return $withPrefix ? $this->tablePrefix . $tableName : $tableName; + }phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration407.php (2)
62-73: Consider using theupdateLanguageCode()helper fromAbstractMigration.The parent class provides
updateLanguageCode()which handles escaping. The current implementation builds SQL strings directly, which bypasses this protection.♻️ Suggested refactor
// Update fr-ca to fr_ca foreach ($languageUpdates as $update) { $recorder->addSql( - sprintf( - "UPDATE %s%s SET %s='fr_ca' WHERE %s='fr-ca'", - $this->tablePrefix, - $update['table'], - $update['column'], - $update['column'], - ), + $this->updateLanguageCode($update['table'], $update['column'], 'fr-ca', 'fr_ca'), sprintf('Fix fr-ca -> fr_ca in %s.%s', $update['table'], $update['column']), ); }The same applies to the pt-br → pt_br updates at lines 85-96.
37-40: Minor: Description mentions uppercase codes but migration uses lowercase.The description says "Fix language codes for fr_CA and pt_BR" (uppercase regional codes), but the actual SQL updates use lowercase
fr_caandpt_br. Ensure this is intentional for consistency with the codebase conventions.phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration410Alpha3.php (1)
54-58: Consider consistent naming for LDAP config keys.The config keys have redundant
ldap.ldap_prefix (e.g.,ldap.ldap_use_group_restriction). This creates keys likeldap.ldap_use_group_restrictionrather thanldap.use_group_restriction.If this naming is intentional for backward compatibility, please disregard. Otherwise, consider:
- $recorder->addConfig('ldap.ldap_use_group_restriction', 'false'); - $recorder->addConfig('ldap.ldap_group_allowed_groups', ''); - $recorder->addConfig('ldap.ldap_group_auto_assign', 'false'); - $recorder->addConfig('ldap.ldap_group_mapping', ''); + $recorder->addConfig('ldap.use_group_restriction', 'false'); + $recorder->addConfig('ldap.group_allowed_groups', ''); + $recorder->addConfig('ldap.group_auto_assign', 'false'); + $recorder->addConfig('ldap.group_mapping', '');phpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationTracker.php (1)
99-102: Missing null check on query result.If the
query()method fails, it may returnfalseor throw. ThefetchObject()call on line 100 assumes$resultis valid, which could cause issues.🛡️ Suggested defensive check
$result = $this->configuration->getDb()->query($query); + if ($result === false) { + return false; + } $row = $this->configuration->getDb()->fetchObject($result); return (int) $row->cnt > 0;Similar patterns exist in
getAppliedMigrations(),getLastAppliedVersion(), andtableExists().tests/phpMyFAQ/Setup/Migration/MigrationTrackerTest.php (1)
197-217: Consider adding a test for SQL ServerTOP 1syntax.The test verifies
LIMIT 1for non-SQL Server databases, but there's no corresponding test to verifyTOP 1is used whenDatabase::getType()returns'sqlsrv'or'pdo_sqlsrv'.🧪 Suggested additional test
public function testGetLastAppliedVersionUsesTopForSqlServer(): void { // Note: This would require mocking Database::getType() to return 'sqlsrv' // which may need a different approach since it's a static method $resultMock = $this->createMock(stdClass::class); $rowMock = new stdClass(); $rowMock->version = '4.2.0'; $this->database ->expects($this->once()) ->method('query') ->with($this->callback(function ($query) { // Should use TOP 1 for SQL Server return str_contains($query, 'TOP 1') && str_contains($query, 'ORDER BY id DESC'); })) ->willReturn($resultMock); $this->database->method('fetchObject')->willReturn($rowMock); // Would need to set Database::getType() to 'sqlsrv' somehow $version = $this->tracker->getLastAppliedVersion(); $this->assertEquals('4.2.0', $version); }phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/TableBuilder.php (1)
47-51: Static dependency onDatabase::getTablePrefix()limits testability.The
table()method directly callsDatabase::getTablePrefix(), making it difficult to unit test without database setup. Consider injecting the prefix via constructor or allowing the prefix to be passed as a parameter.phpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationRegistry.php (3)
50-73: Consider auto-discovery instead of hard-coded migration list.The
registerDefaultMigrations()method contains a hard-coded list of all migrations. This requires manual updates whenever a new migration is added, which is error-prone.Consider implementing auto-discovery by scanning the
Versionsdirectory for migration classes, or using a configuration file that can be validated.
88-91: Silently skipping missing migration classes may hide configuration errors.When a registered migration class doesn't exist (line 89), it's silently skipped. This could mask typos or missing files. Consider logging a warning or throwing an exception during development/testing.
Optional: Add logging for missing classes
foreach ($this->migrationClasses as $version => $className) { if (!class_exists($className)) { + // Log warning in development: "Migration class $className for version $version not found" continue; }
147-160: Potential performance issue with large applied versions list.The
getUnappliedMigrations()method usesin_array()inside a loop. For large numbers of applied migrations, this could be slow. Consider converting$appliedVersionsto a hash set.Optional: Use array_flip for O(1) lookups
public function getUnappliedMigrations(array $appliedVersions): array { $unapplied = []; + $appliedSet = array_flip($appliedVersions); foreach ($this->getMigrations() as $version => $migration) { - if (in_array($version, $appliedVersions, true)) { + if (isset($appliedSet[$version])) { continue; } $unapplied[$version] = $migration; } return $unapplied; }tests/phpMyFAQ/Setup/Migration/MigrationRegistryTest.php (2)
90-96: Make “latest version” assertion resilient to future migrations.
Hard-coding the latest version will break whenever a new default migration is added.♻️ Suggested change
public function testGetLatestVersionReturnsLastVersion(): void { + $versions = $this->registry->getVersions(); $latestVersion = $this->registry->getLatestVersion(); - $this->assertNotNull($latestVersion); - $this->assertEquals('4.2.0-alpha', $latestVersion); + $this->assertNotEmpty($versions); + $this->assertSame(end($versions), $latestVersion); }
113-125: Consider removing hard-coded pending versions to reduce brittleness.
This will fail whenever new versions are added after 4.2.0-alpha.♻️ Suggested change
public function testGetPendingMigrationsFromMidVersion(): void { $pending = $this->registry->getPendingMigrations('4.0.0'); - // Should include versions after 4.0.0 - $this->assertArrayHasKey('4.0.5', $pending); - $this->assertArrayHasKey('4.1.0-alpha', $pending); - $this->assertArrayHasKey('4.2.0-alpha', $pending); - - // Should not include versions before or equal to 4.0.0 - $this->assertArrayNotHasKey('3.2.0-alpha', $pending); - $this->assertArrayNotHasKey('4.0.0-alpha', $pending); + $this->assertNotEmpty($pending); + foreach (array_keys($pending) as $version) { + $this->assertGreaterThan(0, version_compare($version, '4.0.0')); + } }phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Alpha3.php (1)
103-107: Normalize the bot ignore list to avoid whitespace artifacts.
The multi-line literal embeds newlines/indentation. If the consumer doesn’t trim tokens, some entries may never match. Consider normalizing before persisting.🧹 Suggested normalization
- $recorder->updateConfig('main.botIgnoreList', 'nustcrape,webpost,GoogleBot,msnbot,crawler,scooter, - bravobrian,archiver,w3c,controler,wget,bot,spider,Yahoo! Slurp,htdig,gsa-crawler,AirControler,Uptime-Kuma, - facebookcatalog/1.0,facebookexternalhit/1.1 (+http://www.facebook.com/externalhit_uatext.php), - facebookexternalhit/1.1'); + $botIgnoreList = implode(',', array_map('trim', explode(',', 'nustcrape,webpost,GoogleBot,msnbot,crawler,scooter, + bravobrian,archiver,w3c,controler,wget,bot,spider,Yahoo! Slurp,htdig,gsa-crawler,AirControler,Uptime-Kuma, + facebookcatalog/1.0,facebookexternalhit/1.1 (+http://www.facebook.com/externalhit_uatext.php), + facebookexternalhit/1.1'))); + $recorder->updateConfig('main.botIgnoreList', $botIgnoreList);
phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigAddOperation.php
Outdated
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigDeleteOperation.php
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/DirectoryCopyOperation.php
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/PermissionGrantOperation.php
Outdated
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/SqlServerDialect.php
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Alpha.php
Outdated
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration410Alpha3.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/AbstractMigration.php`:
- Around line 145-160: dropColumns() currently builds a semicolon-joined
multi-statement SQL string which will fail when passed to
SqlOperation.execute()/DB drivers; change dropColumns(string $table, array
$columns) to return an array of single ALTER TABLE ... DROP COLUMN statements
(or otherwise return each statement separately) and update callers (e.g., places
that call recorder->addSql(...) with the old single string) to iterate that
array and call recorder->addSql() once per statement so each DB query is a
single statement (refer to the dropColumns method and
SqlOperation.execute/recorder->addSql usage to locate places to update).
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Alpha3.php`:
- Around line 103-107: Replace the multiline literal passed to
$recorder->updateConfig('main.botIgnoreList', ...) in Migration400Alpha3.php
with a single concatenated string using the PHP string concatenation operator
(.) so no newlines or indentation are embedded in the stored value; mirror the
pattern used in Installer.php (concatenate short quoted fragments) to produce a
comma-separated list without whitespace so downstream parsing (explode(',',
...)) and lookups (e.g., in UserSession.php / Tracking.php using
Strings::strstr()) will match correctly.
🧹 Nitpick comments (7)
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (1)
346-355: Align dependency checks to strict comparison for consistency.
getMissingDependenciesnow uses strict checks, butareDependenciesMetstill uses loose comparison, which can yield mismatched results when dependency types differ. Consider makingareDependenciesMetstrict too.Proposed update
- if (in_array($dependency, $this->loadedPlugins)) { + if (in_array($dependency, $this->loadedPlugins, true)) { continue; }phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/DirectoryCopyOperation.php (1)
62-66: Consider catchingThrowablefor broader error coverage.The catch block only handles
Exception. IfrecursiveCopy()triggers anError(e.g.,TypeError), it would propagate uncaught. UsingThrowableprovides more defensive error handling.♻️ Optional: Catch Throwable instead
try { return $this->filesystem->recursiveCopy($this->source, $this->destination); - } catch (\Exception) { + } catch (\Throwable) { return false; }phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/PermissionGrantOperation.php (1)
60-77: Guard for invalid$rightIdhas been added — minor type-safety enhancement possible.The guard on lines 69-71 addresses the previous review concern about
addRight()potentially returning invalid values. The current implementation works because PHP's type coercion treatsfalseandnullas0, satisfying the<= 0condition.For stricter type safety, consider adding an explicit type check:
♻️ Optional: More explicit type guard
$rightId = $user->perm->addRight($rightData); - if ($rightId <= 0) { + if (!is_int($rightId) || $rightId <= 0) { return false; }phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigAddOperation.php (1)
69-84: Consider handling arrays and objects gracefully in formatValue.The fallback
(string) $valueon line 83 may produce warnings for arrays or fatal errors for objects without__toString. While configuration values are typically primitives, adding defensive handling would make the method more robust.💡 Suggested improvement
private function formatValue(mixed $value): string { if (is_bool($value)) { return $value ? 'true' : 'false'; } if (is_string($value)) { if (strlen($value) > 50) { return "'" . substr($value, 0, 47) . "...'"; } return "'{$value}'"; } if (is_null($value)) { return 'null'; } + if (is_array($value)) { + return 'array'; + } + if (is_object($value)) { + return get_class($value); + } return (string) $value; }tests/phpMyFAQ/Setup/Migration/Operations/ConfigAddOperationTest.php (1)
93-105: Consider adding a test for execute() failure scenario.The current test only covers the success path where
Configuration::add()returnstrue. Adding a test for whenadd()returnsfalsewould ensure the operation correctly propagates failure status.🧪 Suggested additional test
public function testExecuteFailure(): void { $this->configuration ->expects($this->once()) ->method('add') ->with('test.key', 'value') ->willReturn(false); $operation = new ConfigAddOperation($this->configuration, 'test.key', 'value'); $result = $operation->execute(); $this->assertFalse($result); }tests/phpMyFAQ/Setup/Migration/Operations/SqlOperationTest.php (1)
125-136: Consider importing the Exception class for consistency.The test uses
\phpMyFAQ\Core\Exceptionwith a fully qualified name while other classes are imported viausestatements. For consistency, you could add ausestatement at the top of the file.💡 Suggested improvement
Add to imports at the top of the file:
use phpMyFAQ\Core\Exception;Then update the test:
- ->willThrowException(new \phpMyFAQ\Core\Exception('Query failed')); + ->willThrowException(new Exception('Query failed'));phpmyfaq/src/phpMyFAQ/Setup/Migration/AbstractMigration.php (1)
56-62: Suppress PHPMD unused-parameter warning indown().PHPMD flags
$recorderas unused; consider suppressing to avoid CI noise.♻️ Proposed suppression
/** * Default implementation - does nothing. Override in subclass if migration is reversible. + * + * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) */ public function down(OperationRecorder $recorder): void
phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Alpha3.php
Outdated
Show resolved
Hide resolved
# Conflicts: # composer.lock
# Conflicts: # phpmyfaq/src/phpMyFAQ/Controller/Administration/FaqController.php
thorsten
force-pushed
the
feat/rewrite-update-process
branch
from
42a1c3b to
272abc1
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigAddOperation.php`:
- Around line 69-83: The formatValue() method currently falls back to an unsafe
(string) cast that emits warnings for arrays or non-stringable objects; update
formatValue to explicitly handle is_array and is_object before the final cast:
for arrays, return a short JSON or var_export representation (truncate if too
long) or a sentinel like "'[array]...'", and for objects, attempt json_encode or
use get_class($value) with a sentinel like "'[object ClassName]'" (or fallback
to gettype) to avoid warnings; keep existing handling for bool/string/null and
ensure any long representations are truncated similarly to the string branch.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigUpdateOperation.php`:
- Around line 54-58: Change ConfigUpdateOperation::execute() to return the bool
result from Configuration::update() instead of unconditionally returning true:
call $this->configuration->update([$this->key => $this->value]) and return its
value; also review Configuration::update() (the underlying method) to ensure it
returns false on failure so callers like ConfigUpdateOperation::execute() can
detect errors rather than always receiving true.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/AlterTableBuilder.php`:
- Around line 187-233: buildCombined() currently emits MySQL-specific ALTER
TABLE syntax (eg. MODIFY, ADD COLUMN ... AFTER) for every dialect; guard it by
checking the dialect before building and fail fast for non-MySQL dialects. In
buildCombined(), use the existing $this->dialect object (or add a small
capability method on the dialect) to detect whether combined MySQL-style ALTERs
are supported (reference $this->dialect and supportsColumnPositioning()); if not
supported, throw a LogicException explaining combined ALTER TABLE is MySQL-only.
This prevents emitting 'MODIFY'/'ADD COLUMN ... AFTER' for PostgreSQL/SQLServer
and keeps behavior localized to buildCombined().
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/TableBuilder.php`:
- Around line 233-246: The current logic in TableBuilder that suppresses adding
an explicit PRIMARY KEY whenever an AUTO_INCREMENT column exists incorrectly
applies to all dialects; update the condition so PRIMARY KEY is only skipped for
SQLite by checking the dialect type (use $this->dialect->getType() ===
'sqlite3') when deciding to suppress the explicit PRIMARY KEY; specifically,
keep the $hasAutoIncrement detection on $this->columns but change the if that
appends "PRIMARY KEY ($pkColumns)" to only omit it when $hasAutoIncrement is
true AND the dialect is sqlite3, otherwise always add the PRIMARY KEY clause
(references: class TableBuilder, properties $this->columns and
$this->primaryKey, and $this->dialect->getType()).
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Alpha.php`:
- Around line 102-104: The call to
$recorder->renameConfig('security.loginWithEmailAddress',
'security.loginWithEmailAddress') in Migration320Alpha is a no-op because the
source and target keys are identical; either remove this line from
Migration320Alpha.php or update the second argument to the correct new key if a
rename was intended (i.e. change the target key string to the actual new config
key), keeping the $recorder->renameConfig(...) call only when the source and
destination differ.
- Around line 43-58: The SQLite ALTER TABLE statements add multiple columns at
once which fails; update the Migration320Alpha logic inside the isSqlite()
branch to call $recorder->addSql multiple times so each call adds exactly one
column (use separate ALTER TABLE %sfaquser ADD COLUMN ... calls for
refresh_token, access_token, code_verifier, jwt and separate ALTER TABLE
%sfaquserdata ADD COLUMN ... calls for twofactor_enabled and secret), reusing
$this->tablePrefix and keeping the descriptive messages (e.g., "Add OAuth
refresh_token to faquser (SQLite)" etc.) so the migration runs sequentially
without SQLite syntax errors.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Beta.php`:
- Around line 109-114: The migration uses the deprecated SQL Server TEXT type;
update the SQL emitted in Migration320Beta (in the branch where
$this->isSqlServer() is true) so the $recorder->addSql call alters
faqconfig.config_value to VARCHAR(MAX) (or NVARCHAR(MAX) if Unicode is required)
instead of TEXT; locate the sprintf call that builds 'ALTER TABLE %sfaqconfig
ALTER COLUMN config_value TEXT' and replace the type token with the appropriate
MAX variant while keeping the existing descriptive message string unchanged.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Alpha2.php`:
- Around line 49-85: The CREATE TABLE statements in Migration400Alpha2 (in the
block that calls $recorder->addSql for %sfaqforms) lack a uniqueness constraint;
add a composite primary key or unique constraint on (form_id, input_id,
input_lang) in each dialect branch (MySQL, SQL Server, and the default) so that
rows cannot be duplicated and updates/deletes target a single row. Update the
SQL strings built in the Migration400Alpha2 file (the three sprintf calls that
create %sfaqforms) to append either "PRIMARY KEY (form_id, input_id,
input_lang)" or an explicit UNIQUE constraint with a clear name to the table
definition for all three database branches.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php`:
- Around line 322-358: The SQL Server branch in Migration420Alpha.php adds the
faqchat_messages table and indexes without idempotency; update the
recorder->addSql calls for CREATE TABLE and each CREATE INDEX (idx_chat_sender,
idx_chat_recipient, idx_chat_conversation, idx_chat_created) to guard against
existing objects—e.g., wrap the CREATE TABLE in an IF NOT EXISTS using OBJECT_ID
or sys.tables check and wrap each index creation in an IF NOT EXISTS using
sys.indexes/sys.index_columns checks (preserving the sprintf usage with
$this->tablePrefix and the existing descriptive messages) so re-running the
migration won't fail.
- Around line 81-84: The CREATE INDEX call in Migration420Alpha (the
$recorder->addSql line creating idx_hash on faqadminlog) must be made
dialect-aware and conditional to avoid errors when re-running or after partial
failures; modify the migration to detect the DB platform via
$this->connection->getDatabasePlatform()->getName() (or existing platform
helper) and emit one of several SQLs: for MySQL/PostgreSQL/SQLite use "CREATE
INDEX IF NOT EXISTS idx_hash ON {prefix}faqadminlog (hash)"; for SQL Server emit
a conditional block that checks sys.indexes (IF NOT EXISTS (SELECT * FROM
sys.indexes WHERE name='idx_hash' AND
object_id=OBJECT_ID('{prefix}faqadminlog')) BEGIN CREATE INDEX idx_hash ON
{prefix}faqadminlog (hash) END); replace the single unconditional
$recorder->addSql call with platform-conditional $recorder->addSql invocations
that include the table prefix variable and the idx_hash/index name.
- Around line 161-186: The SQL Server branch in Migration420Alpha lacks
idempotency: wrap the CREATE TABLE and CREATE INDEX statements with SQL Server
existence checks and add them via $recorder->addSql; specifically, replace the
raw CREATE TABLE in the elseif ($this->isSqlServer()) block with a conditional
like IF NOT EXISTS(SELECT * FROM sys.objects WHERE object_id =
OBJECT_ID(N'%sfaqcustompages') AND type = 'U') BEGIN CREATE TABLE ... END (use
sprintf with $this->tablePrefix to build the object name), and similarly guard
the index with IF NOT EXISTS(SELECT name FROM sys.indexes WHERE name =
'idx_custompages_slug' AND object_id = OBJECT_ID(N'%sfaqcustompages')) CREATE
INDEX ...; keep using $recorder->addSql and the same column definitions and
messages.
In `@tests/phpMyFAQ/Setup/Migration/Versions/Migration320AlphaTest.php`:
- Around line 67-82: The test callback for OperationRecorder::addSql in
testUpAddsSqlForBackupTable currently always returns true so the backup-table
check is never enforced; modify the callback to set a local boolean flag (e.g.,
$foundBackup = false) to true when str_contains($sql, 'faqbackup') and still
return true from the callback, then call $this->migration->up($recorder) and add
an explicit assertion like $this->assertTrue($foundBackup) (or assertFalse with
negation) after the up() invocation to enforce that at least one emitted SQL
contained 'faqbackup'.
🧹 Nitpick comments (11)
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PageController.php (2)
168-176: Simplify required-field check for clarity.The double-negation makes the guard harder to read. A direct check is clearer and preserves behavior.
Suggested refactor
- foreach ($requiredFields as $field) { - if (!(!isset($data->$field) || $data->$field === '')) { - continue; - } - - return $this->json(['error' => "Missing required field: $field"], Response::HTTP_BAD_REQUEST); - } + foreach ($requiredFields as $field) { + if (!isset($data->$field) || $data->$field === '') { + return $this->json(['error' => "Missing required field: $field"], Response::HTTP_BAD_REQUEST); + } + }
323-331: Simplify required-field check for clarity.Same readability concern as the create path; a direct check is easier to follow.
Suggested refactor
- foreach ($requiredFields as $field) { - if (!(!isset($data->$field) || $data->$field === '')) { - continue; - } - - return $this->json(['error' => "Missing required field: $field"], Response::HTTP_BAD_REQUEST); - } + foreach ($requiredFields as $field) { + if (!isset($data->$field) || $data->$field === '') { + return $this->json(['error' => "Missing required field: $field"], Response::HTTP_BAD_REQUEST); + } + }phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (1)
350-354: Good improvement: strictin_arraycomparison.Using the third parameter
truefor strict type comparison is a best practice. However, theareDependenciesMet()method (line 209) performs the same check without strict comparison. Consider updating it for consistency.,
♻️ Suggested consistency fix for areDependenciesMet
private function areDependenciesMet(PluginInterface $plugin): bool { if ($plugin->getDependencies() === []) { return true; } foreach ($plugin->getDependencies() as $dependency) { - if (in_array($dependency, $this->loadedPlugins)) { + if (in_array($dependency, $this->loadedPlugins, true)) { continue; } return false; } return true; }phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/DirectoryCopyOperation.php (1)
80-87: Consider adding path truncation for consistency withFileCopyOperation.The
shortenPath()method inFileCopyOperationincludes truncation for paths longer than 50 characters, while this implementation only removes thePMF_ROOT_DIRprefix. For consistent description formatting across operation types, consider aligning the implementations.♻️ Proposed alignment with FileCopyOperation
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); } + if (strlen($path) > 50) { + return '...' . substr($path, -47); + } return $path; }phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/PermissionGrantOperation.php (1)
60-77: Consider more explicit type validation and logging for failures.The
$rightId <= 0check at line 69 works in practice due to PHP's type coercion (handlesfalse,null,0, negative values), but an explicit type check would be more defensive. Additionally, silently swallowing exceptions without logging could make debugging migration failures difficult.🔧 Optional: More defensive validation and failure logging
+use Psr\Log\LoggerInterface; + readonly class PermissionGrantOperation implements OperationInterface { public function __construct( private Configuration $configuration, private string $permissionName, private string $permissionDescription, private int $userId = 1, ) { }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) { + if (!is_int($rightId) || $rightId <= 0) { return false; } return $user->perm->grantUserRight($this->userId, $rightId); - } catch (\Exception) { + } catch (\Exception $e) { + // Consider logging: $e->getMessage() for debugging migration failures return false; } }phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigUpdateOperation.php (1)
70-84: HardenformatValue()for arrays/objects and multibyte strings.Casting arrays/objects to string can emit notices and the truncation is byte-based (may split UTF‑8). Consider handling complex values explicitly and using mb_* when available.
♻️ Suggested approach
private function formatValue(mixed $value): string { if (is_bool($value)) { return $value ? 'true' : 'false'; } if (is_string($value)) { - if (strlen($value) > 50) { - return "'" . substr($value, 0, 47) . "...'"; + $len = function_exists('mb_strlen') ? mb_strlen($value, 'UTF-8') : strlen($value); + if ($len > 50) { + $prefix = function_exists('mb_substr') + ? mb_substr($value, 0, 47, 'UTF-8') + : substr($value, 0, 47); + return "'" . $prefix . "...'"; } return "'{$value}'"; } if (is_null($value)) { return 'null'; } - return (string) $value; + if (is_array($value) || is_object($value)) { + return json_encode($value, JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES) ?: 'null'; + } + return (string) $value; }tests/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/PostgresDialectTest.php (1)
9-139: Consider adding test coverage fordate()andchar()methods.The MysqlDialectTest includes
testDate()andtestChar()methods, but PostgresDialectTest is missing these. Based on the relevant code snippets, PostgresDialect implements bothdate(): stringandchar(int $length): stringmethods that should be tested for consistency across dialect test suites.💡 Suggested tests to add
+ public function testDate(): void + { + $this->assertEquals('DATE', $this->dialect->date()); + } + + public function testChar(): void + { + $this->assertEquals('CHAR(1)', $this->dialect->char(1)); + $this->assertEquals('CHAR(10)', $this->dialect->char(10)); + }phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Beta.php (1)
123-196: Add defensive handling for unsupported table names.The
DROP TABLEandRENAMEstatements (lines 191-196) execute unconditionally, but theCREATE TABLEandINSERTonly execute forfaqdataandfaqdata_revisions. If this method were called with an unsupported table name, it would drop the original table without creating a replacement first.While this is a private method currently only called with known table names, adding a guard clause would prevent accidental data loss if the method is reused in the future.
Suggested fix
private function rebuildTableWithoutColumns(OperationRecorder $recorder, string $tableName): void { $fullTableName = $this->tablePrefix . $tableName; // For faqdata and faqdata_revisions, we need to define the schema without the removed columns if ($tableName === 'faqdata') { // ... existing CREATE and INSERT for faqdata ... - } elseif ($tableName === 'faqdata_revisions') { + $recorder->addSql(sprintf('DROP TABLE %s', $fullTableName), sprintf('Drop old %s table (SQLite)', $tableName)); + $recorder->addSql( + sprintf('ALTER TABLE %s_new RENAME TO %s', $fullTableName, $fullTableName), + sprintf('Rename new %s table (SQLite)', $tableName), + ); + } elseif ($tableName === 'faqdata_revisions') { // ... existing CREATE and INSERT for faqdata_revisions ... + $recorder->addSql(sprintf('DROP TABLE %s', $fullTableName), sprintf('Drop old %s table (SQLite)', $tableName)); + $recorder->addSql( + sprintf('ALTER TABLE %s_new RENAME TO %s', $fullTableName, $fullTableName), + sprintf('Rename new %s table (SQLite)', $tableName), + ); + } else { + throw new \InvalidArgumentException(sprintf('Unsupported table name: %s', $tableName)); } - - $recorder->addSql(sprintf('DROP TABLE %s', $fullTableName), sprintf('Drop old %s table (SQLite)', $tableName)); - - $recorder->addSql( - sprintf('ALTER TABLE %s_new RENAME TO %s', $fullTableName, $fullTableName), - sprintf('Rename new %s table (SQLite)', $tableName), - ); }phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/SqliteDialect.php (1)
102-106: Silence PHPMD unused-parameter warnings explicitly.
PHPMD flags$after,$tableName,$columnName, and$newTypeas unused in these interface-mandated methods; consider suppressing to keep signal clean.♻️ Suggested suppression
+ /** `@SuppressWarnings`(PHPMD.UnusedFormalParameter) */ public function addColumn(string $tableName, string $columnName, string $type, ?string $after = null): string { // SQLite doesn't support AFTER clause return "ALTER TABLE $tableName ADD COLUMN $columnName $type"; } + /** `@SuppressWarnings`(PHPMD.UnusedFormalParameter) */ public function modifyColumn(string $tableName, string $columnName, string $newType): string { // SQLite doesn't support ALTER COLUMN directly // This would require a table rebuild, which is handled separately throw new \RuntimeException('SQLite does not support modifying columns. Use table rebuild pattern.'); } + /** `@SuppressWarnings`(PHPMD.UnusedFormalParameter) */ public function dropIndex(string $indexName, string $tableName): string { return "DROP INDEX IF EXISTS $indexName"; }Also applies to: 108-113, 122-125
phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/OperationRecorder.php (1)
99-114: Optional: reuse a Filesystem instance to avoid repeated instantiation.If many file operations are recorded, caching the default Filesystem instance in the recorder could reduce repeated object creation.
phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration400Beta2.php (1)
42-58: Optional: usetable()helper for consistency.
This keeps table prefixing centralized and matches other migrations.♻️ Proposed tweak
- $recorder->addSql( - sprintf('ALTER TABLE %sfaquser ADD COLUMN webauthnkeys TEXT NULL DEFAULT NULL', $this->tablePrefix), + $recorder->addSql( + sprintf('ALTER TABLE %s ADD COLUMN webauthnkeys TEXT NULL DEFAULT NULL', $this->table('faquser')), 'Add WebAuthn keys column to faquser (SQLite)', ); } else { $recorder->addSql( - sprintf('ALTER TABLE %sfaquser ADD webauthnkeys TEXT NULL DEFAULT NULL', $this->tablePrefix), + sprintf('ALTER TABLE %s ADD webauthnkeys TEXT NULL DEFAULT NULL', $this->table('faquser')), 'Add WebAuthn keys column to faquser', ); }
phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigUpdateOperation.php
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/AlterTableBuilder.php
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Alpha.php
Outdated
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/MysqlDialect.php`:
- Around line 87-90: The MysqlDialect::autoIncrement method currently returns
"$columnName INT NOT NULL AUTO_INCREMENT" which fails on MySQL because
AUTO_INCREMENT must be part of a PRIMARY/UNIQUE key; update
MysqlDialect::autoIncrement to include the PRIMARY KEY clause (e.g., return
"$columnName INT NOT NULL PRIMARY KEY AUTO_INCREMENT") or alternatively modify
TableBuilder so that when MysqlDialect::autoIncrement is used it also marks that
column as primary (ensure the change mirrors SqliteDialect's behavior and adjust
any tests that expect primary-key presence).
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/TableBuilder.php`:
- Around line 212-295: Both build() and buildIndexStatements() need a fast guard
to validate that table() was called and $this->tableName is set; add at the
start of each method a check like if (empty($this->tableName)) throw new
\LogicException('Table name not set: call table() before
build()/buildIndexStatements()'); to provide a clear, early failure (reference
methods build(), buildIndexStatements(), and the table() setter and property
$this->tableName).
- Around line 159-168: The autoIncrement() method in TableBuilder creates an
AUTO_INCREMENT column but doesn't mark it as a primary/indexed key which breaks
MySQL DDL; update autoIncrement(string $name = 'id') to also set the table's
primary key when no primary exists (e.g. set $this->primaryKey = $name or add
$name to $this->keys as a PRIMARY), so the generated DDL includes the required
PRIMARY KEY constraint by default; keep the existing column definition but
ensure the builder records the primary key for use by the SQL generation logic
(modify autoIncrement() and the code paths that generate keys if necessary).
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Beta.php`:
- Around line 109-114: The migration log message for SQL Server is inconsistent:
in Migration320Beta update where isSqlServer() triggers the recorder->addSql
call that executes "ALTER TABLE %sfaqconfig ALTER COLUMN config_value
NVARCHAR(MAX)", update the human-readable description string passed to
recorder->addSql to reference NVARCHAR(MAX) (or "NVARCHAR(MAX)" / "Change
faqconfig.config_value to NVARCHAR(MAX) (SQL Server)") so it matches the actual
SQL and tablePrefix/faqconfig.config_value target.
- Around line 191-196: The DROP and RENAME SQL are executed unconditionally
using $recorder, $fullTableName and $tableName which can cause data loss if
$tableName is unexpected; restrict execution to known safe names by checking
$tableName is one of 'faqdata' or 'faqdata_revisions' before calling
$recorder->addSql (or else log/throw an exception); update the private migration
routine that constructs these statements (referencing $recorder, $fullTableName,
$tableName) to wrap the DROP/ALTER calls in that guard and handle the else-case
safely.
🧹 Nitpick comments (10)
phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/DialectInterface.php (1)
109-117: Consider adding PHPDoc type hint for the$columnsparameter.The
array $columnsparameter would benefit from a more specific type annotation in the docblock to clarify it expects an array of column name strings.📝 Suggested documentation improvement
/** * Returns the CREATE INDEX syntax. + * + * `@param` string $indexName + * `@param` string $tableName + * `@param` string[] $columns + * `@param` bool $ifNotExists + * `@return` string */ public function createIndex( string $indexName, string $tableName, array $columns, bool $ifNotExists = false, ): string;phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/SqlServerDialect.php (2)
81-84:currentDate()returns datetime, not date-only.
GETDATE()returns aDATETIMEvalue including the time component. If date-only is intended, consider usingCAST(GETDATE() AS DATE). However, if this is intentional for DEFAULT constraint compatibility, a clarifying comment would help.Proposed fix if date-only is intended
public function currentDate(): string { - return 'GETDATE()'; + return 'CAST(GETDATE() AS DATE)'; }
91-100: Consider usingsys.tablesinstead of deprecatedsysobjects.
sysobjectsis a backward-compatibility view. Microsoft recommends usingsys.tablesorsys.objectsfor new development. Also,SELECT 1is marginally more efficient thanSELECT *in existence checks.Proposed modernization
public function createTablePrefix(string $tableName, bool $ifNotExists = false): string { if ($ifNotExists) { return ( - "IF NOT EXISTS (SELECT * FROM sysobjects WHERE name='$tableName' AND xtype='U') " + "IF NOT EXISTS (SELECT 1 FROM sys.tables WHERE name = '$tableName') " . "CREATE TABLE $tableName" ); } return "CREATE TABLE $tableName"; }phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/ConfigUpdateOperation.php (1)
69-84: Consider handling array and object types informatValue().The method handles
bool,string, andnullexplicitly, but falls back to(string) $valuefor other types. If an array is passed, this produces the literal string"Array". Objects without__toString()would throw a fatal error.Since config values are typically scalar, this is likely fine in practice. However, for robustness, you could add explicit handling:
Optional enhancement
private function formatValue(mixed $value): string { if (is_bool($value)) { return $value ? 'true' : 'false'; } if (is_string($value)) { if (strlen($value) > 50) { return "'" . substr($value, 0, 47) . "...'"; } return "'{$value}'"; } if (is_null($value)) { return 'null'; } + if (is_array($value)) { + return '[array]'; + } + if (is_object($value)) { + return '[object]'; + } return (string) $value; }tests/phpMyFAQ/Setup/Migration/Operations/ConfigUpdateOperationTest.php (1)
65-77: Good test for the happy path; consider adding a failure case.The test correctly verifies that
execute()returns the result fromConfiguration::update(). For comprehensive coverage, consider adding a test whereupdate()returnsfalseto ensure failure propagation is tested:Optional: Add failure case test
public function testExecuteReturnsFalseOnUpdateFailure(): void { $this->configuration ->expects($this->once()) ->method('update') ->with(['test.key' => 'value']) ->willReturn(false); $operation = new ConfigUpdateOperation($this->configuration, 'test.key', 'value'); $result = $operation->execute(); $this->assertFalse($result); }phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/SqliteDialect.php (1)
102-106: Unused$afterparameter is intentional for interface compliance.The static analyzer flags this as unused, but it's required to satisfy
DialectInterface. The inline comment correctly documents that SQLite doesn't support theAFTERclause. Consider adding a@SuppressWarnings(PHPMD.UnusedFormalParameter)annotation if your CI enforces PHPMD rules strictly.🔧 Optional: Suppress the warning
+ /** + * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) + */ public function addColumn(string $tableName, string $columnName, string $type, ?string $after = null): string { // SQLite doesn't support AFTER clause return "ALTER TABLE $tableName ADD COLUMN $columnName $type"; }phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Alpha.php (1)
127-130: ReCAPTCHA comment says v3 but keys are v2.
Update the comment (or the keys) to avoid confusion for maintainers.✏️ Suggested fix
- // Google ReCAPTCHAv3 support + // Google ReCAPTCHA v2 supportphpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/PostgresDialect.php (3)
102-106: Silence the unused$afterparameter warning.
$afteris required by the interface but unused for PostgreSQL. Consider suppressing the PHPMD warning to keep static analysis clean.🧹 Suggested suppression
+ /** `@SuppressWarnings`(PHPMD.UnusedFormalParameter) */ public function addColumn(string $tableName, string $columnName, string $type, ?string $after = null): string { // PostgreSQL doesn't support AFTER clause return "ALTER TABLE $tableName ADD COLUMN $columnName $type"; }
120-123: Suppress unused$tableNameparameter warning.The interface requires
$tableName, but PostgreSQL’s DROP INDEX doesn’t use it. Consider suppressing PHPMD here too.🧹 Suggested suppression
+ /** `@SuppressWarnings`(PHPMD.UnusedFormalParameter) */ public function dropIndex(string $indexName, string $tableName): string { return "DROP INDEX $indexName"; }
56-59: PostgreSQL's native BOOLEAN type is more idiomatic than SMALLINT; switching requires updating default handling.PostgreSQL natively supports BOOLEAN (1 byte, semantically clear). However, PostgreSQL does not accept implicit casts from integer to boolean in column defaults, so
DEFAULT 1orDEFAULT 0will fail if using BOOLEAN. The current SMALLINT mapping avoids this by using numeric literals.To switch to BOOLEAN, update TableBuilder to use
DEFAULT TRUE/DEFAULT FALSE(or explicitly cast as'1'::boolean/'0'::boolean) for PostgreSQL instead of numeric defaults.
phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/MysqlDialect.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration320Beta.php`:
- Around line 123-206: The rebuildTableWithoutColumns method has mismatched
column names and missing columns for SQLite: update the CREATE TABLE and INSERT
... SELECT statements inside rebuildTableWithoutColumns so the schemas for
'faqdata' and 'faqdata_revisions' exactly match the SQLite definitions in
Sqlite3.php (add missing columns like updated, notes, sticky_order to the
faqdata CREATE TABLE and include them in the INSERT SELECT; for
faqdata_revisions remove or correct the non-existent faq_id reference to the
actual primary/foreign column name used in Sqlite3.php), and ensure SELECT uses
updated, date_start, date_end (and other exact column names) instead of date,
dateStart, dateEnd; keep the changes localized to the rebuildTableWithoutColumns
function and the blocks that reference $tableName === 'faqdata' and $tableName
=== 'faqdata_revisions'.
Summary by CodeRabbit
New Features
Chores
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.