feat: added optional Redis support for configuration caching#4006
Merged
feat: added optional Redis support for configuration caching#4006
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 optional Redis-backed configuration storage: new storage interfaces and implementations (DB, Redis, hybrid), a settings resolver, admin UI and API for testing Redis, docs and docker updates, migration/default config entries, repository refactor to use the hybrid store, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Admin UI
participant FE as Frontend API
participant Ctrl as ConfigurationController
participant Hybrid as HybridConfigurationStore
participant Redis as Redis
participant DB as Database
UI->>FE: POST testRedisConnection(csrf, redisDsn, timeout)
FE->>Ctrl: /api/configuration/test-redis-connection
Ctrl->>Hybrid: resolve settings / validate connection
Hybrid->>Redis: create client / ping / select DB
alt Redis reachable
Redis-->>Hybrid: OK
Hybrid-->>Ctrl: success
Ctrl-->>FE: 200 {message}
FE-->>UI: show success
else Redis fails
Redis-->>Hybrid: error
Hybrid->>DB: fetchAll() (fallback)
DB-->>Hybrid: rows
Hybrid-->>Ctrl: error/details
Ctrl-->>FE: 400 {error}
FE-->>UI: show error
end
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: 10
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/Setup/Migration/Versions/Migration420Alpha.php (1)
39-42:⚠️ Potential issue | 🟡 Minor
getDescription()does not reflect the new Redis storage config entries.The description currently reads:
'Admin log hash columns, custom pages, chat messages, translation config, API rate limiting, queue jobs, mail provider config, API keys, OAuth2 tables'. The fourstorage.*config entries are not mentioned, making it harder for developers to understand the full scope of this migration.📝 Proposed fix
public function getDescription(): string { - return 'Admin log hash columns, custom pages, chat messages, translation config, API rate limiting, queue jobs, mail provider config, API keys, OAuth2 tables'; + return 'Admin log hash columns, custom pages, chat messages, translation config, API rate limiting, queue jobs, mail provider config, API keys, OAuth2 tables, Redis-backed configuration storage settings'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php` around lines 39 - 42, Update the migration description returned by getDescription() in Migration420Alpha so it lists the four new storage.* configuration entries added by this migration; locate the getDescription() method in the Migration420Alpha class and append a clear mention of the storage.* config changes (e.g. "storage.* config entries") to the existing comma-separated description string so the full scope — Admin log hash columns, custom pages, chat messages, translation config, API rate limiting, queue jobs, mail provider config, API keys, OAuth2 tables, and the storage.* config entries — is reflected.phpmyfaq/src/phpMyFAQ/Configuration/ConfigurationRepository.php (1)
92-104:⚠️ Potential issue | 🟠 Major
updateFaqDataContentuses content column for WHERE clause — will match and update all rows with identical content.The method performs a bulk find-and-replace by matching on the exact content value. In
replaceMainReferenceUrl(), this is called iteratively for each FAQ; if multiple FAQs share identical content, the WHERE clause will match all of them, causing unintended simultaneous updates. Consider whether content uniqueness should be enforced or if the method should operate by FAQ ID instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Configuration/ConfigurationRepository.php` around lines 92 - 104, The current updateFaqDataContent(string $oldContent, string $newContent) updates rows by matching content which can update multiple FAQs sharing identical content; change it to target a single FAQ by ID (e.g., add/replace with updateFaqDataContentById(int $faqId, string $newContent) or add a $faqId parameter) and update the SQL to use the faq_id in the WHERE clause instead of content. Ensure you escape or preferably parameterize the newContent and faqId using the existing coreConfiguration->getDb() methods, update call sites such as replaceMainReferenceUrl() to pass the specific FAQ id, and remove the old content-based bulk-update usage.
🧹 Nitpick comments (14)
phpmyfaq/src/phpMyFAQ/Controller/Administration/SystemInformationController.php (1)
100-101:?? 'n/a'null-coalescing operators are redundantBoth
$esInformationand$openSearchInformationare guaranteed to be assigned before reaching these lines:
$esInformationis always set — either in thetry/catchblock (lines 55–63) or the explicit negativeifat lines 65–67, which together cover the entire boolean space.$openSearchInformationis unconditionally initialized to''at line 69, so it can never be null or undefined.The
?? 'n/a'operators are therefore dead code and can be dropped to keep the intent clear.♻️ Proposed cleanup
- 'Elasticsearch Version' => $esInformation ?? 'n/a', - 'OpenSearch Version' => $openSearchInformation ?? 'n/a', + 'Elasticsearch Version' => $esInformation, + 'OpenSearch Version' => $openSearchInformation,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/SystemInformationController.php` around lines 100 - 101, The null-coalescing fallbacks are redundant for the 'Elasticsearch Version' and 'OpenSearch Version' entries because $esInformation is always assigned (via the try/catch and the explicit negative if) and $openSearchInformation is initialized to '' before use; remove the "?? 'n/a'" from the array entries so they read simply 'Elasticsearch Version' => $esInformation and 'OpenSearch Version' => $openSearchInformation in SystemInformationController (class/method where these variables are prepared).phpmyfaq/src/phpMyFAQ/Configuration/Storage/ConfigurationStoreInterface.php (2)
26-29:@return array<int, object>is overly broad — preferarray<int, \stdClass>.Database fetch results are typically
\stdClassinstances. Narrowing the return type annotation improves static analysis and IDE support.♻️ Proposed fix
/** - * `@return` array<int, object> + * `@return` array<int, \stdClass> */ public function fetchAll(): array;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/ConfigurationStoreInterface.php` around lines 26 - 29, The docblock for ConfigurationStoreInterface::fetchAll() is too generic; update its `@return` annotation from "array<int, object>" to "array<int, \stdClass>" so static analyzers and IDEs know the fetched DB rows are stdClass instances; locate the fetchAll method in the ConfigurationStoreInterface and replace the return type in the PHPDoc accordingly while preserving the method signature.
24-35: Inconsistent parameter naming between$keyand$namefor the same concept.
updateConfigValueuses$key, whileinsertanddeleteuse$name, andrenameKeyuses$currentKey/$newKey. Since all refer to the configuration identifier, pick one term (e.g.,$key) and apply it consistently across all methods.♻️ Proposed uniform naming
- public function updateConfigValue(string $key, string $value): bool; + public function updateConfigValue(string $key, string $value): bool; // already $key - public function insert(string $name, string $value): bool; + public function insert(string $key, string $value): bool; - public function delete(string $name): bool; + public function delete(string $key): bool; - public function renameKey(string $currentKey, string $newKey): bool; + public function renameKey(string $fromKey, string $toKey): bool;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/ConfigurationStoreInterface.php` around lines 24 - 35, The method parameter names are inconsistent: updateConfigValue uses $key while insert/delete use $name and renameKey uses $currentKey/$newKey; standardize to a single term (use $key) across the interface. Update insert(string $name, string $value): bool to insert(string $key, string $value): bool, delete(string $name): bool to delete(string $key): bool, and renameKey(string $currentKey, string $newKey): bool to renameKey(string $key, string $newKey): bool (or rename to renameKey(string $oldKey, string $newKey): bool if you prefer old/new wording), and adjust any associated phpdoc types/comments to reference $key consistently; keep updateConfigValue(string $key, string $value): bool and fetchAll(): array unchanged.phpmyfaq/assets/templates/admin/configuration/macros.twig (1)
82-94: Macro namesendTestMailButtonis misleading — rename to reflect its generic purpose.The macro now handles any config action button (mail test, Redis test, future additions). The current name implies mail-only behaviour.
♻️ Suggested rename
-{% macro sendTestMailButton(label, key, description) %} +{% macro generateActionButton(label, key, description) %}Update all call sites in
main.twigandtab-list.twigaccordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/assets/templates/admin/configuration/macros.twig` around lines 82 - 94, Rename the misleading macro sendTestMailButton to a generic name (e.g., configActionButton) and update all call sites that reference it (notably in main.twig and tab-list.twig) to use the new macro name; ensure the macro signature ({% macro sendTestMailButton(label, key, description) %}) is renamed consistently (e.g., {% macro configActionButton(label, key, description) %}) and that the internal usage of key (id "btn-phpmyfaq-{{ key|replace({'.': '-'}) }}" and data attribute data-pmf-config-action="{{ action }}") remains unchanged so behavior is preserved.phpmyfaq/admin/assets/src/configuration/configuration.test.ts (1)
229-281: Tests cover success paths; consider adding error-path coverage for completeness.
handleSendTestMailhas no test for the missing-CSRF or API-failure case.handleTestRedisConnectionhas no test for the missing-DSN or API-error path. These are documented in the function docstrings as handled cases.✨ Suggested additions
// Inside describe('handleSendTestMail') it('should push error notification when CSRF token is missing', async () => { document.body.innerHTML = `<div></div>`; // no csrf input await handleSendTestMail(); expect(sendTestMail).not.toHaveBeenCalled(); }); // Inside describe('handleTestRedisConnection') it('should push error notification when Redis DSN is missing', async () => { document.body.innerHTML = ` <input id="pmf-csrf-token" value="csrf-token" /> <input id="edit[storage.redisConnectTimeout]" value="1.5" /> <button id="btn-phpmyfaq-storage-testRedisConnection">Test Redis connection</button> `; await handleTestRedisConnection(); expect(testRedisConnection).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/admin/assets/src/configuration/configuration.test.ts` around lines 229 - 281, The tests only cover success paths for handleSendTestMail and handleTestRedisConnection; add negative-path tests: for handleSendTestMail add a test that leaves out the pmf-csrf-token input, asserts sendTestMail is not called and that the proper error/notification flow runs; for handleTestRedisConnection add tests that omit edit[storage.redisDsn] (assert testRedisConnection not called) and simulate API failure by mocking testRedisConnection to reject or resolve with success: false and assert the error/notification handling and button state; reference the functions handleSendTestMail, handleTestRedisConnection, sendTestMail, testRedisConnection and the button id btn-phpmyfaq-storage-testRedisConnection when adding these tests.phpmyfaq/admin/assets/src/api/configuration.ts (1)
157-173: MissingContent-Type: application/jsonheader for JSON request bodies.Both
sendTestMailandtestRedisConnectionserialize the body withJSON.stringifybut do not set theContent-Typeheader. While Symfony's$request->getContent()currently reads the raw body regardless, the missing header violates HTTP semantics and can cause issues with reverse proxies, WAFs, or middleware that inspect content type.♻️ Proposed fix
export const sendTestMail = async (csrf: string): Promise<Response> => { return (await fetchJson('api/configuration/send-test-mail', { method: 'POST', + headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ csrf }), })) as Response; }; export const testRedisConnection = async (csrf: string, redisDsn: string, timeout: number): Promise<Response> => { return (await fetchJson('api/configuration/test-redis-connection', { method: 'POST', + headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ csrf, redisDsn, timeout, }), })) as Response; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/admin/assets/src/api/configuration.ts` around lines 157 - 173, Both sendTestMail and testRedisConnection send JSON bodies but omit the Content-Type header; update their fetchJson calls (in functions sendTestMail and testRedisConnection) to include headers: { 'Content-Type': 'application/json' } so the request properly advertises JSON payloads (ensure you merge with any existing headers option rather than replacing other options).phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ConfigurationController.php (2)
122-125: Hardcoded success message is inconsistent with the rest of the controller.
activateMaintenanceModereturnsTranslation::get('healthCheckOkay')for its success message. Using a raw English string here creates an i18n inconsistency.🌐 Proposed fix
return $this->json([ 'success' => true, - 'message' => 'Redis connection successful.', + 'message' => Translation::get(key: 'msgRedisConnectionSuccessful'), ], Response::HTTP_OK);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ConfigurationController.php` around lines 122 - 125, The success response in ConfigurationController currently returns a hardcoded English string ("Redis connection successful."); replace that literal with the i18n helper used elsewhere (e.g. Translation::get('healthCheckOkay') or a more specific translation key if available) so the JSON response uses Translation::get(...) like activateMaintenanceMode does, updating the 'message' value in the return $this->json(...) statement to call Translation::get(...) instead of the raw string.
113-117: Guard against an empty$redisDsnbefore delegating tovalidateConnection.If neither the request nor the stored config provides a DSN,
$redisDsnwill be'', andRedisSessionHandler::validateConnection('')will propagate a low-quality error to the caller. An explicit early return gives a clearer response.🛡️ Proposed guard
$redisDsn = trim((string) ($data->redisDsn ?? $this->configuration->get('storage.redisDsn') ?? '')); +if ($redisDsn === '') { + return $this->json(['error' => 'Redis DSN is required.'], Response::HTTP_BAD_REQUEST); +} $timeout = (float) ($data->timeout ?? $this->configuration->get('storage.redisConnectTimeout') ?? 1.0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ConfigurationController.php` around lines 113 - 117, The code calls RedisSessionHandler::validateConnection with $redisDsn that may be an empty string; add an explicit guard in ConfigurationController before calling validateConnection to check if trim($redisDsn) === '' and short-circuit (return or set a clear error/validation response) so you don't delegate an empty DSN to RedisSessionHandler; reference the variables $redisDsn and $timeout and the method RedisSessionHandler::validateConnection in your change.phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php (2)
66-70:delete()returnstrueeven when the key didn't exist.
hDelreturns the count of removed fields.>= 0is always true (it returns 0 or more). If the intent is idempotent delete, this is fine. If callers need to know whether the key was actually present, use> 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php` around lines 66 - 70, The delete method in RedisConfigurationStore::delete uses redis->hDel(... ) >= 0 which is always true because hDel returns 0 or more; change the comparison to > 0 so the method returns true only when a field was actually removed (i.e., replace the >= 0 check with > 0 on the hDel result in delete()).
120-172: A new Redis TCP connection is opened for every single operation.
createRedisClient()performs DNS resolution, TCP connect, optional auth, and database select on every method call. For a caching layer meant to reduce latency, this connection overhead likely negates the benefit over a simple DB query.Consider caching the
Redisinstance as a class property (lazy-initialized on first use), or accepting an already-connectedRedisinstance via the constructor.Sketch: lazy-initialize and reuse the Redis client
-readonly class RedisConfigurationStore implements ConfigurationStoreInterface +class RedisConfigurationStore implements ConfigurationStoreInterface { + private ?Redis $redis = null; + public function __construct( private readonly ConfigurationStorageSettings $settings, ) { } // ... (public methods stay the same, calling $this->getRedisClient()) - private function createRedisClient(): Redis + private function getRedisClient(): Redis { + if ($this->redis !== null) { + return $this->redis; + } + if (!extension_loaded('redis')) { throw new RuntimeException('...'); } // ... connection logic ... - return $redis; + $this->redis = $redis; + return $this->redis; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php` around lines 120 - 172, createRedisClient() currently creates a new Redis connection for every call (DNS, connect, auth, select), causing high overhead; modify RedisConfigurationStore to cache and reuse a single Redis instance by adding a private class property (e.g., $redisClient) and change createRedisClient() to lazy-initialize that property on first call and return it thereafter, or alternatively allow injection of an already-connected Redis instance via the constructor and use that instead; ensure the existing connect/auth/select logic runs only once when initializing $redisClient and subsequent calls simply return the stored Redis object.phpmyfaq/admin/assets/src/configuration/configuration.ts (1)
519-542: Consider unifying the error-message extraction pattern.Both
handleSendTestMailandhandleTestRedisConnectionhave identical error-message fallback logic (lines 533–538 and 582–586). A small shared helper would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/admin/assets/src/configuration/configuration.ts` around lines 519 - 542, Both handleSendTestMail and handleTestRedisConnection repeat the same error-message extraction logic; create a small shared helper (e.g., extractResponseErrorMessage(response: unknown): string) that encapsulates the fallbacks (response.error || response.message || typeof response.success === 'string' ? response.success : '' || default message) and use it in both handleSendTestMail and handleTestRedisConnection to replace the duplicated errorMessage construction and subsequent pushErrorNotification calls.phpmyfaq/src/phpMyFAQ/Configuration/Storage/DatabaseConfigurationStore.php (1)
33-47: Consider parameterized queries instead ofsprintf+escape().The current approach of escaping user inputs and interpolating into SQL strings via
sprintfworks but is less robust than prepared statements. If theDatabaseDriversupports prepared statements, this would be a safer pattern. That said, this appears to be the existing codebase convention, so this is a low-priority improvement.Also applies to: 64-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/DatabaseConfigurationStore.php` around lines 33 - 47, Replace string interpolation with a parameterized prepared statement in updateConfigValue: instead of building the SQL via sprintf and escape in DatabaseConfigurationStore::updateConfigValue, use the DatabaseDriver's prepare/execute (or equivalent prepareQuery/bind/execute) API to bind the config_value and config_name parameters; ensure you still include the table name via Database::getTablePrefix() and $this->tableName (these cannot be parameterized) and execute the prepared statement, returning the boolean result. Apply the same change pattern to the other similar method(s) around the 64-78 region that build SQL with sprintf/escape so all updates use prepared statements and bound parameters.phpmyfaq/src/phpMyFAQ/Configuration/Storage/HybridConfigurationStore.php (2)
59-88: fetchAll: doubleresolveRedisStore()call creates two independent Redis connections.When Redis is enabled but the cache is empty (cold start), the flow resolves a Redis store at line 61 (opens connection 1, queries, gets empty), then resolves again at line 77 (opens connection 2, warms). The second call could reuse the store from the first.
Proposed fix
public function fetchAll(): array { $redisStore = $this->resolveRedisStore(); if ($redisStore !== null) { try { $rows = $redisStore->fetchAll(); if ($rows !== []) { return $rows; } } catch (RuntimeException $exception) { $this->logger->warning('Failed to fetch configuration from Redis storage.', [ 'error' => $exception->getMessage(), ]); + $redisStore = null; // skip warming on fetch failure } } $rows = $this->databaseConfigurationStore->fetchAll(); - if ($redisStore !== null && $rows !== []) { + if ($redisStore !== null && $rows !== []) { try { $redisStore->warmFromRows($rows); } catch (RuntimeException $exception) {The main improvement here would come from the caching suggestion above. But even without it, if the first
resolveRedisStore()call already returned a store, you already reuse$redisStorefor warming — which is good. The issue is thatRedisConfigurationStore::createRedisClient()creates a new connection per method call internally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/HybridConfigurationStore.php` around lines 59 - 88, fetchAll() is calling resolveRedisStore() twice which causes RedisConfigurationStore::createRedisClient() to open two separate Redis connections; change the implementation so the same Redis client/connection is reused instead of recreated — either cache the resolved store/client inside HybridConfigurationStore (store the first resolveRedisStore() result in a property and reuse it for warming), or modify RedisConfigurationStore to lazily instantiate and reuse a single client (e.g., keep a private $client and return it from createRedisClient()), ensuring resolveRedisStore(), fetchAll(), and RedisConfigurationStore::createRedisClient() use that single shared connection.
157-165: New Redis connection created on every single operation.
resolveRedisStore()instantiates a newRedisConfigurationStoreper call, and each method inRedisConfigurationStorecallscreateRedisClient()which opens a fresh TCP/socket connection. ForfetchAll(), this is called twice (lines 61 and 77), doubling the overhead.On hot paths (e.g., every page load reading configuration), this means a DB query to resolve settings + a new Redis TCP connection per operation, which undermines the caching benefit.
Consider caching the resolved
RedisConfigurationStoreinstance (and its underlyingRedisconnection) for the lifetime of the request, while still allowing the enabled flag to be re-evaluated if needed.Sketch: cache the resolved store per request
readonly class HybridConfigurationStore implements ConfigurationStoreInterface { + private ?RedisConfigurationStore $cachedRedisStore = null; + private bool $redisStoreResolved = false; + // ... private function resolveRedisStore(): ?RedisConfigurationStore { + if ($this->redisStoreResolved) { + return $this->cachedRedisStore; + } + $settings = $this->settingsResolver->resolve(); + $this->redisStoreResolved = true; + if (!$settings->enabled) { + $this->cachedRedisStore = null; return null; } - return new RedisConfigurationStore($settings); + $this->cachedRedisStore = new RedisConfigurationStore($settings); + return $this->cachedRedisStore; } }Note: this would also require removing the
readonlymodifier from the class, since mutable properties are introduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yml`:
- Around line 86-90: The compose file currently uses depends_on with "redis:
condition: service_healthy" (see the depends_on blocks referencing redis at the
shown diffs and repeats around the other locations) which forces services to
wait for Redis health and blocks startup when Redis is intentionally unused;
update the docker-compose.yml to either make Redis optional or document the
expectation: replace the strict health condition with a looser dependency (e.g.,
remove the condition or use service_started) on the depends_on entries that
reference "redis", and/or add a clear comment next to the redis service
definition explaining that Redis must be running for the dev stack (so
contributors know it’s required), applying the same change/comments to the other
repeated blocks (the other depends_on occurrences noted).
In `@docs/administration.md`:
- Around line 799-816: The "Redis-backed configuration storage" section titled
"5.6.5 Redis-backed configuration storage" is out of order and duplicates an
existing 5.6.5; move the entire section (the header and its content) so it
follows 5.6.4, then renumber that header to 5.6.5; after doing so increment the
existing "Elasticsearch configuration" and the subsequent two sections currently
numbered 5.6.5–5.6.7 to 5.6.6–5.6.8 respectively to avoid duplicate numbers and
preserve ordering.
In `@mkdocs.yml`:
- Line 18: The mkdocs.yml navigation references a missing page
'redis-configuration-storage.md' which will break the site build; either add a
new docs/redis-configuration-storage.md file with the Redis configuration
storage documentation or remove the navigation entry for
'redis-configuration-storage.md' from mkdocs.yml (the unique symbol to locate is
the string 'redis-configuration-storage.md' in the nav list).
In `@phpmyfaq/admin/assets/src/configuration/configuration.ts`:
- Around line 598-612: The current setupRedisTestButtonState creates a new
updateState closure on each call so
redisDsnInput.removeEventListener(updateState) never removes prior listeners;
refactor by declaring a module-scoped variable (e.g., let redisInputListener:
EventListener | null) and assign the listener function to it, then when
setupRedisTestButtonState runs check if redisInputListener exists and call
redisDsnInput.removeEventListener('input', redisInputListener) before
creating/assigning a new listener and adding it with addEventListener; keep the
existing logic for disabling the button using redisDsnInput.value.trim() === ''
and call the listener once to initialize.
In `@phpmyfaq/assets/templates/admin/configuration/macros.twig`:
- Line 83: The template assigns action via a ternary to '' when key is unknown
(variable action in macros.twig), which leaves buttons inert; update the logic
in the macros where action is set (the line that checks key ==
'mail.sendTestEmail' and key == 'storage.testRedisConnection') to explicitly
fail or surface a warning for unknown keys instead of returning an empty
string—e.g., add an {% else %} branch that throws a template error or sets
action to a sentinel like 'unsupported-action' and renders a visible warning
comment so future additions are noticed and buttons don't silently do nothing.
In `@phpmyfaq/assets/templates/admin/configuration/main.twig`:
- Around line 128-133: The new <li> for the storage tab (the element with
data-config-label="{{ 'storageControlCenter' | translate }}" and icon class
"bi-database-gear") is tagged data-config-group="communication" but placed after
the Integrations group items, causing DOM/grouping mismatch; fix by either
moving that <li> to appear before the Integrations group header so it stays in
the "communication" group, or change its data-config-group value to
"integrations" so it logically matches its current DOM position and
Redis/integration semantics.
In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php`:
- Around line 159-169: The calls to $redis->auth(...) and
$redis->select($database) in RedisConfigurationStore are not checked for
failure; update the code around the auth and select calls to verify their
boolean return values and handle failures (e.g., throw a descriptive exception
or log and abort initialization) so auth failures or invalid DB selection aren’t
silent. Specifically, after calling $redis->auth($parsedUrl['pass']) and
$redis->select($database) in the RedisConfigurationStore class, check for ===
false and raise/return an error with context (include the attempted database
index or note authentication failed) so callers know initialization failed.
In `@phpmyfaq/translations/language_en.php`:
- Around line 1694-1698: The description for $LANG_CONF['storage.redisDsn']
contains a Docker-specific hint "(Docker service name)" that may confuse
non-Docker users; update the human-facing string to remove or generalize the
Docker wording—e.g., keep example DSNs but remove the parenthetical or replace
with a neutral phrase like "service hostname"—so the admin UI shows a clear,
implementation-agnostic hint for Redis DSN users.
In `@tests/phpMyFAQ/Configuration/ConfigurationStorageSettingsResolverTest.php`:
- Around line 1-3: Add a strict types declaration at the top of the test file:
insert declare(strict_types=1); immediately after the opening <?php tag and
before the namespace phpMyFAQ\Configuration (affecting the
ConfigurationStorageSettingsResolverTest class in this file) so the file
complies with PER Coding Style 3.0.
In `@tests/phpMyFAQ/Configuration/HybridConfigurationStoreTest.php`:
- Around line 1-3: Add the PHP strict types declaration to this test file by
inserting declare(strict_types=1); immediately after the opening <?php tag to
comply with PER Coding Style 3.0; update the HybridConfigurationStoreTest file
(class HybridConfigurationStoreTest in namespace phpMyFAQ\Configuration) so the
file begins with <?php followed by declare(strict_types=1); and then the
namespace declaration.
---
Outside diff comments:
In `@phpmyfaq/src/phpMyFAQ/Configuration/ConfigurationRepository.php`:
- Around line 92-104: The current updateFaqDataContent(string $oldContent,
string $newContent) updates rows by matching content which can update multiple
FAQs sharing identical content; change it to target a single FAQ by ID (e.g.,
add/replace with updateFaqDataContentById(int $faqId, string $newContent) or add
a $faqId parameter) and update the SQL to use the faq_id in the WHERE clause
instead of content. Ensure you escape or preferably parameterize the newContent
and faqId using the existing coreConfiguration->getDb() methods, update call
sites such as replaceMainReferenceUrl() to pass the specific FAQ id, and remove
the old content-based bulk-update usage.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php`:
- Around line 39-42: Update the migration description returned by
getDescription() in Migration420Alpha so it lists the four new storage.*
configuration entries added by this migration; locate the getDescription()
method in the Migration420Alpha class and append a clear mention of the
storage.* config changes (e.g. "storage.* config entries") to the existing
comma-separated description string so the full scope — Admin log hash columns,
custom pages, chat messages, translation config, API rate limiting, queue jobs,
mail provider config, API keys, OAuth2 tables, and the storage.* config entries
— is reflected.
---
Nitpick comments:
In `@phpmyfaq/admin/assets/src/api/configuration.ts`:
- Around line 157-173: Both sendTestMail and testRedisConnection send JSON
bodies but omit the Content-Type header; update their fetchJson calls (in
functions sendTestMail and testRedisConnection) to include headers: {
'Content-Type': 'application/json' } so the request properly advertises JSON
payloads (ensure you merge with any existing headers option rather than
replacing other options).
In `@phpmyfaq/admin/assets/src/configuration/configuration.test.ts`:
- Around line 229-281: The tests only cover success paths for handleSendTestMail
and handleTestRedisConnection; add negative-path tests: for handleSendTestMail
add a test that leaves out the pmf-csrf-token input, asserts sendTestMail is not
called and that the proper error/notification flow runs; for
handleTestRedisConnection add tests that omit edit[storage.redisDsn] (assert
testRedisConnection not called) and simulate API failure by mocking
testRedisConnection to reject or resolve with success: false and assert the
error/notification handling and button state; reference the functions
handleSendTestMail, handleTestRedisConnection, sendTestMail, testRedisConnection
and the button id btn-phpmyfaq-storage-testRedisConnection when adding these
tests.
In `@phpmyfaq/admin/assets/src/configuration/configuration.ts`:
- Around line 519-542: Both handleSendTestMail and handleTestRedisConnection
repeat the same error-message extraction logic; create a small shared helper
(e.g., extractResponseErrorMessage(response: unknown): string) that encapsulates
the fallbacks (response.error || response.message || typeof response.success ===
'string' ? response.success : '' || default message) and use it in both
handleSendTestMail and handleTestRedisConnection to replace the duplicated
errorMessage construction and subsequent pushErrorNotification calls.
In `@phpmyfaq/assets/templates/admin/configuration/macros.twig`:
- Around line 82-94: Rename the misleading macro sendTestMailButton to a generic
name (e.g., configActionButton) and update all call sites that reference it
(notably in main.twig and tab-list.twig) to use the new macro name; ensure the
macro signature ({% macro sendTestMailButton(label, key, description) %}) is
renamed consistently (e.g., {% macro configActionButton(label, key, description)
%}) and that the internal usage of key (id "btn-phpmyfaq-{{ key|replace({'.':
'-'}) }}" and data attribute data-pmf-config-action="{{ action }}") remains
unchanged so behavior is preserved.
In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/ConfigurationStoreInterface.php`:
- Around line 26-29: The docblock for ConfigurationStoreInterface::fetchAll() is
too generic; update its `@return` annotation from "array<int, object>" to
"array<int, \stdClass>" so static analyzers and IDEs know the fetched DB rows
are stdClass instances; locate the fetchAll method in the
ConfigurationStoreInterface and replace the return type in the PHPDoc
accordingly while preserving the method signature.
- Around line 24-35: The method parameter names are inconsistent:
updateConfigValue uses $key while insert/delete use $name and renameKey uses
$currentKey/$newKey; standardize to a single term (use $key) across the
interface. Update insert(string $name, string $value): bool to insert(string
$key, string $value): bool, delete(string $name): bool to delete(string $key):
bool, and renameKey(string $currentKey, string $newKey): bool to
renameKey(string $key, string $newKey): bool (or rename to renameKey(string
$oldKey, string $newKey): bool if you prefer old/new wording), and adjust any
associated phpdoc types/comments to reference $key consistently; keep
updateConfigValue(string $key, string $value): bool and fetchAll(): array
unchanged.
In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/DatabaseConfigurationStore.php`:
- Around line 33-47: Replace string interpolation with a parameterized prepared
statement in updateConfigValue: instead of building the SQL via sprintf and
escape in DatabaseConfigurationStore::updateConfigValue, use the
DatabaseDriver's prepare/execute (or equivalent prepareQuery/bind/execute) API
to bind the config_value and config_name parameters; ensure you still include
the table name via Database::getTablePrefix() and $this->tableName (these cannot
be parameterized) and execute the prepared statement, returning the boolean
result. Apply the same change pattern to the other similar method(s) around the
64-78 region that build SQL with sprintf/escape so all updates use prepared
statements and bound parameters.
In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/HybridConfigurationStore.php`:
- Around line 59-88: fetchAll() is calling resolveRedisStore() twice which
causes RedisConfigurationStore::createRedisClient() to open two separate Redis
connections; change the implementation so the same Redis client/connection is
reused instead of recreated — either cache the resolved store/client inside
HybridConfigurationStore (store the first resolveRedisStore() result in a
property and reuse it for warming), or modify RedisConfigurationStore to lazily
instantiate and reuse a single client (e.g., keep a private $client and return
it from createRedisClient()), ensuring resolveRedisStore(), fetchAll(), and
RedisConfigurationStore::createRedisClient() use that single shared connection.
In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php`:
- Around line 66-70: The delete method in RedisConfigurationStore::delete uses
redis->hDel(... ) >= 0 which is always true because hDel returns 0 or more;
change the comparison to > 0 so the method returns true only when a field was
actually removed (i.e., replace the >= 0 check with > 0 on the hDel result in
delete()).
- Around line 120-172: createRedisClient() currently creates a new Redis
connection for every call (DNS, connect, auth, select), causing high overhead;
modify RedisConfigurationStore to cache and reuse a single Redis instance by
adding a private class property (e.g., $redisClient) and change
createRedisClient() to lazy-initialize that property on first call and return it
thereafter, or alternatively allow injection of an already-connected Redis
instance via the constructor and use that instead; ensure the existing
connect/auth/select logic runs only once when initializing $redisClient and
subsequent calls simply return the stored Redis object.
In
`@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ConfigurationController.php`:
- Around line 122-125: The success response in ConfigurationController currently
returns a hardcoded English string ("Redis connection successful."); replace
that literal with the i18n helper used elsewhere (e.g.
Translation::get('healthCheckOkay') or a more specific translation key if
available) so the JSON response uses Translation::get(...) like
activateMaintenanceMode does, updating the 'message' value in the return
$this->json(...) statement to call Translation::get(...) instead of the raw
string.
- Around line 113-117: The code calls RedisSessionHandler::validateConnection
with $redisDsn that may be an empty string; add an explicit guard in
ConfigurationController before calling validateConnection to check if
trim($redisDsn) === '' and short-circuit (return or set a clear error/validation
response) so you don't delegate an empty DSN to RedisSessionHandler; reference
the variables $redisDsn and $timeout and the method
RedisSessionHandler::validateConnection in your change.
In
`@phpmyfaq/src/phpMyFAQ/Controller/Administration/SystemInformationController.php`:
- Around line 100-101: The null-coalescing fallbacks are redundant for the
'Elasticsearch Version' and 'OpenSearch Version' entries because $esInformation
is always assigned (via the try/catch and the explicit negative if) and
$openSearchInformation is initialized to '' before use; remove the "?? 'n/a'"
from the array entries so they read simply 'Elasticsearch Version' =>
$esInformation and 'OpenSearch Version' => $openSearchInformation in
SystemInformationController (class/method where these variables are prepared).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
CHANGELOG.mddocker-compose.ymldocs/administration.mddocs/installation.mdmkdocs.ymlphpmyfaq/admin/assets/src/api/configuration.test.tsphpmyfaq/admin/assets/src/api/configuration.tsphpmyfaq/admin/assets/src/api/fetch-wrapper.test.tsphpmyfaq/admin/assets/src/api/fetch-wrapper.tsphpmyfaq/admin/assets/src/configuration/configuration.test.tsphpmyfaq/admin/assets/src/configuration/configuration.tsphpmyfaq/assets/templates/admin/configuration/macros.twigphpmyfaq/assets/templates/admin/configuration/main.twigphpmyfaq/assets/templates/admin/configuration/tab-list.twigphpmyfaq/src/phpMyFAQ/Configuration/ConfigurationRepository.phpphpmyfaq/src/phpMyFAQ/Configuration/Storage/ConfigurationStorageSettings.phpphpmyfaq/src/phpMyFAQ/Configuration/Storage/ConfigurationStorageSettingsResolver.phpphpmyfaq/src/phpMyFAQ/Configuration/Storage/ConfigurationStoreInterface.phpphpmyfaq/src/phpMyFAQ/Configuration/Storage/DatabaseConfigurationStore.phpphpmyfaq/src/phpMyFAQ/Configuration/Storage/HybridConfigurationStore.phpphpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ConfigurationController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/SystemInformationController.phpphpmyfaq/src/phpMyFAQ/Setup/Installation/DefaultDataSeeder.phpphpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.phpphpmyfaq/translations/language_en.phptests/phpMyFAQ/Auth/OAuth2/AuthorizationServerTest.phptests/phpMyFAQ/Configuration/ConfigurationStorageSettingsResolverTest.phptests/phpMyFAQ/Configuration/HybridConfigurationStoreTest.phptests/phpMyFAQ/Controller/Administration/Api/ConfigurationControllerTest.phptests/phpMyFAQ/Setup/Installation/DefaultDataSeederTest.phptests/phpMyFAQ/Setup/Migration/MigrationRegistryTest.php
💤 Files with no reviewable changes (1)
- tests/phpMyFAQ/Auth/OAuth2/AuthorizationServerTest.php
phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php
Outdated
Show resolved
Hide resolved
tests/phpMyFAQ/Configuration/ConfigurationStorageSettingsResolverTest.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
phpmyfaq/admin/assets/src/configuration/configuration.ts (1)
598-618:setupRedisTestButtonStatelistener management correctly addressed.The module-scoped
redisInputListenervariable is properly used to remove the prior listener before registering a new one, and the initial state is seeded by calling the listener once. This resolves the listener-accumulation bug from the previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/admin/assets/src/configuration/configuration.ts` around lines 598 - 618, The listener-accumulation bug has been addressed by using the module-scoped variable redisInputListener in setupRedisTestButtonState to remove the previous listener before adding a new one and by calling redisInputListener() to seed the initial state; leave the implementation of setupRedisTestButtonState and the redisInputListener handling as-is (no further changes required).phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php (1)
172-176: Previous concern about uncheckedauth()andselect()has been addressed.Both
auth()andselect()return values are now properly checked with exceptions thrown on failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php` around lines 172 - 176, The RedisConfigurationStore previously did not check Redis auth/select return values; update the RedisConfigurationStore class so that in the block using $redis->auth($parsedUrl['pass']) and $redis->select($dbIndex) you validate the return values and throw a RuntimeException on failure (e.g., "Redis authentication failed for configuration storage." and a similar message for select) to fail fast and surface errors from authentication/selection.tests/phpMyFAQ/Configuration/HybridConfigurationStoreTest.php (1)
1-4: Previous concern about missingdeclare(strict_types=1)has been addressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Configuration/HybridConfigurationStoreTest.php` around lines 1 - 4, The previous issue about adding declare(strict_types=1) has been resolved in tests/phpMyFAQ/Configuration/HybridConfigurationStoreTest.php; no further changes are required—keep the declared strict types and proceed with approval of the test file (HybridConfigurationStoreTest).tests/phpMyFAQ/Configuration/ConfigurationStorageSettingsResolverTest.php (1)
1-4: Previous concern about missingdeclare(strict_types=1)has been addressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Configuration/ConfigurationStorageSettingsResolverTest.php` around lines 1 - 4, The file now includes the required declare(strict_types=1) directive at the top (the declare statement), so no further changes are needed; leave the declare(strict_types=1) line as-is and proceed to approve the change.
🧹 Nitpick comments (5)
phpmyfaq/admin/assets/src/configuration/configuration.ts (3)
497-517:registerConfigurationActionButtonsis needlesslyasync.The function body has no
awaitand theforEachcallbacks are synchronous. Theasynckeyword and theawaitat the two call sites (lines 144, 147) are no-ops. Remove both to keep the signature honest.♻️ Proposed fix
-const registerConfigurationActionButtons = async (): Promise<void> => { +const registerConfigurationActionButtons = (): void => {- case '#mail': - await handleSMTPPasswordToggle(); - await handleMailProvider(); - await registerConfigurationActionButtons(); + case '#mail': + await handleSMTPPasswordToggle(); + await handleMailProvider(); + registerConfigurationActionButtons(); break; - case '#storage': - await registerConfigurationActionButtons(); - setupRedisTestButtonState(); + case '#storage': + registerConfigurationActionButtons(); + setupRedisTestButtonState(); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/admin/assets/src/configuration/configuration.ts` around lines 497 - 517, The function registerConfigurationActionButtons is declared async but contains no awaits; remove the async modifier from its declaration and change its return type to void (or inferred), and then remove the unnecessary await operators at its call sites (where registerConfigurationActionButtons() is invoked) so callers call it directly without awaiting; update any related type annotations if present to reflect the synchronous signature.
560-595: Add a client-sideAbortControllertimeout as a safety net.
timeoutValueis forwarded to the server for it to apply when connecting to Redis, but there is no client-side abort. If the server itself hangs (e.g., TCP connection never established), thefetchblocks indefinitely and the button stays permanently disabled with the spinner active.Wrap the call with an
AbortControllerusing a slightly longer deadline thantimeoutValue(e.g.,timeoutValue + 2seconds) to ensure the UI always recovers.♻️ Proposed refactor
+ const controller = new AbortController(); + const abortTimer = setTimeout(() => controller.abort(), (timeoutValue + 2) * 1000); + try { const response = (await testRedisConnection( csrfInput.value, redisDsnInput.value.trim(), timeoutValue )) as unknown as Response; + clearTimeout(abortTimer); ... } catch (error) { + clearTimeout(abortTimer); pushErrorNotification(error instanceof Error ? error.message : 'Redis connection test failed.'); } finally {You will also need to pass
signal: controller.signaldown throughtestRedisConnectioninto the underlyingfetchJsoncall.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/admin/assets/src/configuration/configuration.ts` around lines 560 - 595, The client currently awaits testRedisConnection(...) without a client-side timeout, so a hanging server can leave the spinner/button disabled; create an AbortController, set a timeout using setTimeout to call controller.abort() after (timeoutValue + 2) seconds, pass controller.signal into testRedisConnection (and ensure testRedisConnection forwards it into fetchJson), and clear the timer on success/failure so that the finally block re-enables the button and restores innerHTML; keep using csrfInput.value and redisDsnInput.value.trim() and call pushNotification / pushErrorNotification as before, but ensure aborted fetches are handled in the catch to show an error and restore the UI.
533-537: Redundant ternary in error fallback — same issue exists inhandleTestRedisConnection(lines 582–586).
Response.successis typed as a non-optionalstring, sotypeof response.success === 'string'is alwaystrue. By the time this branch is reached,response.successwas already falsy (the early-return guard on line 528 would have fired otherwise), so the third operand always resolves to''and never contributes a real message. Remove the ternary.♻️ Proposed fix (applies identically to lines 582–586)
const errorMessage = response.error || response.message || - (typeof response.success === 'string' ? response.success : '') || 'Sending test email failed.';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/admin/assets/src/configuration/configuration.ts` around lines 533 - 537, The fallback building of errorMessage uses a redundant ternary "(typeof response.success === 'string' ? response.success : '')" which always evaluates the same and never contributes; in the error construction (variable errorMessage) replace that ternary with a simple response.success || '' (or just response.success) and apply the same change in the other handler (handleTestRedisConnection) where the identical ternary appears so the fallback chain becomes response.error || response.message || response.success || 'Sending test email failed.' (and similarly for the Redis test message).phpmyfaq/src/phpMyFAQ/Configuration/ConfigurationRepository.php (1)
92-105: SQL injection surface viasprintf+escape()— pre-existing pattern, but worth noting.The
sprintfapproach with'%s'relies entirely onescape()being correct for all edge cases (e.g., multi-byte attacks). This is a pre-existing pattern across the codebase (DatabaseConfigurationStoreuses the same approach), but parameterized queries would be a safer long-term direction.Not blocking — just flagging for awareness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Configuration/ConfigurationRepository.php` around lines 92 - 105, The updateFaqDataContentById method builds an SQL string via sprintf and escape() which can be fragile against edge-case injection; replace this with a parameterized/prepared statement using the DB API exposed by $this->coreConfiguration->getDb() (use prepare on the DB connection, bind the $newContent, $faqId and $lang values, and execute) instead of injecting values into the query string; keep Database::getTablePrefix() for the table name but do not inject user data into the SQL text—use prepared parameters for content and lang (and faqId) and return the boolean result of the prepared execution.phpmyfaq/src/phpMyFAQ/Configuration.php (1)
534-539: Return value ofupdateFaqDataContentById()is silently discarded.If any individual FAQ update fails, the method still returns
trueon line 543. Consider accumulating failures or short-circuiting on error.♻️ Proposed fix — track failures
+ $success = true; foreach ($contentItems as $contentItem) { if (str_contains((string) $contentItem->content, $oldUrl)) { $newContent = str_replace($oldUrl, $newUrl, $contentItem->content); - $this->configurationRepository->updateFaqDataContentById( + if (!$this->configurationRepository->updateFaqDataContentById( (int) $contentItem->id, (string) $contentItem->lang, $newContent, - ); + )) { + $success = false; + } } } - return true; + return $success;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Configuration.php` around lines 534 - 539, The call to updateFaqDataContentById(...) currently ignores its return value; change the code in the loop that processes $contentItem so you capture the boolean result from updateFaqDataContentById and either accumulate failures (e.g. $success = $success && $result) or short-circuit on first false (return false), and ensure the enclosing method (the function that contains this loop) returns a consolidated boolean instead of always true; reference updateFaqDataContentById, $contentItem, $newContent and the enclosing method name to locate where to check the result and update the final return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/administration.md`:
- Line 865: Update the two occurrences of the phrase "Elasticsearch relevant"
(and the similar "OpenSearch relevant") in the documentation so the compound
adjectives are hyphenated when used as pre-noun modifiers; replace
"Elasticsearch relevant usage data" with "Elasticsearch-relevant usage data" and
likewise change "OpenSearch relevant" to "OpenSearch-relevant" to follow
compound-adjective hyphenation rules.
- Around line 843-859: The link target redis-configuration-storage.md referenced
as "Redis-Backed Configuration Storage" is missing; either add a new
documentation page with that filename containing the Redis-backed configuration
setup and troubleshooting content and ensure it is included in the docs index,
or update the link in the Administration section to point to an existing,
correct document (or remove the link) so the reference in the "5.6.5
Redis-backed configuration storage" subsection resolves correctly.
In `@docs/installation.md`:
- Around line 808-815: Under the "DSN Examples" section replace the 4-space
indented nested list items that are currently being parsed as indented code with
proper fenced code blocks: for each example (Docker Compose service name, Local
host, Unix socket) move the descriptive line (e.g., "Docker Compose service
name:") as plain text and follow it with a triple-backtick fenced block
containing the DSN (e.g., tcp://redis:6379?database=1), ensuring each DSN is
wrapped in its own fenced block; update the "DSN Examples" subsection
accordingly to remove the 4-space indentation and eliminate the MD046 warning.
In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php`:
- Around line 32-36: The class currently calls createRedisClient() inside each
public method (e.g., updateConfigValue(), fetchAll(), warmFromRows(),
deleteConfigValue(), etc.), causing a fresh Redis connect/auth/select per call;
change to a single lazily-initialized client accessor (e.g., add a private
getRedisClient() that caches the client instance via a Closure stored in a
readonly property or by making the client property mutable if you drop readonly)
and have all public methods (updateConfigValue, fetchAll, warmFromRows,
deleteConfigValue, getConfigValue, setConfigValue, etc.) use getRedisClient()
instead of createRedisClient() so connections are reused; ensure
getRedisClient() calls createRedisClient() only once and returns the cached
client for subsequent calls.
- Around line 72-87: The renameKey method can leave both keys present if hDel
fails; update renameKey to check the return value of
$redis->hDel($this->getHashKey(), $currentKey) and handle failures: if hDel
returns false or 0, attempt to rollback by removing the newly set $newKey (or
restoring the original $currentKey) and then return false (or throw/propagate an
error as your error strategy mandates). Use the existing createRedisClient(),
getHashKey(), renameKey, hSet and hGet symbols to locate the logic to add this
failure check and rollback.
In
`@phpmyfaq/src/phpMyFAQ/Controller/Administration/SystemInformationController.php`:
- Around line 81-109: Replace the manual defaulting block for
redisDsn/redisPrefix/redisTimeout with the ConfigurationStorageSettingsResolver:
instantiate ConfigurationStorageSettingsResolver using $this->configuration,
call its resolve() (or equivalent) to obtain a ConfigurationStorageSettings
instance, then pass that settings object into RedisConfigurationStore and call
getInstalledRedisVersion(); also add imports for
ConfigurationStorageSettingsResolver (and DatabaseConfigurationStore if used
elsewhere) and remove the duplicated default logic and its try/catch placement
so the resolver provides canonical defaults.
---
Duplicate comments:
In `@phpmyfaq/admin/assets/src/configuration/configuration.ts`:
- Around line 598-618: The listener-accumulation bug has been addressed by using
the module-scoped variable redisInputListener in setupRedisTestButtonState to
remove the previous listener before adding a new one and by calling
redisInputListener() to seed the initial state; leave the implementation of
setupRedisTestButtonState and the redisInputListener handling as-is (no further
changes required).
In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php`:
- Around line 172-176: The RedisConfigurationStore previously did not check
Redis auth/select return values; update the RedisConfigurationStore class so
that in the block using $redis->auth($parsedUrl['pass']) and
$redis->select($dbIndex) you validate the return values and throw a
RuntimeException on failure (e.g., "Redis authentication failed for
configuration storage." and a similar message for select) to fail fast and
surface errors from authentication/selection.
In `@tests/phpMyFAQ/Configuration/ConfigurationStorageSettingsResolverTest.php`:
- Around line 1-4: The file now includes the required declare(strict_types=1)
directive at the top (the declare statement), so no further changes are needed;
leave the declare(strict_types=1) line as-is and proceed to approve the change.
In `@tests/phpMyFAQ/Configuration/HybridConfigurationStoreTest.php`:
- Around line 1-4: The previous issue about adding declare(strict_types=1) has
been resolved in tests/phpMyFAQ/Configuration/HybridConfigurationStoreTest.php;
no further changes are required—keep the declared strict types and proceed with
approval of the test file (HybridConfigurationStoreTest).
---
Nitpick comments:
In `@phpmyfaq/admin/assets/src/configuration/configuration.ts`:
- Around line 497-517: The function registerConfigurationActionButtons is
declared async but contains no awaits; remove the async modifier from its
declaration and change its return type to void (or inferred), and then remove
the unnecessary await operators at its call sites (where
registerConfigurationActionButtons() is invoked) so callers call it directly
without awaiting; update any related type annotations if present to reflect the
synchronous signature.
- Around line 560-595: The client currently awaits testRedisConnection(...)
without a client-side timeout, so a hanging server can leave the spinner/button
disabled; create an AbortController, set a timeout using setTimeout to call
controller.abort() after (timeoutValue + 2) seconds, pass controller.signal into
testRedisConnection (and ensure testRedisConnection forwards it into fetchJson),
and clear the timer on success/failure so that the finally block re-enables the
button and restores innerHTML; keep using csrfInput.value and
redisDsnInput.value.trim() and call pushNotification / pushErrorNotification as
before, but ensure aborted fetches are handled in the catch to show an error and
restore the UI.
- Around line 533-537: The fallback building of errorMessage uses a redundant
ternary "(typeof response.success === 'string' ? response.success : '')" which
always evaluates the same and never contributes; in the error construction
(variable errorMessage) replace that ternary with a simple response.success ||
'' (or just response.success) and apply the same change in the other handler
(handleTestRedisConnection) where the identical ternary appears so the fallback
chain becomes response.error || response.message || response.success || 'Sending
test email failed.' (and similarly for the Redis test message).
In `@phpmyfaq/src/phpMyFAQ/Configuration.php`:
- Around line 534-539: The call to updateFaqDataContentById(...) currently
ignores its return value; change the code in the loop that processes
$contentItem so you capture the boolean result from updateFaqDataContentById and
either accumulate failures (e.g. $success = $success && $result) or
short-circuit on first false (return false), and ensure the enclosing method
(the function that contains this loop) returns a consolidated boolean instead of
always true; reference updateFaqDataContentById, $contentItem, $newContent and
the enclosing method name to locate where to check the result and update the
final return behavior.
In `@phpmyfaq/src/phpMyFAQ/Configuration/ConfigurationRepository.php`:
- Around line 92-105: The updateFaqDataContentById method builds an SQL string
via sprintf and escape() which can be fragile against edge-case injection;
replace this with a parameterized/prepared statement using the DB API exposed by
$this->coreConfiguration->getDb() (use prepare on the DB connection, bind the
$newContent, $faqId and $lang values, and execute) instead of injecting values
into the query string; keep Database::getTablePrefix() for the table name but do
not inject user data into the SQL text—use prepared parameters for content and
lang (and faqId) and return the boolean result of the prepared execution.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docker-compose.ymldocs/administration.mddocs/installation.mdphpmyfaq/admin/assets/src/configuration/configuration.tsphpmyfaq/assets/templates/admin/configuration/macros.twigphpmyfaq/assets/templates/admin/configuration/main.twigphpmyfaq/src/phpMyFAQ/Configuration.phpphpmyfaq/src/phpMyFAQ/Configuration/ConfigurationRepository.phpphpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/SystemInformationController.phpphpmyfaq/translations/language_en.phptests/phpMyFAQ/Configuration/ConfigurationStorageSettingsResolverTest.phptests/phpMyFAQ/Configuration/HybridConfigurationStoreTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- phpmyfaq/translations/language_en.php
- docker-compose.yml
- phpmyfaq/assets/templates/admin/configuration/macros.twig
- phpmyfaq/assets/templates/admin/configuration/main.twig
phpmyfaq/src/phpMyFAQ/Controller/Administration/SystemInformationController.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
docs/installation.md (1)
788-788: Minor heading casing inconsistency between the two docs.
administration.mduses sentence case ("Redis-backed configuration storage") while this file uses title case ("Redis-Backed Configuration Storage"). Aligning to one style across the docs would improve consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/installation.md` at line 788, Change the heading "Redis-Backed Configuration Storage" in this file to sentence case "Redis-backed configuration storage" to match administration.md; update any internal references/TOC entries or links that rely on the exact heading text/anchor so they still resolve correctly (search for the heading string "Redis-Backed Configuration Storage" and replace it with "Redis-backed configuration storage", and verify any anchors or cross-doc links).phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php (2)
193-197: Path-based database index (redis://host/1) is silently ignored — only query-string format is supported.The conventional
redis://host:6379/1path format for selecting database 1 is ignored; the code only reads?database=or?db=from the query string. Users following the standard convention will silently land on database 0. Consider supporting or at minimum documenting the supported DSN format.♻️ Proposed fix — fall back to path for database index
$database = 0; if (isset($parsedUrl['query'])) { parse_str($parsedUrl['query'], $queryParams); $database = (int) ($queryParams['database'] ?? $queryParams['db'] ?? 0); + } elseif (isset($parsedUrl['path']) && $parsedUrl['path'] !== '' && $parsedUrl['path'] !== '/') { + $database = (int) ltrim($parsedUrl['path'], '/'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php` around lines 193 - 197, The code only reads database index from query params (via parse_str into $queryParams) and ignores a path-style index like redis://host/1; update the logic in RedisConfigurationStore (where $parsedUrl is inspected and $database is set) to, after checking query params, fall back to parsing $parsedUrl['path'] (trim leading '/' and cast to int) when no database/query param is present; ensure you handle empty/invalid path values by leaving $database as 0 and preserve existing behavior when query params explicitly provide db/database.
62-66:insert()cannot distinguish a Redis error from "field already exists".
hSetNxreturnsfalsefor both "field already exists" and a Redis error. The(bool)cast merges these two failure modes, so callers have no way to tell whether the insert was skipped or failed. This may be acceptable given the current interface contract, but callers relying on the return value for error detection may be misled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php` around lines 62 - 66, insert() currently casts hSetNx() to bool so it cannot distinguish "field already exists" from a Redis error; update insert(string $name, string $value) to disambiguate by either (A) checking the field first with the Redis client (use hExists via getRedisClient()->hExists($this->getHashKey(), $name)) and return false if it exists, then call hSetNx and treat a false there as an error, or (B) wrap the hSetNx call in a try/catch catching RedisException from getRedisClient()->hSetNx($this->getHashKey(), $name, $value) and rethrow or return a different signal; reference the insert method, getRedisClient(), hSetNx and getHashKey() so you can locate and implement the chosen fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/installation.md`:
- Around line 812-826: Three fenced code blocks containing the DSNs
("tcp://redis:6379?database=1", "tcp://127.0.0.1:6379?database=1",
"unix:///var/run/redis/redis.sock?database=1") lack a language specifier which
triggers MD040; add a language token such as text or plaintext to each fenced
block header to silence MD040. Also address the MD046 warnings about indented vs
fenced style by updating the markdownlint configuration (change style to
"fenced" or "consistent") so these deliberate fenced blocks are allowed rather
than converting them to indented code blocks.
In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php`:
- Around line 111-132: warmFromRows currently only calls hMSet so
deleted/renamed config keys stay in Redis; modify warmFromRows (use
getRedisClient() and getHashKey()) to run a pipeline that first deletes the
existing hash (DEL <hashKey>) and then inserts the new fields (HMSET/HMSET
equivalent) so the replacement is atomic and avoids stale entries returned by
fetchAll(); keep the early-return behavior for empty $rows and ensure you still
return the boolean result of the pipeline operation.
- Around line 68-72: The delete() method currently treats a false return from
Redis::hDel as true due to PHP's comparison rules; change delete() (using
getRedisClient(), getHashKey(), and hDel) to explicitly detect a false return
(Redis error) and return false, and otherwise return whether the numeric result
is >=1 (or >0) to indicate a field was actually deleted (e.g. assign $res =
$redis->hDel(...); return $res !== false && $res > 0;).
- Around line 148-207: The createRedisClient() and all public methods that call
Redis (updateConfigValue(), fetchAll(), insert(), delete(), renameKey(),
getInstalledRedisVersion(), warmFromRows()) currently let phpredis throw
RedisException which leaks out; modify createRedisClient() to wrap all Redis
calls (connect, auth, select, etc.) in try/catch(RedisException $e) and rethrow
a RuntimeException with a clear message and pass $e as the previous exception,
and similarly wrap the Redis operations inside each listed public method in
try/catch to convert any RedisException into a RuntimeException (preserving the
original exception as the previous exception) so callers only ever receive
RuntimeException.
---
Nitpick comments:
In `@docs/installation.md`:
- Line 788: Change the heading "Redis-Backed Configuration Storage" in this file
to sentence case "Redis-backed configuration storage" to match
administration.md; update any internal references/TOC entries or links that rely
on the exact heading text/anchor so they still resolve correctly (search for the
heading string "Redis-Backed Configuration Storage" and replace it with
"Redis-backed configuration storage", and verify any anchors or cross-doc
links).
In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php`:
- Around line 193-197: The code only reads database index from query params (via
parse_str into $queryParams) and ignores a path-style index like redis://host/1;
update the logic in RedisConfigurationStore (where $parsedUrl is inspected and
$database is set) to, after checking query params, fall back to parsing
$parsedUrl['path'] (trim leading '/' and cast to int) when no database/query
param is present; ensure you handle empty/invalid path values by leaving
$database as 0 and preserve existing behavior when query params explicitly
provide db/database.
- Around line 62-66: insert() currently casts hSetNx() to bool so it cannot
distinguish "field already exists" from a Redis error; update insert(string
$name, string $value) to disambiguate by either (A) checking the field first
with the Redis client (use hExists via
getRedisClient()->hExists($this->getHashKey(), $name)) and return false if it
exists, then call hSetNx and treat a false there as an error, or (B) wrap the
hSetNx call in a try/catch catching RedisException from
getRedisClient()->hSetNx($this->getHashKey(), $name, $value) and rethrow or
return a different signal; reference the insert method, getRedisClient(), hSetNx
and getHashKey() so you can locate and implement the chosen fix.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/administration.mddocs/installation.mdphpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/SystemInformationController.php
🚧 Files skipped from review as they are similar to previous changes (1)
- phpmyfaq/src/phpMyFAQ/Controller/Administration/SystemInformationController.php
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
phpmyfaq/translations/language_en.php (1)
1694-1698: DSN description still carries an implementation-specific parenthetical.The prior review flagged the
(Docker service name)annotation; this was updated to(use the service hostname). While the Docker reference is gone, the trailing parenthetical on the second example still ties explanatory context to only one of the two examples and can be confusing for users not using service-name-based DNS.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/translations/language_en.php` around lines 1694 - 1698, The DSN examples in $LANG_CONF['storage.redisDsn'] include a parenthetical "(use the service hostname)" only after the second example which implies implementation-specific context; remove that parenthetical (or replace it with a short neutral note that applies to both examples) so both example DSNs stand alone and no deployment-specific hint is embedded in the example list.docs/installation.md (1)
812-841: MD046 linter warnings still present on fenced code blocks — update markdownlint config.The language specifier (
text,bash) was added to satisfy MD040, but the project's markdownlint configuration still enforcesstyle: indented, so MD046 fires for all four fenced blocks (lines 812, 818, 824, 837). As noted in prior review, the cleanest fix is to change the project-level markdownlint config tostyle: fencedorstyle: consistentrather than converting these blocks back to indented syntax.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/installation.md` around lines 812 - 841, Update the repository's markdownlint configuration to set the code block style to "fenced" or "consistent" (change the "style" option from "indented" to "fenced" or "consistent") so MD046 no longer fires for the fenced code blocks shown (the examples using language specifiers like "text" and "bash"); keep the fenced blocks in docs/installation.md as-is and remove the ad-hoc conversions to indented blocks.phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php (1)
224-228:⚠️ Potential issue | 🟠 Major
parse_urldoes not decode the password —auth()receives a percent-encoded string.
parse_urlreturns the raw percent-encoded value for thepasscomponent; it does not URL-decode it. Any password that contains reserved characters (@,:,/,#,?,[,]) must be percent-encoded in the DSN forparse_urlto extract it at all, but then the literal%40-style string gets forwarded toauth()instead of the actual password, causing an authentication failure wrapped asRuntimeException.🐛 Proposed fix
if (isset($parsedUrl['pass']) && $parsedUrl['pass'] !== '') { - if ($redis->auth($parsedUrl['pass']) !== true) { + if ($redis->auth(rawurldecode($parsedUrl['pass'])) !== true) { throw new RuntimeException('Redis authentication failed for configuration storage.'); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php` around lines 224 - 228, The RedisConfigurationStore currently passes the percent-encoded password from parse_url directly into $redis->auth, causing auth failures; update the code in RedisConfigurationStore where $parsedUrl['pass'] is checked (the block that calls $redis->auth($parsedUrl['pass'])) to URL-decode the password first (e.g., use rawurldecode/urldecode on $parsedUrl['pass']) and then call $redis->auth with the decoded value, keeping the same error handling that throws RuntimeException on auth failure.
🧹 Nitpick comments (1)
phpmyfaq/translations/language_en.php (1)
1671-1671:storageControlCenteris misplaced inside the Push Notifications block.Line 1671 sits between
$PMF_LANG['pushControlCenter']and$PMF_LANG['msgGenerateVapidKeys'], which are both part of the "Web Push Notifications" section. The storage control-center label should either open its own versioned comment block or be co-located with the storage$LANG_CONFentries starting at line 1689.✏️ Suggested improvement
Remove line 1671 from the push block and add it with the storage config entries:
$PMF_LANG['pushControlCenter'] = 'Push'; -$PMF_LANG['storageControlCenter'] = 'Storage'; $PMF_LANG['msgGenerateVapidKeys'] = 'Generate VAPID Keys';+// added v4.2.0 - 2026-02-23 by Thorsten +$PMF_LANG['storageControlCenter'] = 'Storage'; + $LANG_CONF['storage.useRedisForConfiguration'] = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/translations/language_en.php` at line 1671, The translation key $PMF_LANG['storageControlCenter'] is currently placed inside the "Web Push Notifications" block between $PMF_LANG['pushControlCenter'] and $PMF_LANG['msgGenerateVapidKeys']; move this entry out of the push notifications section and into the storage configuration area alongside the storage $LANG_CONF entries (the block starting at the storage config entries around $LANG_CONF) so that $PMF_LANG['storageControlCenter'] is co-located with other storage-related labels and not mixed with $PMF_LANG['pushControlCenter'] or $PMF_LANG['msgGenerateVapidKeys'].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/installation.md`:
- Around line 845-851: Update the "Recommended Defaults" DSN documentation to
note that the example DSN value `tcp://redis:6379?database=1` is Docker-specific
(the hostname `redis` resolves only inside a Docker network); instruct users
running phpMyFAQ outside Docker to replace `redis` with their Redis host (e.g.,
`localhost`, an IP, or a configured hostname) or provide guidance to adjust the
DSN accordingly, and keep the Prefix `pmf:config:` and Timeout `1.0` unchanged
for clarity.
In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php`:
- Around line 195-247: The Redis client created in RedisConfigurationStore does
not set Redis::OPT_READ_TIMEOUT, so a stalled Redis server can block request
threads; after the successful connect/auth/select sequence (reference the $redis
variable in RedisConfigurationStore::... where connect/auth/select are called)
set the socket read timeout using $redis->setOption(Redis::OPT_READ_TIMEOUT,
<timeout>) using a sensible value (preferably a configured readTimeout in
$this->settings or fallback to (float)ini_get('default_socket_timeout')),
ensuring the option is applied before returning $redis and preserving existing
exception behavior.
---
Duplicate comments:
In `@docs/installation.md`:
- Around line 812-841: Update the repository's markdownlint configuration to set
the code block style to "fenced" or "consistent" (change the "style" option from
"indented" to "fenced" or "consistent") so MD046 no longer fires for the fenced
code blocks shown (the examples using language specifiers like "text" and
"bash"); keep the fenced blocks in docs/installation.md as-is and remove the
ad-hoc conversions to indented blocks.
In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php`:
- Around line 224-228: The RedisConfigurationStore currently passes the
percent-encoded password from parse_url directly into $redis->auth, causing auth
failures; update the code in RedisConfigurationStore where $parsedUrl['pass'] is
checked (the block that calls $redis->auth($parsedUrl['pass'])) to URL-decode
the password first (e.g., use rawurldecode/urldecode on $parsedUrl['pass']) and
then call $redis->auth with the decoded value, keeping the same error handling
that throws RuntimeException on auth failure.
In `@phpmyfaq/translations/language_en.php`:
- Around line 1694-1698: The DSN examples in $LANG_CONF['storage.redisDsn']
include a parenthetical "(use the service hostname)" only after the second
example which implies implementation-specific context; remove that parenthetical
(or replace it with a short neutral note that applies to both examples) so both
example DSNs stand alone and no deployment-specific hint is embedded in the
example list.
---
Nitpick comments:
In `@phpmyfaq/translations/language_en.php`:
- Line 1671: The translation key $PMF_LANG['storageControlCenter'] is currently
placed inside the "Web Push Notifications" block between
$PMF_LANG['pushControlCenter'] and $PMF_LANG['msgGenerateVapidKeys']; move this
entry out of the push notifications section and into the storage configuration
area alongside the storage $LANG_CONF entries (the block starting at the storage
config entries around $LANG_CONF) so that $PMF_LANG['storageControlCenter'] is
co-located with other storage-related labels and not mixed with
$PMF_LANG['pushControlCenter'] or $PMF_LANG['msgGenerateVapidKeys'].
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/installation.mdphpmyfaq/assets/templates/admin/configuration/macros.twigphpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.phpphpmyfaq/translations/language_de.phpphpmyfaq/translations/language_en.php
🚧 Files skipped from review as they are similar to previous changes (1)
- phpmyfaq/assets/templates/admin/configuration/macros.twig
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/installation.md (1)
812-841: MD046 linter warnings still active on fenced blocks.All four fenced blocks (lines 812, 818, 824, 837) still trigger
MD046("Expected: indented; Actual: fenced") because the project markdownlint config is set tostyle: indented. Language specifiers (text,bash) are now correctly applied, soMD040is resolved. The remaining fix is updating the markdownlint configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/installation.md` around lines 812 - 841, The MD046 failures come from the repo markdownlint config enforcing indented code blocks while the docs use fenced blocks; update the markdownlint config (e.g., .markdownlint.json / .markdownlint.yml) to accept fenced blocks by setting MD046 to use "fenced" (or disable MD046) so the fenced blocks (the tcp://..., 127.0.0.1, unix:///..., and the bash block) no longer trigger errors; apply the change to the shared config so all occurrences, including the fenced blocks in installation.md, comply with the lint rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@phpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php`:
- Around line 224-228: The Redis auth block in RedisConfigurationStore.php
currently ignores $parsedUrl['user'] and does not urldecode credentials; update
the logic so that both $parsedUrl['user'] and $parsedUrl['pass'] are passed
through urldecode(), then call $redis->auth([$user, $pass]) when a username is
present and fall back to $redis->auth($pass) when only a password exists, and
still throw a RuntimeException if auth does not return true; reference the
$parsedUrl['user'],$parsedUrl['pass'] variables and the $redis->auth(...) call
to locate and replace the existing check.
---
Duplicate comments:
In `@docs/installation.md`:
- Around line 812-841: The MD046 failures come from the repo markdownlint config
enforcing indented code blocks while the docs use fenced blocks; update the
markdownlint config (e.g., .markdownlint.json / .markdownlint.yml) to accept
fenced blocks by setting MD046 to use "fenced" (or disable MD046) so the fenced
blocks (the tcp://..., 127.0.0.1, unix:///..., and the bash block) no longer
trigger errors; apply the change to the shared config so all occurrences,
including the fenced blocks in installation.md, comply with the lint rules.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/installation.mdphpmyfaq/src/phpMyFAQ/Configuration/Storage/RedisConfigurationStore.php
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
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
New Features
Documentation
Chores
Tests
Bug Fixes