Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds tenant/schema support across the app: new DB Changes
Sequence DiagramsequenceDiagram
participant ClientApp as Application
participant Config as MultisiteConfigLocator
participant Bootstrap as Bootstrapper
participant DB as DatabaseDriver
participant Instance as Instance\Client
participant Installer as SchemaInstaller
ClientApp->>Config: locateConfigurationDirectory(host)
Config-->>ClientApp: configDir, tenant id?
ClientApp->>Bootstrap: connectDatabase()
Bootstrap->>DB: connect()
DB-->>Bootstrap: connection ready
Bootstrap->>Bootstrap: switchToTenantSchema(dbConfig)
alt schema present
Bootstrap->>DB: CREATE SCHEMA / CREATE DATABASE / USE / SET search_path
DB-->>Bootstrap: OK / error
else
Bootstrap-->>Bootstrap: no schema (no-op)
end
ClientApp->>Instance: createClientDatabase(tenantId, mode)
Instance->>Installer: createTables(prefix, schema?)
alt SCHEMA or DATABASE mode
Installer->>DB: create schema/database and select context
DB-->>Installer: OK / error
end
Installer->>DB: execute CREATE TABLE / INSERT seed data
DB-->>Installer: done
Installer-->>Instance: tenant creation complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
phpmyfaq/src/phpMyFAQ/Instance/Database/PdoMysql.php (1)
26-30:⚠️ Potential issue | 🟡 MinorIncorrect class docblock: says "Class Mysqli" but file defines
PdoMysql.Proposed fix
/** - * Class Mysqli + * Class PdoMysql * * `@package` phpMyFAQ\Instance\Database */phpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlsrv.php (1)
26-29:⚠️ Potential issue | 🟡 MinorIncorrect class docblock: says "Class Sqlsrv" but file defines
PdoSqlsrv.Proposed fix
/** - * Class Sqlsrv + * Class PdoSqlsrv * * `@package` phpMyFAQ\Instance\Database */phpmyfaq/src/phpMyFAQ/Instance/Database/PdoPgsql.php (1)
397-407:⚠️ Potential issue | 🔴 CriticalBug: Missing handling for
faqsearches_*andfaqchat_messages_idx_*keys that require twosprintfarguments.The
createTableStatementsarray (lines 297-299, 372-375) defines index statements likeCREATE INDEX idx_faqsearches_searchterm_%s ON %sfaqsearches ...with two%splaceholders. However, the loop condition at line 398 only checks foridx_recordsandfaqsessions_idx, so these statements receive only one$prefixargument viasprintf($stmt, $prefix), producing malformed SQL.Compare with
Pgsql.php(lines 398-408) which correctly handles all four patterns:if ( $key == 'idx_records' || $key == 'faqsessions_idx' || str_starts_with($key, 'faqsearches_') || str_starts_with($key, 'faqchat_messages_idx_') ) {Proposed fix
foreach ($this->createTableStatements as $key => $stmt) { - if ($key == 'idx_records' || $key == 'faqsessions_idx') { + if ( + $key == 'idx_records' + || $key == 'faqsessions_idx' + || str_starts_with($key, 'faqsearches_') + || str_starts_with($key, 'faqchat_messages_idx_') + ) { $result = $this->configuration->getDb()->query(sprintf($stmt, $prefix, $prefix)); } else { $result = $this->configuration->getDb()->query(sprintf($stmt, $prefix));
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Bootstrapper.php`:
- Around line 174-193: In switchToTenantSchema, ensure failures and injection
risks are handled: validate/whitelist the $schema from
DatabaseConfiguration::getSchema() (e.g., allow only alphanumeric/underscore)
before use, escape/quote it as an identifier rather than interpolating raw, then
execute the switch query using $this->db->query and check its result/exception;
on failure throw or propagate a clear exception (e.g., RuntimeException) so the
caller aborts rather than silently continuing, and apply the same
validation/quoting logic for both the mysql/mysqli branch (USE `schema`) and the
pgsql branch (SET search_path TO "schema"); leave SQL Server note intact since
no global switch is required.
In `@phpmyfaq/src/phpMyFAQ/Instance/Client.php`:
- Around line 119-128: The empty catch in createClientTablesWithSchema hides
errors during InstanceDatabase::factory, createTables or copyBaseDataToSchema;
change the catch to catch (Exception $e) and log the exception (including
message and stack) using the project's logger or error logging mechanism, then
rethrow or propagate the exception so tenant creation fails loudly; apply the
same change to the similar catch in createClientTables to avoid silently
swallowing failures.
- Around line 133-168: In copyBaseDataToSchema, avoid interpolating $schema and
$this->clientUrl directly into SQL: validate $schema against /^[A-Za-z0-9_]+$/
(same check used in Sqlsrv::createTables) and use that safe value for any
identifier quoting/SET search_path operations; for $this->clientUrl use
parameterized queries or the DB library's bound parameters/escaping rather than
sprintf interpolation when calling $this->configuration->getDb()->query; update
the INSERT/UPDATE statements in copyBaseDataToSchema to bind the client URL and
construct table identifiers only from the validated $schema (or use a proper
identifier-quoting helper) so no untrusted input is injected into SQL.
- Around line 106-111: DATABASE mode is incorrectly routed to
createClientTablesWithSchema causing CREATE SCHEMA behavior; add a distinct
branch for TenantIsolationMode::DATABASE that invokes a new method (e.g.,
createClientDatabase or createClientTablesWithDatabase) instead of
createClientTablesWithSchema, and implement driver-specific logic in the
Instance/Database layer (replace instanceDatabase->createTables('', $schema)
usage) to actually create a separate database for supported drivers (PostgreSQL,
SQL Server) and adjust connection/credential handling to connect to that newly
created DB; keep TenantIsolationMode::SCHEMA using createClientTablesWithSchema
and TenantIsolationMode::PREFIX using createClientTables, and ensure the new
method is referenced where TenantIsolationMode::DATABASE is matched.
In `@phpmyfaq/src/phpMyFAQ/Instance/Database/Mysqli.php`:
- Around line 401-406: The createTables method currently injects the raw $schema
into SQL (e.g., CREATE DATABASE/USE) without escaping or checking results;
update createTables (and any createClientTablesWithSchema helpers) in Mysqli,
PdoMysql, Pgsql, PdoPgsql, Sqlsrv and PdoSqlsrv to (1) properly escape
identifier names for the target engine (MySQL: backticks with internal backtick
doubling; PostgreSQL: double-quotes with internal quote doubling; SQL Server:
either bracket escaping or use parameterization for the sys.schemas WHERE
clause) before inserting into SQL, and (2) check the return value of the CREATE
DATABASE/CREATE SCHEMA and USE/SET search_path (or sys.schemas query) calls and
abort/return false or throw a clear exception on failure instead of continuing;
locate references to createTables and createClientTablesWithSchema in each
driver class to apply the escaping logic and error handling consistently.
In `@phpmyfaq/src/phpMyFAQ/Instance/Database/PdoMysql.php`:
- Around line 402-405: Validate and safely use $schema before executing SQL in
PdoMysql:: (the block that runs CREATE DATABASE / USE): ensure $schema matches a
strict whitelist pattern (e.g., /^[a-zA-Z0-9_]+$/) and reject or sanitize values
that fail; do not interpolate raw $schema into queries — either use a validated
identifier builder or throw/return false. After each
$this->configuration->getDb()->query(...) call check the return value and if
CREATE DATABASE or USE fails, log/return false so table creation does not
proceed in the wrong context. Apply the same validation and result-checking
changes to the other drivers (Mysqli, Pgsql, PdoPgsql, PdoSqlsrv, Sqlsrv) and
update Bootstrapper::switchToTenantSchema() to use the same validation helper.
In `@phpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlsrv.php`:
- Around line 389-397: Validate the $schema input before interpolating it into
the SQL and bail out on invalid names: inside the method that builds the IF NOT
EXISTS ... EXEC('CREATE SCHEMA [...]') block (the code that calls
$this->configuration->getDb()->query(sprintf(...))), add a check using
preg_match('/^[a-zA-Z0-9_]+$/', $schema') and return false (or equivalent error
path) if it fails, and after calling ->query(...) verify the returned result
(treat failure as an error/return false) instead of assuming success; this
prevents SQL injection from malicious schema names and ensures the query result
is handled.
In `@phpmyfaq/src/phpMyFAQ/Instance/Database/Sqlsrv.php`:
- Around line 390-398: The current code in Sqlsrv.php (and the other drivers and
SchemaInstaller::createAndUseSchema) directly interpolates $schema into a SQL
string via sprintf and executes it with $this->configuration->getDb()->query,
which is an SQL-injection risk and ignores failures; change to first validate
$schema against a strict allowlist (for example enforce /^[A-Za-z0-9_]+$/) and
reject/throw if it doesn't match, then execute the schema-creation using a safe
API (parameterized query or a driver-specific safe identifier-quoting routine
instead of raw sprintf) and check the query return value; if the query fails,
log and throw a descriptive exception (or return an error) so downstream table
creation does not continue on silent failure.
In `@phpmyfaq/src/phpMyFAQ/Instance/Setup.php`:
- Around line 143-146: The dbSchema value from $data is written raw into the
generated PHP config and can break out of the single-quoted string; in the Setup
class/method that builds the config string (look for uses of $data['dbSchema']
in phpMyFAQ/Instance/Setup.php) validate the input against a strict identifier
pattern (e.g. require it to match /^[a-zA-Z_][a-zA-Z0-9_]*$/) and throw or
reject on failure, and when inserting into the PHP literal ensure any remaining
value is safely escaped for a single-quoted PHP string (or use a safe
serializer) rather than interpolating raw user input.
In `@tests/phpMyFAQ/Instance/ClientTest.php`:
- Around line 110-130: The tests currently never force InstanceDatabase::factory
to receive a valid type because Database::getType() can be null in tests; update
the tests so the intended code path is exercised by either (A) stubbing/mocking
Database::getType() to return the expected DB type before calling
createClientDatabase (so InstanceDatabase::factory($this->configuration,
Database::getType()) runs normally), or (B) avoid relying on the static by
injecting or replacing the InstanceDatabase dependency (or its factory) into the
code under test so createClientTablesWithSchema/createClientTablesWithDatabase
run real logic; additionally tighten assertions in
testCreateClientDatabaseWithSchema and testCreateClientDatabaseWithDatabase to
expect specific SQL/query calls on the $dbMock (e.g., using withConsecutive or
exact SQL argument checks) to verify tables/data copy occurred rather than
merely asserting no exception.
🧹 Nitpick comments (7)
phpmyfaq/src/phpMyFAQ/Configuration/MultisiteConfigurationLocator.php (1)
65-85: Consider normalizing the tenant name to lowercase.DNS hostnames are case-insensitive, so
Acme.faq.example.comandacme.faq.example.comresolve identically. However, the extracted tenant is used as-is in a filesystem path (Line 45), and on case-sensitive filesystems (Linux),Acme≠acme. This could lead to tenant resolution failures depending on theHostheader casing.Proposed fix
- $tenant = substr($host, offset: 0, length: -strlen($suffix)); + $tenant = strtolower(substr($host, offset: 0, length: -strlen($suffix)));Also normalize
$hostearly:public static function extractTenantFromSubdomain(string $host): ?string { + $host = strtolower($host); $baseDomain = getenv('PMF_MULTISITE_BASE_DOMAIN');tests/phpMyFAQ/BootstrapperTest.php (1)
19-24: This test duplicates coverage already inDatabaseConfigurationTestand doesn't testBootstrapper.
DatabaseConfigurationTest::testDBConfigProperties()already asserts$this->assertNull($config->getSchema())with the same config file path. This test instantiatesDatabaseConfigurationdirectly rather than exercising anyBootstrapperlogic, making it misplaced in this test class.Consider removing this in favor of the existing assertion in
DatabaseConfigurationTest, or replacing it with a test that actually exercisesBootstrapper's schema-switching behavior (e.g.,switchToTenantSchema).phpmyfaq/src/phpMyFAQ/Bootstrapper.php (1)
183-183: Redundant condition:str_contains($dbType, 'mysql')already matches'mysqli'.The
|| str_contains($dbType, 'mysqli')part is unnecessary since'mysqli'contains'mysql'.Proposed fix
- if (str_contains($dbType, 'mysql') || str_contains($dbType, 'mysqli')) { + if (str_contains($dbType, 'mysql')) {tests/phpMyFAQ/Enums/TenantIsolationModeTest.php (1)
11-28: Consider usingassertSameinstead ofassertEqualsfor enum comparisons.
assertSameprovides strict identity checks, which is more appropriate for enums and string values.assertEqualsperforms loose comparison which could mask type mismatches.phpmyfaq/src/phpMyFAQ/Setup/Installation/SchemaInstaller.php (1)
115-142: Dialect detection viastr_containson class name is fragile.Using
str_contains($dialectClass, 'Mysql')etc. to determine the database dialect couples this logic to class naming conventions. If a dialect class is renamed or a new one is added that doesn't follow the convention, this silently falls through to thereturn trueno-op path (Line 141), which could leave a tenant without proper isolation.Consider adding a method to
DialectInterface(e.g.,getDialectType(): string) that returns a well-known identifier, or useinstanceofchecks against concrete dialect classes.tests/phpMyFAQ/Instance/ClientTest.php (1)
132-146: Environment variable cleanup should usesetUp/tearDownto be resilient against test failures.If the test fails before Line 145,
PMF_TENANT_ISOLATION_MODEremains set, potentially contaminating subsequent tests. UsetearDown()or atry/finallyblock instead.🔧 Proposed fix
public function testCreateClientDatabaseDefaultsToPrefix(): void { putenv('PMF_TENANT_ISOLATION_MODE=prefix'); - - $prefix = 'default_'; - $dbMock = $this->createMock(DatabaseDriver::class); - $this->configuration->method('getDb')->willReturn($dbMock); - - $dbMock->expects($this->atLeastOnce())->method('query'); - - $this->client->setClientUrl('https://default.example.com'); - $this->client->createClientDatabase($prefix); - - putenv('PMF_TENANT_ISOLATION_MODE'); + + try { + $prefix = 'default_'; + $dbMock = $this->createMock(DatabaseDriver::class); + $this->configuration->method('getDb')->willReturn($dbMock); + + $dbMock->expects($this->atLeastOnce())->method('query'); + + $this->client->setClientUrl('https://default.example.com'); + $this->client->createClientDatabase($prefix); + } finally { + putenv('PMF_TENANT_ISOLATION_MODE'); + } }tests/phpMyFAQ/Configuration/MultisiteConfigurationLocatorTest.php (1)
68-87: Environment and filesystem cleanup is not failure-safe.If the assertion on line 83 fails,
rmdir()andputenv()cleanup on lines 85–86 won't execute, leaking state into subsequent tests. The same fragility exists in the pre-existing test (testLocateConfigurationDirectoryReturnsConfigDirIfExists), so this is consistent — but worth hardening now that multiple tests manipulate the same env var.Consider using
setUp/tearDown(ortry/finally) to guarantee cleanup.♻️ Suggested approach using tearDown
+ private ?string $tempDir = null; + private ?string $savedBaseDomain = null; + + protected function setUp(): void + { + $this->savedBaseDomain = getenv('PMF_MULTISITE_BASE_DOMAIN') ?: null; + } + + protected function tearDown(): void + { + if ($this->tempDir !== null && is_dir($this->tempDir)) { + rmdir($this->tempDir); + $this->tempDir = null; + } + + if ($this->savedBaseDomain !== null) { + putenv('PMF_MULTISITE_BASE_DOMAIN=' . $this->savedBaseDomain); + } else { + putenv('PMF_MULTISITE_BASE_DOMAIN'); + } + }Then in each test, assign
$this->tempDirinstead of a local variable, and drop the manual cleanup lines.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
phpmyfaq/src/phpMyFAQ/Bootstrapper.php (1)
139-170:⚠️ Potential issue | 🔴 Critical
RuntimeExceptionfromswitchToTenantSchemaescapes thecatchblock — tenant switch failures won't be wrapped inDatabaseConnectionException.The
catchon Line 152 catchesException, which resolves tophpMyFAQ\Core\Exception(imported on Line 27), not\Exception. SinceswitchToTenantSchemathrows\RuntimeException(imported on Line 30), it will bypass this catch and propagate unwrapped, breaking the method's contract of throwingDatabaseConnectionException.Proposed fix — catch \RuntimeException explicitly or use \Exception
try { $dbConfig = new DatabaseConfiguration($databaseFile); Database::setTablePrefix($dbConfig->getPrefix()); $this->db = Database::factory($dbConfig->getType()); $this->db->connect( $dbConfig->getServer(), $dbConfig->getUser(), $dbConfig->getPassword(), $dbConfig->getDatabase(), $dbConfig->getPort(), ); $this->switchToTenantSchema($dbConfig); - } catch (Exception $exception) { + } catch (Exception|RuntimeException $exception) { throw new DatabaseConnectionException( message: 'Database connection failed: ' . $exception->getMessage(), code: 500, previous: $exception, ); }phpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlsrv.php (1)
25-30:⚠️ Potential issue | 🟡 MinorDocblock says "Class Sqlsrv" but the class is
PdoSqlsrv.Copy-paste artifact from the deprecated
Sqlsrvdriver.✏️ Proposed fix
/** - * Class Sqlsrv + * Class PdoSqlsrv * * `@package` phpMyFAQ\Instance\Database */phpmyfaq/src/phpMyFAQ/Instance/Database/Mysqli.php (1)
422-425:⚠️ Potential issue | 🟠 MajorDebug
echostatements leak SQL and error details — consider using a logger or removing them.Lines 423–424 output the raw CREATE TABLE statement and the database error directly. In a multi-tenant context this could expose schema internals to an attacker or to other tenants. This is pre-existing code, but it's more risky now that
createTablesis reachable from tenant provisioning flows.🛡️ Suggested fix
if (!$result) { - echo sprintf($createTableStatement, $prefix); - echo $this->configuration->getDb()->error(); - return false; }phpmyfaq/src/phpMyFAQ/Instance/Database/Sqlsrv.php (1)
388-416:⚠️ Potential issue | 🟠 MajorSchema is created but never activated — tables will be created in the default schema.
The pattern across all other drivers confirms this is a real issue: Mysqli and PdoMysql execute
USEafter creating the database, and Pgsql/PdoPgsql executeSET search_path TOafter creating the schema. Both Sqlsrv and PdoSqlsrv create the schema but then immediately create tables using only$prefix, with no schema activation. In SQL Server, tables created without an explicit schema qualifier are placed in the user's default schema (typicallydbo), not in the newly created one.You need to either:
- Set the default schema for the connection:
ALTER USER ... WITH DEFAULT_SCHEMA = [schema]- Prefix table names with the schema:
[schema].[tablename]in the$createTableStatementsphpmyfaq/src/phpMyFAQ/Instance/Database/PdoPgsql.php (1)
408-418:⚠️ Potential issue | 🔴 CriticalFix ArgumentCountError in index statement creation: check placeholder count instead of hardcoding index names.
The issue is confirmed. Seven index statements (
faqsearches_searchterm_idx,faqsearches_date_term_idx,faqsearches_date_term_lang_idx,faqchat_messages_idx_sender,faqchat_messages_idx_recipient,faqchat_messages_idx_conversation,faqchat_messages_idx_created) contain two%splaceholders but are only passed one argument tosprintf(), triggeringArgumentCountErrorin PHP 8+. The non-PDOPgsql.phpdriver already handles this correctly; the fix must be applied toPdoPgsql.phpas well.The proposed fix using
substr_count($stmt, '%s') >= 2is superior to pattern-based matching because it automatically adapts to any dual-placeholder statement, present or future.🐛 Proposed fix — match all dual-placeholder keys
foreach ($this->createTableStatements as $key => $stmt) { - if ($key == 'idx_records' || $key == 'faqsessions_idx') { + if (substr_count($stmt, '%s') >= 2) { $result = $this->configuration->getDb()->query(sprintf($stmt, $prefix, $prefix)); } else { $result = $this->configuration->getDb()->query(sprintf($stmt, $prefix)); }
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Bootstrapper.php`:
- Around line 191-213: The inner catch block using catch (Exception $exception)
does not catch the RuntimeException instances thrown in this try because
Exception resolves to phpMyFAQ\Core\Exception; update the catch to a global
throwable (change catch (Exception $exception) to catch (\Throwable $exception))
so RuntimeException and other PHP core exceptions are caught, or alternatively
remove the inner try/catch if you want those RuntimeException to bubble; locate
the block around the $this->db->query(...) calls and the explicit
RuntimeException throws to apply this change.
In `@phpmyfaq/src/phpMyFAQ/Configuration/MultisiteConfigurationLocator.php`:
- Around line 68-78: MultisiteConfigurationLocator currently lowercases $host
but leaves $baseDomain from getenv() unchanged, causing str_ends_with($host,
$suffix) to fail for env values with uppercase letters; update the code that
reads $baseDomain to normalize it (e.g., apply strtolower to $baseDomain before
creating $suffix and before ltrim) so comparisons use a lowercased $baseDomain,
keeping the rest of the logic (ltrim, suffix construction, and str_ends_with)
unchanged and referencing variables $baseDomain, $host and the str_ends_with
check.
In `@phpmyfaq/src/phpMyFAQ/Instance/Client.php`:
- Around line 301-319: The INSERT builds column names from array_keys($rowData)
without quoting, which can break for reserved words; in insertRows convert each
column name to a safely quoted identifier before composing the query (replace
the current $columns usage in insertRows), e.g. call an identifier-quoting
helper on each column (use or add a method on $this->configuration->getDb(),
e.g. quoteIdentifier(string $name) or a small adapter switch that uses
backticks/double-quotes depending on the driver) and then use implode(', ',
$quotedColumns) when building $query so column identifiers are properly escaped
for the DB engine.
- Around line 201-241: The copyBaseDataToSchema method assumes MySQL backticks
and only special-cases PgSQL; add a branch for SQL Server by checking
Database::getType() for 'sqlsrv' or 'Sqlsrv' and set $targetPrefix to use
square-bracket quoting (e.g. sprintf('[%s].', $schema)) before executing
queries; do not call PostgreSQL-specific search_path for SQL Server, but ensure
you continue to use $escapedClientUrl and the same
configuration->getDb()->query(...) calls with the new $targetPrefix so the
INSERT/UPDATE statements run correctly against SQL Server schemas.
- Around line 148-151: The validation in createClientTablesWithDatabase silently
returns on an invalid $databaseName (preg_match check) which is inconsistent
with createClientTablesWithSchema that throws an Exception; change the silent
return in createClientTablesWithDatabase to throw the same kind of Exception
(with a clear message about an invalid database identifier) so callers in
DATABASE mode can detect the validation failure; reference the preg_match
validation in createClientTablesWithDatabase and mirror the Exception behavior
used by createClientTablesWithSchema.
In `@phpmyfaq/src/phpMyFAQ/Instance/Database.php`:
- Around line 129-164: createTenantDatabase currently returns false for
unsupported drivers (e.g. MySQL/MariaDB) causing silent failures; update
validation to explicitly reject DATABASE isolation for incompatible drivers by
checking the normalized $type in createTenantDatabase and throwing a clear
exception (e.g. InvalidArgumentException or RuntimeException) when the driver is
not PostgreSQL or SQL Server, and/or add a guard in
createClientTablesWithDatabase to prevent selecting DATABASE isolation for
non-pgsql/non-sqlsrv drivers so the caller receives an explicit error instead of
a silent false.
In `@phpmyfaq/src/phpMyFAQ/Tenant/TenantContextResolver.php`:
- Around line 32-34: The hostname fallback directly reads $_SERVER['HTTP_HOST']
in TenantContextResolver::resolve (the block that sets $hostname when it's ===
''), which bypasses Symfony's Request host validation; remove the raw $_SERVER
fallback and instead rely on the already-created Symfony Request::getHost() (or,
if you must keep a fallback, validate/sanitize the value using Symfony
Request::getHost() or filter_var with FILTER_VALIDATE_DOMAIN/allowed flags) so
the hostname used by TenantContextResolver is always validated/sanitized before
any configDir/tenant lookup.
In `@tests/phpMyFAQ/Instance/ClientTest.php`:
- Around line 111-143: Add a tearDown method to the test class that clears the
static Database state after each test: implement protected function tearDown():
void that calls Database::setTablePrefix('') to reset the table prefix (and any
other static cleanup you need for Database::factory if applicable) and then
calls parent::tearDown(); this prevents Database::factory('pdo_pgsql') and
Database::setTablePrefix('') in testCreateClientDatabaseWithSchemaMode from
leaking state into other tests.
- Around line 145-195: The test fails to exercise the DATABASE code path because
Client::createClientTablesWithDatabase() calls getDatabaseCredentials() which
returns null when PMF_CONFIG_DIR . '/database.php' is missing; fix by ensuring
getDatabaseCredentials() returns non-null in the test—either add a
tests/content/core/config/database.php fixture with valid credentials so
createClientTablesWithDatabase() proceeds, or modify the test to mock the
client's getDatabaseCredentials() (or the Configuration method used) to return a
credential array/object before calling createClientDatabase('tenant_db',
TenantIsolationMode::DATABASE) so the code path invokes
DatabaseDriver::connect() and query() as expected.
In `@tests/phpMyFAQ/Tenant/TenantContextResolverTest.php`:
- Around line 20-33: The tearDown method in TenantContextResolverTest currently
clears environment variables but does not unset the global
$_SERVER['HTTP_HOST'], which can leak between tests; update the tearDown()
method to also unset $_SERVER['HTTP_HOST'] (e.g., use
unset($_SERVER['HTTP_HOST']) or set it to null) so any test that sets HTTP_HOST
(such as the fallback test) is cleaned up after each test run; modify the
tearDown() implementation where putenv(...) and Database::setTablePrefix('') are
called to include removal of $_SERVER['HTTP_HOST'].
🧹 Nitpick comments (5)
tests/phpMyFAQ/Instance/SetupTest.php (1)
117-139: Consider using PHP's built-inRecursiveDirectoryIteratorfor recursive deletion.The hand-rolled
removeDirectoryworks correctly but could be simplified. This is a minor nicety for test infrastructure.♻️ Optional simplification
private function removeDirectory(string $directory): void { - $entries = scandir($directory); - if ($entries === false) { - return; - } - - foreach ($entries as $entry) { - if ($entry === '.' || $entry === '..') { - continue; - } - - $path = $directory . '/' . $entry; - if (is_dir($path)) { - $this->removeDirectory($path); - continue; - } - - unlink($path); - } - - rmdir($directory); + $files = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator($directory, \FilesystemIterator::SKIP_DOTS), + \RecursiveIteratorIterator::CHILD_FIRST, + ); + + foreach ($files as $file) { + $file->isDir() ? rmdir($file->getPathname()) : unlink($file->getPathname()); + } + + rmdir($directory); }phpmyfaq/src/phpMyFAQ/Instance/Database.php (1)
140-150: PostgreSQL: existence check uses single-quoted string literal while CREATE uses double-quoted identifier — correct but inconsistent escaping approach.The regex on Line 136 already restricts
$databaseNameto[A-Za-z0-9_]+, so the raw interpolation on Line 150 is safe. However, for defense-in-depth, consider usingescape()consistently (or at least a comment explaining why it's safe to use the raw value).tests/phpMyFAQ/Instance/DatabaseTest.php (1)
86-110: Consider adding a test for the "database already exists" path.The pgsql branch has an early-return
truewhen the database already exists (numRows > 0), but there's no test covering that path. A test wherenumRowsreturns1and verifying only one query is issued would improve coverage.tests/phpMyFAQ/Instance/ClientTest.php (1)
197-209:queryContainshelper is only used once — consider usingassertThator inline check.Minor nit: this private helper is only called on Line 132. You could simplify by using a loop with
assertThator just inline the check. Not blocking.phpmyfaq/src/phpMyFAQ/Instance/Client.php (1)
243-264:include $databaseFileexecutes arbitrary code from the filesystem.While this file path is constructed from a constant (
PMF_CONFIG_DIR), it's worth noting that this pattern is fragile — any file-write vulnerability elsewhere could lead to code execution here. This pattern is used throughout the project in at least 8 files (Configuration, Translation, Routing, Administration modules), so refactoring would require project-wide changes. Consider usingparse_ini_fileor a JSON config instead if feasible.
There was a problem hiding this comment.
Actionable comments posted: 1
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/Bootstrapper.php (1)
150-158:⚠️ Potential issue | 🔴 Critical
RuntimeExceptionfromswitchToTenantSchemaescapes thecatchblock.Line 152's
catch (Exception $exception)resolves tophpMyFAQ\Core\Exception(Line 27 import). The\RuntimeExceptionthrown byswitchToTenantSchema(Lines 186, 196, 205, 209) is a global PHP exception and does not extendphpMyFAQ\Core\Exception, so it will not be caught here. This means a tenant schema switch failure bypasses theDatabaseConnectionExceptionwrapping and propagates raw to callers ofrun().Proposed fix — catch \Throwable or the global \RuntimeException
try { $dbConfig = new DatabaseConfiguration($databaseFile); Database::setTablePrefix($dbConfig->getPrefix()); $this->db = Database::factory($dbConfig->getType()); $this->db->connect( $dbConfig->getServer(), $dbConfig->getUser(), $dbConfig->getPassword(), $dbConfig->getDatabase(), $dbConfig->getPort(), ); $this->switchToTenantSchema($dbConfig); - } catch (Exception $exception) { + } catch (Exception|\RuntimeException $exception) { throw new DatabaseConnectionException( message: 'Database connection failed: ' . $exception->getMessage(), code: 500, previous: $exception, ); }Alternatively, catch
\Throwableto cover all failure modes uniformly.
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Instance/Client.php`:
- Around line 313-331: The insertRows method currently discards the result of
$this->configuration->getDb()->query($query) so failed INSERTs are silent;
update insertRows to check the query return value and handle failures (e.g., log
the failed $query and DB error or throw an exception) so callers notice errors.
Specifically, in insertRows after calling
$this->configuration->getDb()->query($query) inspect the returned value (or
error info from $this->configuration->getDb()), and either collect/log the error
with context (including the $table and failing $query) or throw a
RuntimeException containing the DB error message; ensure the error handling uses
existing logging/error mechanisms available on the configuration/getDb objects
rather than suppressing failures.
🧹 Nitpick comments (5)
phpmyfaq/src/phpMyFAQ/Instance/Client.php (1)
365-378:$this->clientUrlis interpolated without escaping increateClientTables.Line 377 uses
$this->clientUrldirectly in SQL, whereas the newinsertSeedRows(Line 306) correctly calls$this->configuration->getDb()->escape(). This is pre-existing code, but now that the pattern is established elsewhere, it's worth aligning.Proposed fix
$this->configuration ->getDb() ->query(sprintf( "UPDATE %sfaqconfig SET config_value = '%s' WHERE config_name = 'main.referenceURL'", $prefix, - $this->clientUrl, + $this->configuration->getDb()->escape($this->clientUrl), ));tests/phpMyFAQ/Instance/DatabaseTest.php (1)
86-110: Consider adding a test for the idempotent "already exists" path.The PostgreSQL branch in
createTenantDatabasereturnstrueearly whennumRows > 0. A test wherenumRowsreturns1would verify this short-circuit and ensureCREATE DATABASEis never issued in that case.tests/phpMyFAQ/Instance/ClientTest.php (1)
231-245: Environment variable cleanup could be more robust.If the test fails before Line 244,
PMF_TENANT_ISOLATION_MODEremains set for subsequent tests. Consider moving the cleanup totearDownor usingtry/finally.Proposed fix
public function testCreateClientDatabaseDefaultsToPrefix(): void { putenv('PMF_TENANT_ISOLATION_MODE=prefix'); - - $prefix = 'default_'; - $dbMock = $this->createMock(DatabaseDriver::class); - $this->configuration->method('getDb')->willReturn($dbMock); - - $dbMock->expects($this->atLeastOnce())->method('query'); - - $this->client->setClientUrl('https://default.example.com'); - $this->client->createClientDatabase($prefix); - - putenv('PMF_TENANT_ISOLATION_MODE'); + try { + $prefix = 'default_'; + $dbMock = $this->createMock(DatabaseDriver::class); + $this->configuration->method('getDb')->willReturn($dbMock); + + $dbMock->expects($this->atLeastOnce())->method('query'); + + $this->client->setClientUrl('https://default.example.com'); + $this->client->createClientDatabase($prefix); + } finally { + putenv('PMF_TENANT_ISOLATION_MODE'); + } }phpmyfaq/src/services.php (1)
226-233: Service placement nit: consider grouping tenant services with a section comment.The other service groups have clear section comments (e.g.,
// ========== Core Symfony framework services ==========). The tenant services are interleaved betweenphpmyfaq.commentsandphpmyfaq.configurationwithout a visual separator, making them easy to miss.♻️ Suggested grouping
+ // ========== Tenant services ========== $services->set('phpmyfaq.tenant.context-resolver', TenantContextResolver::class); $services->set('phpmyfaq.tenant.context', TenantContext::class)->factory([ service('phpmyfaq.tenant.context-resolver'), 'resolve', ]); $services->set('phpmyfaq.tenant.event-dispatcher', TenantEventDispatcher::class)->args([ service('phpmyfaq.event_dispatcher'), ]);phpmyfaq/src/phpMyFAQ/Tenant/TenantEventDispatcher.php (1)
63-74: Caller-suppliedoldPlan/newPlancontext keys are silently overwritten.Lines 70-71 unconditionally set
$eventContext['oldPlan']and$eventContext['newPlan'], overwriting any values a caller might have passed in$contextunder those same keys. This is likely intentional (the dedicated parameters are the source of truth), but you may want to document this behavior in the PHPDoc or assert/warn if the keys already exist.
There was a problem hiding this comment.
Actionable comments posted: 3
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/Instance/Client.php (1)
383-389:⚠️ Potential issue | 🔴 Critical
$this->clientUrlis interpolated into SQL without escaping — potential SQL injection.In
createClientTables, line 387 uses$this->clientUrldirectly in the query string. Compare withinsertSeedRows(line 306) andcopyBaseDataToSchema(line 219), which both properly call$this->configuration->getDb()->escape(...)on the URL before interpolation.Proposed fix
$this->configuration ->getDb() ->query(sprintf( "UPDATE %sfaqconfig SET config_value = '%s' WHERE config_name = 'main.referenceURL'", $prefix, - $this->clientUrl, + $this->configuration->getDb()->escape($this->clientUrl), ));
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Instance/Client.php`:
- Around line 163-191: The method createClientDatabase currently exits silently
on failures (calls to InstanceDatabase::createTenantDatabase,
$this->configuration->getDb()->connect,
InstanceDatabase::factory(...)->createTables and any seed/apply steps), which
hides root causes; replace those bare "return" paths with throwing descriptive
exceptions (or at minimum logging then throwing) that include the failing
operation and relevant details (e.g., database name, server, error from DB
connection, result from createTenantDatabase/createTables) so callers can detect
success/failure and operators can diagnose issues; update error handling around
InstanceDatabase::createTenantDatabase, $this->configuration->getDb()->connect,
InstanceDatabase::factory(...)->createTables and the seed application code to
throw RuntimeException (or a specific domain exception) with contextual messages
instead of returning silently.
In `@tests/phpMyFAQ/Instance/ClientTest.php`:
- Around line 235-249: The test testCreateClientDatabaseDefaultsToPrefix sets
the environment variable PMF_TENANT_ISOLATION_MODE but currently resets it at
the end of the test body which will leak if an exception occurs; modify the test
to ensure cleanup always runs by wrapping the core test logic (setting $prefix,
creating $dbMock, expectations, setClientUrl and createClientDatabase calls) in
a try/finally and move the putenv('PMF_TENANT_ISOLATION_MODE') reset into the
finally block (or alternatively remove the inline putenv reset and add
restoration logic to tearDown), referencing the test method
testCreateClientDatabaseDefaultsToPrefix and the usage of putenv in that method
so the env var is cleared even on failure.
- Around line 155-219: The testCreateClientDatabaseWithDatabaseMode method
currently uses assertFileExists to guard for the
tests/content/core/config/database.php fixture which causes a hard failure if
missing; change that to an environment-precondition check that calls
$this->markTestSkipped('...') when PMF_CONFIG_DIR . '/database.php' does not
exist so the test is skipped instead of failing (update the message to explain
the missing fixture), leaving the rest of the test logic (Database::factory,
Database::setTablePrefix, mock setup, and assertions) unchanged.
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/Instance/Client.php (1)
398-404:⚠️ Potential issue | 🟠 Major
$this->clientUrlis interpolated into SQL without escaping — SQL injection risk.In
createClientTables(PREFIX mode), line 403 uses$this->clientUrldirectly in the SQL string. Compare withcopyBaseDataToSchema(line 234) andinsertSeedRows(line 321), which both call$this->configuration->getDb()->escape(...). A malicious or malformed URL containing single quotes will break the query or allow injection.Proposed fix
$this->configuration ->getDb() ->query(sprintf( "UPDATE %sfaqconfig SET config_value = '%s' WHERE config_name = 'main.referenceURL'", $prefix, - $this->clientUrl, + $this->configuration->getDb()->escape($this->clientUrl), ));
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Instance/Client.php`:
- Around line 245-268: In copyBaseDataToSchema, the four calls to
$this->configuration->getDb()->query(...) ignore return values so failures are
silent; update copyBaseDataToSchema to check each query result and throw a
descriptive exception (e.g., RuntimeException) if a query returns false or an
error, including the DB error/info and which statement failed so
createClientTablesWithSchema can detect and abort; use the same
$this->configuration->getDb() instance to retrieve error details (error,
errorInfo or platform-specific method) and include identifiers like the
targetPrefix/sourcePrefix and the operation (INSERT/UPDATE) in the thrown
message.
- Around line 210-218: The finally block currently calls
$this->configuration->getDb()->connect(...) and silently ignores any failure;
change it to attempt the reconnect inside a try/catch so reconnect exceptions
are caught and logged (but do not replace or swallow the original exception
thrown earlier). Wrap the call to $this->configuration->getDb()->connect(...) in
a try { ... } catch (\Throwable $e) { ... } and emit a clear error message using
the existing logging mechanism (e.g. $this->logger or
$this->configuration->getLogger()) including $sourceDatabase and the exception
message/trace so callers still receive the original exception while the
reconnect failure is recorded.
In `@tests/phpMyFAQ/Instance/ClientTest.php`:
- Around line 319-326: Remove the dead assignment to $origConfigDir (the unused
variable set from PMF_CONFIG_DIR) in the test setup; delete the line "
$origConfigDir = PMF_CONFIG_DIR;" and any surrounding unused references so
ClientTest::test... no longer contains the unused $origConfigDir variable while
preserving the intended rename/restore logic that checks for database.php
existence.
- Line 444: Remove the unused variable declaration $queryCount in the test (it
is declared but never incremented or used); locate the declaration of
$queryCount in the ClientTest test method and delete that line so no unused
variable remains.
|
|
||
| // Use a non-existent config dir to ensure getDatabaseCredentials returns null | ||
| $origConfigDir = PMF_CONFIG_DIR; | ||
| // We can't change the constant, but getDatabaseCredentials reads from PMF_CONFIG_DIR/database.php. | ||
| // Instead, test by invoking on a client whose config dir won't have the file. | ||
| // Since PMF_CONFIG_DIR is set to tests/content/core/config, if database.php doesn't exist there | ||
| // the test would pass. But it does exist. So we test via a different approach: | ||
| // We'll just verify the exception is thrown by temporarily renaming the file. |
There was a problem hiding this comment.
Unused variable $origConfigDir.
Line 321 assigns $origConfigDir = PMF_CONFIG_DIR but it is never referenced. The surrounding comments suggest an earlier approach was abandoned. Remove the dead assignment.
Proposed fix
- $origConfigDir = PMF_CONFIG_DIR;
- // We can't change the constant, but getDatabaseCredentials reads from PMF_CONFIG_DIR/database.php.
- // Instead, test by invoking on a client whose config dir won't have the file.
- // Since PMF_CONFIG_DIR is set to tests/content/core/config, if database.php doesn't exist there
- // the test would pass. But it does exist. So we test via a different approach:
- // We'll just verify the exception is thrown by temporarily renaming the file.📝 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.
| // Use a non-existent config dir to ensure getDatabaseCredentials returns null | |
| $origConfigDir = PMF_CONFIG_DIR; | |
| // We can't change the constant, but getDatabaseCredentials reads from PMF_CONFIG_DIR/database.php. | |
| // Instead, test by invoking on a client whose config dir won't have the file. | |
| // Since PMF_CONFIG_DIR is set to tests/content/core/config, if database.php doesn't exist there | |
| // the test would pass. But it does exist. So we test via a different approach: | |
| // We'll just verify the exception is thrown by temporarily renaming the file. | |
| // Use a non-existent config dir to ensure getDatabaseCredentials returns null |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 321-321: Avoid unused local variables such as '$origConfigDir'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In `@tests/phpMyFAQ/Instance/ClientTest.php` around lines 319 - 326, Remove the
dead assignment to $origConfigDir (the unused variable set from PMF_CONFIG_DIR)
in the test setup; delete the line " $origConfigDir = PMF_CONFIG_DIR;" and any
surrounding unused references so ClientTest::test... no longer contains the
unused $origConfigDir variable while preserving the intended rename/restore
logic that checks for database.php existence.
1 participant
Summary by CodeRabbit
New Features
Tests