refactor(setup): improved installation and setup classes #3932
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughRefactors installation into modular components (schema builder, seeder, installer runner, input validator); Database factory now resolves dialect via DialectFactory and returns a SchemaInstaller; adds longText/blob dialect support, new migration operations, and extensive tests for installation/migration flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Installer Caller
participant Validator as InstallationInputValidator
participant Runner as InstallationRunner
participant SchemaInstaller as SchemaInstaller
participant Seeder as DefaultDataSeeder
participant DB as Database Driver
participant Search as Search Initializer
Caller->>Validator: validate(setup)
Validator-->>Caller: InstallationInput
Caller->>Runner: run(InstallationInput)
activate Runner
Runner->>DB: validate connectivity
DB-->>Runner: ok
Runner->>SchemaInstaller: createTables(prefix?)
SchemaInstaller->>DB: executeSql(CREATE TABLE ...)
DB-->>SchemaInstaller: result
Runner->>Seeder: seed config & form inputs
Seeder-->>Runner: done
Runner->>Search: init indices (ES/OS) (if enabled)
Search-->>Runner: indices ready
deactivate Runner
Runner-->>Caller: finished
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Setup/Installation/SchemaInstaller.php`:
- Around line 116-131: dropTables currently ignores any global table prefix when
called with an empty string; update the dropTables(string $prefix = '') method
in SchemaInstaller so that if $prefix === '' it falls back to the configured
global prefix from $this->configuration (call the appropriate getter on the
configuration object, e.g. getPrefix()/getTablePrefix()/getDbPrefix()), then use
that prefix when building the DROP TABLE SQL (the sprintf call that currently
uses $prefix and the loop over $this->schema->getTableNames()); keep the same
error handling that checks $this->configuration->getDb()->query($sql) and
returns false on failure.
In `@phpmyfaq/src/phpMyFAQ/Setup/InstallationInput.php`:
- Around line 22-45: Change the sensitive credential properties in the readonly
InstallationInput class from public to private (at minimum password and
loginName; consider email/permLevel if they carry sensitive data) and add
explicit getter methods (e.g., getPassword(), getLoginName()) to expose them
only when needed; update the constructor signature to accept these values and
assign them to the private properties, and adjust any code that referenced the
public properties to use the new accessors (look for references to
InstallationInput::password, ::loginName, and other credential fields).
In `@phpmyfaq/src/phpMyFAQ/Setup/InstallationInputValidator.php`:
- Around line 136-139: The null check on $dbSetup['dbUser'] after calling
Filter::filterInput with a default of '' is ineffective (is_null will never be
true); update the validation in InstallationInputValidator.php to detect missing
username correctly by either changing the filterInput default to null or
replacing is_null($dbSetup['dbUser']) with a check for an empty value (e.g.,
$dbSetup['dbUser'] === '' or empty($dbSetup['dbUser'])) while preserving the
System::isSqlite($dbSetup['dbType']) exemption and throwing the same
Exception('Installation Error: Please add a database username.') when
appropriate.
- Around line 121-124: The null check is ineffective because Filter::filterInput
returns '' by default; in InstallationInputValidator.php replace the is_null
check on $dbSetup['dbServer'] (populated via Filter::filterInput) with an
emptiness check (e.g., trim($dbSetup['dbServer']) === '' or empty(trim(...)))
and keep the System::isSqlite($dbSetup['dbType']) exclusion so the throw in the
validation block still fires for non-SQLite when the server string is empty.
In `@phpmyfaq/src/phpMyFAQ/Setup/InstallationRunner.php`:
- Around line 275-293: The stepCreateAdminUser method currently ignores failures
from User::setStatus, User::setUserData, and User::setSuperAdmin; update
stepCreateAdminUser to check each call's return value and treat any false result
as a fatal installation error: call Installer::cleanFailedInstallationFiles()
and throw an Exception with a descriptive message including the User::error()
output. Locate the calls to setStatus('protected'), setUserData($adminData), and
setSuperAdmin(true) and perform immediate checks after each (or aggregate
checks) so the installation aborts and logs the specific user error if any
mutation fails, similar to how createUser failures are handled.
- Around line 355-381: In stepInitializeSearchEngine(InstallationInput $input)
the code checks/is_file and loads elasticsearch.php and opensearch.php from
"{$input->rootDir}/config/..." but earlier installation writes those files to
"{$input->rootDir}/content/core/config/..."; update the path used in both
elasticsearch and opensearch flows (the is_file checks and the constructors
ElasticsearchConfiguration(...) and OpenSearchConfiguration(...)) to point to
"{$input->rootDir}/content/core/config/..." so that setElasticsearchConfig(...),
setOpenSearchConfig(...), setElasticsearch(...), setOpenSearch(...) and
subsequent createIndex() calls operate on the actual written config files.
- Around line 89-157: In stepValidateConnectivity update the Elasticsearch and
OpenSearch checks to actually probe the servers: after creating $esClient via
ClientBuilder::create()->setHosts(...)->build() call $esClient->ping() (or
$esClient->info()) inside a try-catch and throw an Exception if the call returns
false or an exception is thrown; similarly, after creating $osClient with
SymfonyClientFactory()->create() perform a HEAD request to '/' (e.g.
$osClient->request('HEAD','/')) inside a try-catch and throw an Exception on
non-2xx responses or exceptions; wrap both probes to catch network errors and
return clear installation errors instead of relying on the client object being
non-null.
In
`@phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/FormInputInsertOperation.php`:
- Around line 52-57: The execute() method currently always returns true even
though Forms::insertInputIntoDatabase($this->formInput) returns a boolean;
change execute() to capture that boolean result from
$forms->insertInputIntoDatabase(...) and return that result (or return false on
failure), so the insert result is propagated; locate the Forms instantiation in
execute() and replace the unconditional return true with returning the actual
insert outcome from Forms::insertInputIntoDatabase.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/OperationRecorder.php`:
- Around line 125-158: The method declaration for createUser has its opening
brace on the same line; move the brace to the next line to comply with PER-3
(adjust the createUser(...) declaration so its "{" is on the following line),
and ensure the rest of the method (the new UserCreateOperation push and return
$this) keeps its current indentation and formatting; verify neighboring methods
(e.g., insertFormInput) still follow PER-3 for consistency.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/UserCreateOperation.php`:
- Around line 51-74: The execute() method currently ignores boolean return
values from User::setStatus, User::setUserData, and User::setSuperAdmin so it
can return true despite partial failures; update execute() to check each call's
return and immediately return false if any of setStatus($this->status),
setUserData($userData) or (when $this->isSuperAdmin) setSuperAdmin(true) returns
false (while still preserving the initial createUser(...) check and the
try/catch behavior).
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/TableBuilder.php`:
- Around line 329-331: The check in TableBuilder::buildIndexStatements()
accesses $this->tableName before it's guaranteed to be initialized; change the
condition to guard for an uninitialized or empty property (e.g. if
(!isset($this->tableName) || $this->tableName === '') ) and throw the
LogicException if true—ensure this mirrors how table() sets the property so
buildIndexStatements() no longer reads an uninitialized $this->tableName.
- Around line 254-256: The check in TableBuilder::build() accesses the typed
property $tableName before it's guaranteed to be initialized, causing a fatal
Error if table() was never called; update the guard to avoid direct access to an
uninitialized property (e.g. initialize $tableName with a default empty string
when declared or change the check to use isset($this->tableName) or null
coalescing like ($this->tableName ?? '') === ''), and keep the thrown
LogicException message the same; touch the TableBuilder::$tableName declaration
or modify the build() guard to reference isset/?? so build() and table() logic
remain consistent.
In `@tests/phpMyFAQ/Setup/Installation/SchemaInstallerTest.php`:
- Around line 103-123: The test testPostgresNoIndexStatementsDuplicate currently
only asserts there is at least one CREATE INDEX but doesn't verify duplicates;
update the test in SchemaInstallerTest::testPostgresNoIndexStatementsDuplicate
to collect the actual CREATE INDEX statements from $sql (e.g. into an $indexes
array), then assert that count($indexes) equals count(array_unique($indexes))
(or use $this->assertCount and $this->assertEquals) to ensure no duplicate
CREATE INDEX statements are emitted by SchemaInstaller::createTables and
returned from getCollectedSql.
🧹 Nitpick comments (7)
tests/phpMyFAQ/Session/SessionWrapperTest.php (1)
26-33: Consider clarifying or removing the trailing comment.The comment on lines 31-32 states "Instead, verify the wrapper works by testing delegation" but this test method doesn't actually perform that verification—delegation is tested by the other test methods in this class. The dangling comment could be confusing to future readers.
Consider either removing the comment entirely or rephrasing it to clarify that delegation tests exist separately:
✏️ Suggested improvement
public function testConstructorWithSessionParameter(): void { $sessionWrapper = new SessionWrapper($this->sessionMock); $this->assertInstanceOf(SessionWrapper::class, $sessionWrapper); - - // The session is private, so we can't access it directly - // Instead, verify the wrapper works by testing delegation + // Delegation behavior is verified in separate test methods (e.g., testGetMethodDelegatesToSession) }phpmyfaq/src/phpMyFAQ/Setup/Update.php (1)
360-396: Redundant version check inmigrateAdminLogHashes().The version check at Line 362 is already guarded by the same check at Line 263 in
runPostMigrationTasks(). The inner check is redundant and can be removed to simplify the code.♻️ Proposed simplification
private function migrateAdminLogHashes(): void { - if (version_compare($this->version, '4.2.0-alpha', '<')) { - $repository = new AdminLogRepository($this->configuration); + $repository = new AdminLogRepository($this->configuration); - try { - $entries = $repository->getAll(); - $previousHash = null; + try { + $entries = $repository->getAll(); + $previousHash = null; - foreach ($entries as $entity) { - if ($entity->getHash() !== null) { - continue; - } + foreach ($entries as $entity) { + if ($entity->getHash() !== null) { + continue; + } - $entity->setPreviousHash($previousHash); - $hash = $entity->calculateHash(); + $entity->setPreviousHash($previousHash); + $hash = $entity->calculateHash(); - // Execute UPDATE directly instead of adding to the queries array - $updateQuery = sprintf( - "UPDATE %sfaqadminlog SET hash = '%s', previous_hash = %s WHERE id = %d", - Database::getTablePrefix(), - $this->configuration->getDb()->escape($hash), - $previousHash !== null - ? "'" . $this->configuration->getDb()->escape($previousHash) . "'" - : 'NULL', - $entity->getId(), - ); + // Execute UPDATE directly instead of adding to the queries array + $updateQuery = sprintf( + "UPDATE %sfaqadminlog SET hash = '%s', previous_hash = %s WHERE id = %d", + Database::getTablePrefix(), + $this->configuration->getDb()->escape($hash), + $previousHash !== null + ? "'" . $this->configuration->getDb()->escape($previousHash) . "'" + : 'NULL', + $entity->getId(), + ); - $this->configuration->getDb()->query($updateQuery); + $this->configuration->getDb()->query($updateQuery); - $previousHash = $hash; - } - } catch (\Exception $e) { - $this->configuration->getLogger()->error('Admin log hash migration failed: ' . $e->getMessage()); + $previousHash = $hash; } + } catch (\Exception $e) { + $this->configuration->getLogger()->error('Admin log hash migration failed: ' . $e->getMessage()); } }phpmyfaq/src/phpMyFAQ/Setup/InstallationInputValidator.php (1)
311-316: Password length validation uses inconsistent boundary.The check
strlen(...) <= 7means passwords of exactly 8 characters pass, but the error message says "minimum of 8 characters". Consider using< 8for clarity and consistency with the message.♻️ Proposed fix for clarity
- if (strlen((string) $password) <= 7 || strlen((string) $passwordRetyped) <= 7) { + if (strlen((string) $password) < 8 || strlen((string) $passwordRetyped) < 8) { throw new Exception( 'Installation Error: Your password and retyped password are too short. Please set your password ' . 'and your retyped password with a minimum of 8 characters.', ); }tests/phpMyFAQ/Setup/InstallationInputValidatorTest.php (1)
25-55: Test name is misleading - doesn't actually verify readonly behavior.The test
testInstallationInputIsReadonlyonly asserts that properties can be read with expected values. It doesn't verify thereadonlyconstraint (that properties cannot be modified after construction).Consider renaming or adding actual readonly verification.
♻️ Suggested improvement
Either rename the test:
- public function testInstallationInputIsReadonly(): void + public function testInstallationInputCanBeConstructedWithExpectedValues(): voidOr add actual readonly verification (PHP 8.1+ readonly properties throw Error on modification):
public function testInstallationInputIsReadonly(): void { $input = new InstallationInput(/* ... */); $this->expectException(\Error::class); $input->loginName = 'changed'; // Should throw }tests/phpMyFAQ/Setup/InstallationRunnerTest.php (1)
67-101: Source code parsing test is fragile.This test parses the raw source code to verify step order, which is brittle:
- Refactoring (e.g., extracting steps to a loop over an array) would break the test even if behavior is correct.
- Comments mentioning step names would cause false positives.
Consider using a spy/mock pattern or extracting the step list to a constant that both the implementation and test reference.
♻️ Alternative approach
If
InstallationRunnerexposed its step order (e.g., via a constant or protected method), tests could verify against that contract without parsing source:// In InstallationRunner protected const STEP_ORDER = [ 'stepValidateConnectivity', 'stepCreateConfigFiles', // ... ]; // In test public function testRunCallsStepsInCorrectOrder(): void { $this->assertEquals( self::EXPECTED_STEPS, InstallationRunner::STEP_ORDER ); }phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/Dialect/SqliteDialect.php (1)
119-124: Include context in the SQLitemodifyColumn()exception.
It will make migration failures easier to diagnose.♻️ Suggested tweak
- throw new RuntimeException('SQLite does not support modifying columns. Use table rebuild pattern.'); + throw new RuntimeException( + sprintf( + 'SQLite does not support modifying columns. Use table rebuild pattern. Table: %s, Column: %s, Type: %s', + $tableName, + $columnName, + $newType, + ), + );tests/phpMyFAQ/Setup/Installation/SchemaInstallerTest.php (1)
30-54: Avoid hard‑coding the table count in dry‑run assertions.
Using the schema as the source of truth keeps the test resilient to future table additions.♻️ Suggested tweak
- // Should have at least one CREATE TABLE per table definition - $createTableCount = 0; + $expectedTableCount = count($installer->getSchema()->getTableNames()); + $createTableCount = 0; foreach ($sql as $statement) { if (str_contains($statement, 'CREATE TABLE')) { $createTableCount++; } } - $this->assertEquals(43, $createTableCount, 'Should generate CREATE TABLE for all 43 tables'); + $this->assertSame( + $expectedTableCount, + $createTableCount, + sprintf('Should generate CREATE TABLE for all %d tables', $expectedTableCount), + );
phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/UserCreateOperation.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@phpmyfaq/.htaccess`:
- Line 105: The .htaccess currently contains absolute local filesystem paths;
replace those with URL-relative paths: update the RewriteBase directive (symbol:
RewriteBase) to a URL path such as "/" or "/phpmyfaq/" depending on your
deployment, and change the ErrorDocument 404 directive (symbol: ErrorDocument)
to point to a URL path (e.g., "/404.html" or "/phpmyfaq/404.html") instead of
any /Users/... filesystem path.
In `@phpmyfaq/src/phpMyFAQ/Setup/Installation/SchemaInstaller.php`:
- Around line 119-135: The dropTables method currently executes DROP queries
directly and ignores dry-run and collectedSql; change
SchemaInstaller::dropTables to build the DROP TABLE statements as it does now
but call $this->executeSql($sql) (the same helper used by createTables())
instead of $this->configuration->getDb()->query(...), so each DROP is appended
to collectedSql and the dryRun flag is respected; ensure you still return false
on failure (propagate executeSql's boolean) and reference the existing
properties/methods: dropTables, executeSql, createTables, collectedSql, and
dryRun.
In `@phpmyfaq/src/phpMyFAQ/Setup/InstallationRunner.php`:
- Around line 92-99: The connect() calls created via Database::factory(...)
inside InstallationRunner must handle both exceptions and the declared ?bool
return: wrap each DatabaseDriver::connect(...) invocation in a try-catch that
catches Throwable/Exception, rethrow or convert it to an Installation-specific
failure, and explicitly check the returned value (if === false) to treat it as a
connection failure (again throwing/returning a descriptive error); also preserve
and use the DatabaseDriver instanceof check only for type validation but not as
a proxy for successful connection—ensure the catch path logs or surfaces the
original exception message so connection failures are not silently ignored.
- Around line 153-156: Change the OpenSearch client creation in
InstallationRunner to enable TLS verification by default: set 'verify_peer' =>
true instead of false in the SymfonyClientFactory()->create(...) calls (both
occurrences where $osClient is built). Add support for optional CA bundle
parameters (e.g., 'cafile' and 'capath') passed into the client config from
installation settings/env so self-signed certs can be validated in
non-production environments, and ensure any code that constructs the options for
SymfonyClientFactory (the array passed into create) merges explicit
'verify_peer' and 'cafile'/'capath' values rather than hardcoding verification
off.
🧹 Nitpick comments (6)
phpmyfaq/src/phpMyFAQ/Setup/Migration/Operations/FormInputInsertOperation.php (1)
42-50: Consider defensive array key access.The method accesses
form_id,input_id, andinput_labelkeys directly. If the array is malformed, this will throw an undefined key warning. Since this is an internal operation class with controlled inputs, this is a minor concern.🛡️ Optional defensive approach
public function getDescription(): string { return sprintf( 'Insert form input: form=%d, input=%d, label=%s', - $this->formInput['form_id'], - $this->formInput['input_id'], - $this->formInput['input_label'], + $this->formInput['form_id'] ?? 0, + $this->formInput['input_id'] ?? 0, + $this->formInput['input_label'] ?? 'unknown', ); }phpmyfaq/src/phpMyFAQ/Setup/Migration/QueryBuilder/TableBuilder.php (1)
297-300: Minor style inconsistency in column list formatting.Line 298 uses
implode(',', $ftColumns)without spaces, while line 303 usesimplode(', ', $indexDef['columns'])with a space after the comma. Consider using consistent formatting for SQL output.♻️ Suggested fix for consistency
foreach ($this->fullTextIndexes as $ftColumns) { - $columnList = implode(',', $ftColumns); + $columnList = implode(', ', $ftColumns); $parts[] = "FULLTEXT ($columnList)"; }phpmyfaq/src/phpMyFAQ/Setup/Installation/SchemaInstaller.php (1)
137-152: Minor: return statement can be simplified.The conditional return is functionally correct but verbose.
✨ Optional simplification
private function executeSql(string $sql): bool { $this->collectedSql[] = $sql; if ($this->dryRun) { return true; } - $result = $this->configuration->getDb()->query($sql); - - if (!$result) { - return false; - } - - return true; + return (bool) $this->configuration->getDb()->query($sql); }tests/phpMyFAQ/Setup/Installation/SchemaInstallerTest.php (2)
30-54: Hardcoded table count (43) may become brittle.The assertion on line 53 expects exactly 43 tables. If tables are added or removed from
DatabaseSchema, this test will fail without clear indication of what changed.✨ Consider deriving expected count from schema
+ #[DataProvider('dialectProvider')] + public function testDryRunCollectsAllSql(DialectInterface $dialect): void + { $db = $this->createStub(DatabaseDriver::class); $configuration = $this->createStub(Configuration::class); $configuration->method('getDb')->willReturn($db); $installer = new SchemaInstaller($configuration, $dialect); $installer->setDryRun(true); $result = $installer->createTables(''); $this->assertTrue($result); $sql = $installer->getCollectedSql(); $this->assertNotEmpty($sql); + $expectedTableCount = count($installer->getSchema()->getTableNames()); + // Should have at least one CREATE TABLE per table definition $createTableCount = 0; foreach ($sql as $statement) { if (str_contains($statement, 'CREATE TABLE')) { $createTableCount++; } } - $this->assertEquals(43, $createTableCount, 'Should generate CREATE TABLE for all 43 tables'); + $this->assertEquals( + $expectedTableCount, + $createTableCount, + "Should generate CREATE TABLE for all {$expectedTableCount} tables" + ); }
15-130: Consider adding test coverage fordropTables().The test suite covers
createTables()comprehensively but lacks tests fordropTables(). Key scenarios to consider:
dropTables()with explicit prefixdropTables()falling back to global prefix- Error handling when DROP fails
- Dry-run mode behavior (currently inconsistent with implementation)
phpmyfaq/src/phpMyFAQ/Setup/InstallationInputValidator.php (1)
62-67: Consider honoring$setupoverrides for non‑DB fields.
validate()is documented for programmatic installs, but language/realname/email/permLevel (and dbPrefix) always come from POST. Allowing$setupvalues keeps CLI/automation installs consistent with UI installs.♻️ Suggested refactor
- $language = Filter::filterInput(INPUT_POST, 'language', FILTER_SANITIZE_SPECIAL_CHARS, 'en'); - $realname = Filter::filterInput(INPUT_POST, 'realname', FILTER_SANITIZE_SPECIAL_CHARS, ''); - $email = Filter::filterInput(INPUT_POST, 'email', FILTER_SANITIZE_EMAIL, ''); - $permLevel = Filter::filterInput(INPUT_POST, 'permLevel', FILTER_SANITIZE_SPECIAL_CHARS, 'basic'); + $language = $setup['language'] ?? Filter::filterInput(INPUT_POST, 'language', FILTER_SANITIZE_SPECIAL_CHARS, 'en'); + $realname = $setup['realname'] ?? Filter::filterInput(INPUT_POST, 'realname', FILTER_SANITIZE_SPECIAL_CHARS, ''); + $email = $setup['email'] ?? Filter::filterInput(INPUT_POST, 'email', FILTER_SANITIZE_EMAIL, ''); + $permLevel = $setup['permLevel'] ?? Filter::filterInput(INPUT_POST, 'permLevel', FILTER_SANITIZE_SPECIAL_CHARS, 'basic'); - $dbSetup['dbPrefix'] = Filter::filterInput(INPUT_POST, 'sqltblpre', FILTER_SANITIZE_SPECIAL_CHARS, ''); + $dbSetup['dbPrefix'] = $setup['dbPrefix'] ?? Filter::filterInput(INPUT_POST, 'sqltblpre', FILTER_SANITIZE_SPECIAL_CHARS, '');Also applies to: 95-98
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Setup/InstallationRunner.php`:
- Around line 278-281: The salt is generated deterministically with
md5($this->configuration->getDefaultUrl()) in InstallationRunner; replace this
with a cryptographically secure random value (e.g., use random_bytes and encode
with bin2hex or base64_encode) when calling
$this->configuration->add('security.salt', ...); ensure you generate sufficient
entropy (e.g., 32 bytes) and handle any potential exceptions from random_bytes
so the salt is unpredictable and high-entropy.
- Around line 137-141: The ping response from $esClient->ping() in
InstallationRunner must be checked — call ->asBool() on the response and treat a
false result as a failure (e.g., throw or log and enter the catch flow) instead
of only relying on exceptions; update the InstallationRunner code around
$esClient->ping() to mirror the successful check used in the existing phpMyFAQ
Instance Search Elasticsearch implementation so that non-OK HTTP responses are
handled as errors.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Setup/Installation/SchemaInstaller.php`:
- Around line 39-42: The public typed property $collectedSql on SchemaInstaller
is declared but not initialized, which can cause a fatal uninitialized property
access before createTables() runs; initialize $collectedSql (either inline to an
empty array or in the SchemaInstaller constructor) so it is always an array when
accessed, e.g. set $collectedSql = [] in the class property or assign [] in
__construct() to ensure safe access from methods like createTables().
In `@phpmyfaq/src/phpMyFAQ/Setup/InstallationRunner.php`:
- Around line 265-274: In stepCreateDatabaseTables, treat a false return from
InstanceDatabase::factory(...)->createTables(...) as a fatal error: after
calling createTables on the $databaseInstaller, check its boolean result; if it
is false, call Installer::cleanFailedInstallationFiles() and throw a new
Exception (including a clear message that table creation failed, and optionally
the dbType/dbPrefix context) so installation stops; keep the existing catch
block for thrown Exceptions from createTables as well.
- Around line 113-121: InstallationRunner is directly accessing the private
DefaultDataSeeder::$mainConfig (via $seeder->mainConfig) which will cause a
fatal error; add a public accessor on DefaultDataSeeder such as getConfig() or
getConfigByPrefix(string $prefix) that returns the config array (or filtered
entries), then update InstallationRunner to call the new method (e.g.,
$seeder->getConfigByPrefix('ldap.') or filter $seeder->getConfig()) and iterate
over that result instead of reading $mainConfig directly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Setup/InstallationRunner.php`:
- Around line 399-413: The call in stepCreateInstance uses
Request::createFromGlobals()->getScriptName() passed into Link::getSystemUri(),
which triggers duplicate removal in getSystemRelativeUri() and can yield an
empty/incorrect URI and unreliable server globals in non-HTTP contexts; change
the setUrl invocation to call Link::getSystemUri() with no parameter (i.e., pass
null/omit the path) so Link::getSystemUri()/getSystemRelativeUri() compute the
correct relative URI internally and avoid using Request::createFromGlobals()
here (adjust the setUrl line in stepCreateInstance of InstallationRunner.php to
remove the Request::createFromGlobals() argument).
🧹 Nitpick comments (4)
phpmyfaq/src/phpMyFAQ/Setup/InstallationRunner.php (3)
361-370: Unused parameter$input.The
$inputparameter is declared but never used in this method. Consider removing it if not needed for interface consistency.🧹 Proposed fix
- private function stepGrantPermissions(InstallationInput $input): void + private function stepGrantPermissions(): void { $user = new User($this->configuration); $user->getUserById(1, true);And update the call site in
run():- $this->stepGrantPermissions($input); + $this->stepGrantPermissions();
418-446: Add exception handling forcreateIndex()calls.Lines 429 and 444 call
createIndex()on Elasticsearch/OpenSearch instances without try-catch protection. If index creation fails, it may throw an exception that bubbles up without proper cleanup or a descriptive installation error message.🛡️ Proposed fix
if ($input->esEnabled && is_file($input->rootDir . '/content/core/config/elasticsearch.php')) { $elasticsearchConfiguration = new ElasticsearchConfiguration($input->rootDir . '/content/core/config/elasticsearch.php'); $this->configuration->setElasticsearchConfig($elasticsearchConfiguration); $esClient = ClientBuilder::create()->setHosts($elasticsearchConfiguration->getHosts())->build(); $this->configuration->setElasticsearch($esClient); $elasticsearch = new Elasticsearch($this->configuration); - $elasticsearch->createIndex(); + try { + $elasticsearch->createIndex(); + } catch (\Throwable $e) { + throw new Exception(sprintf('Elasticsearch Installation Error: Could not create index: %s', $e->getMessage())); + } } if ($input->osEnabled && is_file($input->rootDir . '/content/core/config/opensearch.php')) { $openSearchConfiguration = new OpenSearchConfiguration($input->rootDir . '/content/core/config/opensearch.php'); $this->configuration->setOpenSearchConfig($openSearchConfiguration); $osClient = new SymfonyClientFactory()->create($this->buildOpenSearchClientOptions( $openSearchConfiguration->getHosts()[0], $input->osSetup, )); $this->configuration->setOpenSearch($osClient); $openSearch = new OpenSearch($this->configuration); - $openSearch->createIndex(); + try { + $openSearch->createIndex(); + } catch (\Throwable $e) { + throw new Exception(sprintf('OpenSearch Installation Error: Could not create index: %s', $e->getMessage())); + } }
288-294: Check the return value fromexecuteInsertQueriesor throw an exception on failure.Line 291 calls
$stopWords->executeInsertQueries()which returns abool, but the return value is ignored. If stopword insertion fails, the method only echoes error details to output while the installation continues without error. This is inconsistent withstepCreateDatabaseTableswhich properly checks the return value and throws an exception on failure.phpmyfaq/src/phpMyFAQ/Setup/Installation/SchemaInstaller.php (1)
108-108: AddIF EXISTSto DROP TABLE statements for improved resilience.All supported databases (MySQL, MariaDB, PostgreSQL, SQLite, SQL Server) support
DROP TABLE IF EXISTSsyntax. This change would prevent failures when tables don't exist during partial installation or repeated cleanup operations. The codebase already follows this pattern inAbstractMigration::dropTableIfExists()andQueryBuilder::dropTableIfExists().♻️ Suggested improvement
- $sql = sprintf('DROP TABLE %s%s', $prefix, $tableName); + $sql = sprintf('DROP TABLE IF EXISTS %s%s', $prefix, $tableName);
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
New Features
Enhancements
Tests