Improvement of the experimental plugin system #3852
Conversation
📝 WalkthroughWalkthroughIntroduces a plugin subsystem: DB table/schema for plugins, PluginManager enhancements (activation, config persistence, prepared-statement DB APIs), admin API endpoints and UI for toggling/saving plugin configs, admin Twig template/modal, frontend JS for plugin UI, plugin example classes, and update routines to create plugin table. Changes
Sequence Diagram(s)sequenceDiagram
actor AdminBrowser as Admin (Browser)
participant JS as Admin JS
participant API as PluginController (Server)
participant PM as PluginManager
participant DB as Database
AdminBrowser->>JS: click toggle / open config modal / save config
JS->>API: POST /api/plugin/toggle or /api/plugin/config (JSON, X-CSRF-Token)
API->>PM: loadPlugins() / validate plugin / activate/deactivate or savePluginConfig(...)
PM->>DB: getPluginsFromDatabase() / update plugin row or persist config
DB-->>PM: return plugin rows / ack update
PM-->>API: operation result (success / error)
API-->>JS: JSON response
JS-->>AdminBrowser: update UI / show notification / reload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (1)
1-541: Fix formatting issues flagged by CI pipeline.The pipeline reports that
mago format --dry-runfound formatting changes needed in this file. Please run the formatter to fix these issues:mago format phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php
🤖 Fix all issues with AI agents
In @phpmyfaq/admin/assets/src/api/plugins.ts:
- Around line 45-58: The savePluginConfig function lacks CSRF protection and
HTTP error handling; update savePluginConfig to read the CSRF token (e.g., from
the existing helper or meta tag used by togglePluginStatus) and include it in
the request headers (e.g., 'X-CSRF-Token'), and after fetch verify response.ok
and throw or return a rejected error (including status/text) before parsing
JSON; reference the savePluginConfig function and mirror the header and
response.ok checks implemented in togglePluginStatus so the request is protected
and failures are surfaced properly.
- Around line 24-37: The togglePluginStatus function currently parses JSON
without checking HTTP status; update togglePluginStatus to inspect response.ok
after fetch and handle non-OK responses (e.g., throw an error or return a
rejected Promise with a descriptive message and include status/statusText and
parsed error body if present) before calling response.json(); ensure the
function signature still returns the same Response type on success and that
error paths propagate useful information for callers.
- Around line 24-37: The togglePluginStatus function is missing CSRF protection;
update the function signature (togglePluginStatus) to accept a csrfToken
(string) parameter, add the CSRF token to the fetch request (include it in
headers using the same header name your codebase uses for admin endpoints, e.g.,
'X-CSRF-Token' or the project's configured header), and ensure the body remains
unchanged; then update all callers to pass the current CSRF token when invoking
togglePluginStatus so admin API requests include the token for server
validation.
In @phpmyfaq/admin/assets/src/configuration/plugins.ts:
- Line 36: Calls to togglePluginStatus and savePluginConfig do not include a
CSRF token; update the code to fetch the CSRF token (e.g. from a meta tag like
document.querySelector('meta[name="csrf-token"]')?.getAttribute('content') or an
existing getCsrfToken() helper) and pass that token as an additional argument to
togglePluginStatus(pluginName, active, csrfToken) and to
savePluginConfig(pluginName, config, csrfToken) so the API layer receives CSRF
protection; apply the same change for the savePluginConfig invocation mentioned
in the comment.
- Around line 168-181: The form serialization loop in configuration/plugins.ts
collects inputs from container and treats non-checkboxs as text, so numeric
fields like wordsPerMinute lack HTML5 number inputs and select/textarea/radio
options are never generated; update the generation logic (where inputs are
created) to infer field types from the plugin schema or a type map and create
appropriate elements (e.g., HTMLInputElement with type="number" for integers,
type="email"/"date" as needed, select/textarea/radio for enumerations), and
adjust the serialization loop (the inputs NodeList and usage of configData) to
read value/checked consistently for these element types so numbers are preserved
(parseInt/parseFloat where appropriate) and select/textarea values are
supported.
In @phpmyfaq/assets/templates/admin/configuration/plugins.twig:
- Around line 36-37: The HTML id uses plugin.name directly (id="active_{{
plugin.name }}"), which can produce invalid IDs if plugin names contain spaces
or special chars; update the template to sanitize plugin.name when building the
id (e.g., replace dots/spaces/unsafe chars with underscores or use a regex
replace) so the label's for attribute and the checkbox id remain valid and
accessible; locate the id and corresponding for attribute in plugins.twig
(references: plugin.name, class "plugin-toggle", label for="active_...") and
apply a Twig replace/regex filter to produce a safe identifier consistently.
- Around line 50-53: The data-plugin-config attribute uses plugin.config with
json_encode but lacks HTML attribute escaping; update the template to pipe the
json_encode output through the Twig escaper for attributes (use e('html_attr'))
so the JSON string emitted by plugin.config is safe inside the HTML attribute;
keep the existing default handling for plugin.getAdvDescription() and
plugin.getImplementation() as-is.
In @phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php:
- Around line 58-60: The JSON decoding of $request->getContent() in
PluginController (where $data = json_decode(...), $name = $data['name'] ?? null,
$config = (array) ($data['config'] ?? [])) lacks error handling: detect
json_last_error() / json_last_error_msg() after json_decode and treat malformed
JSON as a bad request; return an appropriate error response (HTTP 400) or throw
the same kind of exception used in toggleStatus for invalid JSON, so invalid
payloads don't produce null-driven notices—ensure you validate $data is an array
before accessing ['name'] or ['config'].
- Around line 35-37: The code uses json_decode($request->getContent(), true) and
then reads $data['name'] and $data['active'] without checking for JSON parse
errors; detect malformed JSON by verifying that json_decode returned an array
and/or checking json_last_error() after the decode in the PluginController
method, and if invalid respond with a 400 Bad Request (e.g., throw
BadRequestHttpException or return a JsonResponse with status 400) before
accessing $data; keep references to $data, $name, $active and the json_decode
call so the check is added immediately after that call.
- Line 66: The saveConfig() handler must validate CSRF and handle save errors:
extract the CSRF token from the request payload (e.g. $data['csrf']) and call
Token::getInstance($this->session)->verifyToken($csrf, 'admin-plugin-config'
/*or appropriate page id*/) before proceeding; then wrap the call to
$pluginManager->savePluginConfig($name, $config) in a try-catch that catches
database/exception types and returns a JSON error response (with an error
message and non-200 status) when an exception occurs, otherwise return success.
In @phpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlsrv.php:
- Around line 176-180: The config column for the faqdata_plugins table uses
NVARCHAR(MAX) in PdoSqlsrv.php which creates a capacity/behavior mismatch versus
other drivers; update the declaration in the 'faqdata_plugins' entry in
PdoSqlsrv.php so the config column uses a TEXT-like type consistent with other
drivers (use TEXT where supported, or VARCHAR(MAX) for SQL Server if you must
stay with single-byte semantics) and adjust any schema migration/creation code
accordingly so all drivers present a functionally equivalent unlimited-text
column for config.
In @phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php:
- Around line 407-410: Remove the unused private method getPluginWebPath(string
$pluginName) from the PluginManager class; since stylesheets and scripts
registration already construct plugin paths inline (see the registration code
around the stylesheet and script registration blocks), simply delete the
getPluginWebPath method and ensure no other methods call it (if any callers
exist, replace them with the inline 'content/plugins/' . $pluginName
construction or refactor callers to use the same inline pattern).
In @phpmyfaq/src/phpMyFAQ/Setup/Update.php:
- Around line 1128-1179: The file containing the method applyUpdates420Alpha
needs to be formatted with the mago formatter; run the mago format tool (e.g.,
mago format at the repo root or on the file containing applyUpdates420Alpha),
stage and commit the resulting changes, and push the commit so the pipeline no
longer flags the formatting error.
In @phpmyfaq/translations/language_de.php:
- Around line 1543-1557: This file contains duplicate plugin translation keys:
remove the second block that redefines keys like msgPlugins, msgPluginListing,
msgPluginName, msgPluginVersion, msgPluginAuthor, msgPluginDescription, and
msgPluginReason (the duplicates at the later block) and merge any missing keys
from that block (msgActions, msgConfig, msgIncompatiblePlugins,
msgIncompatiblePluginsInfo, msgNoPluginConfig, msgPluginImplementation) into the
original plugin translation group (the earlier definitions around
msgPlugins/msgPluginListing) so all plugin-related keys are defined once with
the correct, consistent values.
In @phpmyfaq/translations/language_en.php:
- Around line 1543-1556: Remove the duplicated translation keys in the later
block: delete the second definitions of msgPlugins, msgPluginListing,
msgPluginName, msgPluginVersion, msgPluginAuthor, msgPluginDescription,
msgIncompatiblePlugins, msgIncompatiblePluginsInfo, and msgPluginReason so they
do not override the originals defined earlier; then merge the unique keys from
this block (msgActions, msgConfig, msgNoPluginConfig, msgPluginImplementation)
into the earlier translation section (the block added in v4.1.0-alpha) so all
plugin-related keys exist exactly once.
🧹 Nitpick comments (9)
phpmyfaq/src/phpMyFAQ/Instance/Database/Mysqli.php (1)
183-188: Consider using TINYINT for the active column.Line 185 uses
INT(1)for the active flag. While this is consistent with other boolean columns in this file,TINYINTwould be more appropriate for boolean flags and avoids the deprecated display width syntax in MySQL 8.0.17+.♻️ Suggested improvement
'faqdata_plugins' => 'CREATE TABLE %sfaqdata_plugins ( name VARCHAR(255) NOT NULL, - active INT(1) NOT NULL DEFAULT 0, + active TINYINT NOT NULL DEFAULT 0, config LONGTEXT DEFAULT NULL, PRIMARY KEY (name)) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ENGINE = InnoDB',phpmyfaq/src/phpMyFAQ/Instance/Database/PdoMysql.php (1)
182-187: LGTM! Consider using TINYINT for the boolean column.The table structure is well-defined for plugin management. However, for better MySQL convention, consider using
TINYINT(1)instead ofINT(1)for theactivecolumn, as it's more memory-efficient and semantically clearer for boolean values.♻️ Optional refinement for the active column type
'faqdata_plugins' => 'CREATE TABLE %sfaqdata_plugins ( name VARCHAR(255) NOT NULL, - active INT(1) NOT NULL DEFAULT 0, + active TINYINT(1) NOT NULL DEFAULT 0, config LONGTEXT DEFAULT NULL, PRIMARY KEY (name)) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ENGINE = InnoDB',phpmyfaq/content/plugins/HelloWorld/HelloWorldPluginConfiguration.php (1)
24-31: Consider readonly properties for configuration immutability.The configuration class uses public mutable properties. For better encapsulation and to prevent accidental modification after initialization, consider making the properties
readonly. However, if the plugin system requires mutable configuration at runtime, the current approach is acceptable.♻️ Optional refinement for immutability
class HelloWorldPluginConfiguration implements PluginConfigurationInterface { public function __construct( - public string $greeting = 'Hello, World!', - public string $username = 'John Doe', + public readonly string $greeting = 'Hello, World!', + public readonly string $username = 'John Doe', ) { } }phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php (1)
39-41: Consider stricter plugin name validation.While checking plugin existence is good, there's no validation of the
$nameformat itself. An empty string would pass the null check but might cause issues.♻️ Suggested validation enhancement
if ($name === null || !array_key_exists($name, $pluginManager->getPlugins())) { return new JsonResponse(['success' => false, 'message' => 'Plugin not found'], 404); } + + if (empty(trim($name))) { + return new JsonResponse(['success' => false, 'message' => 'Invalid plugin name'], 400); + }Or combine into a single validation:
- if ($name === null || !array_key_exists($name, $pluginManager->getPlugins())) { + if (empty($name) || !array_key_exists($name, $pluginManager->getPlugins())) { return new JsonResponse(['success' => false, 'message' => 'Plugin not found'], 404); }phpmyfaq/admin/assets/src/configuration/plugins.ts (1)
189-189: Consider UX improvement: avoid full page reload.While
window.location.reload()works, it disrupts the user experience. Consider updating only the affected UI elements or closing the modal with a success notification instead.♻️ Optional UX improvement
if (result.success) { - window.location.reload(); + // Close modal + const modal = bootstrap.Modal.getInstance(configModalEl); + modal?.hide(); + pushNotification('Configuration saved successfully.'); + // Optionally refresh just the plugin table row } else { pushErrorNotification('Failed to save configuration: ' + (result.message || 'Unknown error')); }phpmyfaq/content/plugins/HelloWorld/HelloWorldPlugin.php (1)
24-25: Redundantrequire_onceat file top level.The
require_onceat line 24 loadsHelloWorldPluginConfiguration.phpunconditionally. However,getConfig()at lines 74-76 also has aclass_existsguard with anotherrequire_once. Since line 24 always executes first, the guard will never trigger.Consider removing one of them for clarity:
♻️ Option 1: Remove top-level require (preferred for lazy loading)
-require_once __DIR__ . '/HelloWorldPluginConfiguration.php'; - use phpMyFAQ\Plugin\PluginEvent;This keeps the lazy loading pattern in
getConfig().♻️ Option 2: Remove the guard in getConfig()
public function getConfig(): ?PluginConfigurationInterface { if ($this->config === null) { - if (!class_exists(HelloWorldPluginConfiguration::class)) { - require_once __DIR__ . '/HelloWorldPluginConfiguration.php'; - } $this->config = new HelloWorldPluginConfiguration(); } return $this->config; }This is simpler if eager loading is acceptable.
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (3)
201-203: Remove development comments before merging.These appear to be development notes rather than useful code documentation:
// Remove plugin from the plugins array since it's incompatible (actually, maybe keep it but mark inactive?) // Original code removed it. I'll stick to original behavior for incompatible ones.♻️ Proposed fix
$this->incompatiblePlugins[$plugin->getName()] = [ 'plugin' => $plugin, 'reason' => sprintf('Missing dependencies: %s', implode(', ', $missingDeps)), ]; - // Remove plugin from the plugins array since it's incompatible (actually, maybe keep it but mark inactive?) - // Original code removed it. I'll stick to original behavior for incompatible ones. unset($this->plugins[$plugin->getName()]); }
304-309: Consider catching a more specific exception type.Catching the generic
\Exceptioncould mask legitimate errors unrelated to table existence. Consider using a more specific exception type or explicitly checking if the table exists first.♻️ Suggested improvement
// Ensure table exists to avoid crashes during update/install if not yet run try { $result = $db->query("SELECT name, active, config FROM $table"); - } catch (\Exception $e) { + } catch (\PDOException|\mysqli_sql_exception $e) { // Table might not exist yet return []; }Alternatively, you could check table existence explicitly before querying if the database driver supports it.
145-148: Clean up development notes.This comment reads like a development note rather than documentation:
// I will default to inactive (false) for new plugins found on disk but not in DB. $isActive = false;♻️ Proposed fix
} } else { - - // I will default to inactive (false) for new plugins found on disk but not in DB. - $isActive = false; + // New plugins default to inactive until explicitly activated + $isActive = false; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
phpmyfaq/admin/assets/src/api/index.tsphpmyfaq/admin/assets/src/api/plugins.tsphpmyfaq/admin/assets/src/configuration/index.tsphpmyfaq/admin/assets/src/configuration/plugins.tsphpmyfaq/admin/assets/src/index.tsphpmyfaq/assets/templates/admin/configuration/plugins.twigphpmyfaq/content/plugins/HelloWorld/HelloWorldConfiguration.phpphpmyfaq/content/plugins/HelloWorld/HelloWorldPlugin.phpphpmyfaq/content/plugins/HelloWorld/HelloWorldPluginConfiguration.phpphpmyfaq/src/admin-routes.phpphpmyfaq/src/phpMyFAQ/Configuration.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/PluginController.phpphpmyfaq/src/phpMyFAQ/Database/Mysqli.phpphpmyfaq/src/phpMyFAQ/Database/PdoMysql.phpphpmyfaq/src/phpMyFAQ/Database/PdoPgsql.phpphpmyfaq/src/phpMyFAQ/Database/PdoSqlite.phpphpmyfaq/src/phpMyFAQ/Database/PdoSqlsrv.phpphpmyfaq/src/phpMyFAQ/Database/Pgsql.phpphpmyfaq/src/phpMyFAQ/Database/Sqlite3.phpphpmyfaq/src/phpMyFAQ/Database/Sqlsrv.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Mysqli.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoMysql.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoPgsql.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlite.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlsrv.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Pgsql.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Sqlite3.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Sqlsrv.phpphpmyfaq/src/phpMyFAQ/Plugin/PluginManager.phpphpmyfaq/src/phpMyFAQ/Setup/Update.phpphpmyfaq/src/phpMyFAQ/Twig/Extensions/PluginTwigExtension.phpphpmyfaq/src/services.phpphpmyfaq/translations/language_de.phpphpmyfaq/translations/language_en.php
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript coding standards for TypeScript code
Use semicolons at the end of each statement
Use single quotes for strings
Use arrow functions for callbacks
Files:
phpmyfaq/admin/assets/src/configuration/index.tsphpmyfaq/admin/assets/src/configuration/plugins.tsphpmyfaq/admin/assets/src/api/index.tsphpmyfaq/admin/assets/src/api/plugins.tsphpmyfaq/admin/assets/src/index.ts
**/phpmyfaq/src/**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
Use PER Coding Style 3.0 for PHP code
Files:
phpmyfaq/src/phpMyFAQ/Database/PdoPgsql.phpphpmyfaq/src/phpMyFAQ/Database/PdoSqlite.phpphpmyfaq/src/phpMyFAQ/Twig/Extensions/PluginTwigExtension.phpphpmyfaq/src/phpMyFAQ/Database/Pgsql.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlite.phpphpmyfaq/src/admin-routes.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Sqlite3.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Pgsql.phpphpmyfaq/src/phpMyFAQ/Database/Sqlite3.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlsrv.phpphpmyfaq/src/services.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoPgsql.phpphpmyfaq/src/phpMyFAQ/Database/PdoSqlsrv.phpphpmyfaq/src/phpMyFAQ/Database/Sqlsrv.phpphpmyfaq/src/phpMyFAQ/Plugin/PluginManager.phpphpmyfaq/src/phpMyFAQ/Database/Mysqli.phpphpmyfaq/src/phpMyFAQ/Setup/Update.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Mysqli.phpphpmyfaq/src/phpMyFAQ/Configuration.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/PluginController.phpphpmyfaq/src/phpMyFAQ/Database/PdoMysql.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoMysql.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Sqlsrv.php
**/translations/**
📄 CodeRabbit inference engine (AGENTS.md)
Use UTF-8 encoding for all translation files
Files:
phpmyfaq/translations/language_en.phpphpmyfaq/translations/language_de.php
🧬 Code graph analysis (11)
phpmyfaq/src/phpMyFAQ/Twig/Extensions/PluginTwigExtension.php (2)
phpmyfaq/src/phpMyFAQ/Configuration.php (4)
Configuration(47-530)getConfigurationInstance(96-99)getPluginManager(516-519)triggerEvent(521-524)phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (1)
triggerEvent(326-332)
phpmyfaq/admin/assets/src/configuration/plugins.ts (2)
phpmyfaq/admin/assets/src/api/plugins.ts (2)
togglePluginStatus(24-37)savePluginConfig(45-58)phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (1)
savePluginConfig(235-262)
phpmyfaq/src/admin-routes.php (1)
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php (1)
PluginController(27-70)
phpmyfaq/admin/assets/src/api/plugins.ts (3)
phpmyfaq/admin/assets/src/interfaces/response.ts (1)
Response(1-8)phpmyfaq/admin/assets/js/configuration.js (1)
response(49-56)phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (1)
savePluginConfig(235-262)
phpmyfaq/admin/assets/src/index.ts (1)
phpmyfaq/admin/assets/src/configuration/plugins.ts (1)
handlePlugins(22-199)
phpmyfaq/content/plugins/HelloWorld/HelloWorldPluginConfiguration.php (2)
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (1)
__construct(55-60)phpmyfaq/src/phpMyFAQ/Configuration.php (1)
__construct(73-94)
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (5)
phpmyfaq/src/phpMyFAQ/Configuration.php (2)
Configuration(47-530)getDb(134-137)phpmyfaq/src/phpMyFAQ/Plugin/PluginInterface.php (6)
getName(29-29)getConfig(54-54)registerEvents(59-59)getTranslationsPath(75-75)getStylesheets(67-67)getScripts(83-83)phpmyfaq/src/phpMyFAQ/Translation.php (2)
Translation(25-436)registerPluginTranslations(292-322)phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php (1)
escape(83-86)phpmyfaq/src/phpMyFAQ/Database/DatabaseDriver.php (1)
escape(55-55)
phpmyfaq/src/phpMyFAQ/Setup/Update.php (1)
phpmyfaq/src/phpMyFAQ/Database.php (1)
getTablePrefix(138-141)
phpmyfaq/src/phpMyFAQ/Configuration.php (1)
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (1)
PluginManager(31-541)
phpmyfaq/src/phpMyFAQ/Controller/Administration/PluginController.php (5)
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (2)
getPlugins(350-353)isPluginActive(227-230)phpmyfaq/content/plugins/HelloWorld/HelloWorldPlugin.php (1)
getName(34-37)tests/phpMyFAQ/Plugin/PluginManagerTest.php (1)
getName(249-252)phpmyfaq/src/phpMyFAQ/Plugin/PluginInterface.php (1)
getName(29-29)tests/phpMyFAQ/Plugin/MockPlugin.php (1)
getName(9-12)
phpmyfaq/content/plugins/HelloWorld/HelloWorldPlugin.php (3)
phpmyfaq/src/phpMyFAQ/Plugin/PluginInterface.php (4)
getDependencies(49-49)getConfig(54-54)getTranslationsPath(75-75)registerEvents(59-59)phpmyfaq/content/plugins/HelloWorld/HelloWorldPluginConfiguration.php (1)
HelloWorldPluginConfiguration(24-31)phpmyfaq/src/phpMyFAQ/Plugin/PluginEvent.php (3)
PluginEvent(26-49)getData(35-38)setOutput(40-43)
🪛 ast-grep (0.40.3)
phpmyfaq/admin/assets/src/configuration/plugins.ts
[warning] 90-90: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: container.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🪛 GitHub Actions: phpMyFAQ Build
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php
[error] 1-1: mago format --dry-run reported formatting changes needed in this file. Found 2 file(s) that need formatting.
phpmyfaq/src/phpMyFAQ/Setup/Update.php
[error] 1-1: mago format --dry-run reported formatting changes needed in this file. Found 2 file(s) that need formatting.
🪛 PHPMD (2.15.0)
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php
407-410: Avoid unused private methods such as 'getPluginWebPath'. (undefined)
(UnusedPrivateMethod)
🔇 Additional comments (30)
phpmyfaq/src/phpMyFAQ/Database/PdoSqlsrv.php (1)
222-222: LGTM! Clean addition to support plugin system.The addition of
faqdata_pluginsto the table names array is correct and follows the established naming convention. This properly extends the MS SQL Server driver to recognize the new plugin data table.phpmyfaq/src/phpMyFAQ/Database/Pgsql.php (1)
317-317: LGTM: Plugin table correctly registered.The addition of
faqdata_pluginsto the table registry is correct and consistent with the plugin management feature being introduced across all database drivers.phpmyfaq/src/phpMyFAQ/Database/PdoMysql.php (1)
222-222: LGTM: Plugin table correctly registered.The addition of
faqdata_pluginsto the table registry is correct and aligns with the broader plugin management implementation across all database backends.phpmyfaq/src/phpMyFAQ/Database/Mysqli.php (1)
255-255: LGTM: Plugin table correctly registered.The addition of
faqdata_pluginsto the table registry is correct and maintains consistency with the plugin data persistence layer being added across all database drivers.phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php (1)
273-273: LGTM: Plugin table correctly registered.The addition of
faqdata_pluginsto the table registry is correct and consistent with the plugin management feature implementation across all supported database backends.phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php (1)
312-312: LGTM: Plugin table correctly registered.The addition of
faqdata_pluginsto the table registry is correct and completes the consistent multi-driver update for the new plugin management feature.phpmyfaq/content/plugins/HelloWorld/HelloWorldConfiguration.php (1)
24-27: LGTM!The plugin configuration class follows a clean structure with proper interface implementation. The public property pattern is appropriate for configuration data.
phpmyfaq/src/phpMyFAQ/Instance/Database/Pgsql.php (1)
180-184: LGTM!The table definition follows PostgreSQL conventions and is consistent with other table definitions in this driver.
phpmyfaq/src/phpMyFAQ/Instance/Database/PdoPgsql.php (1)
179-183: LGTM!The table definition is consistent with the legacy Pgsql driver and follows proper PostgreSQL syntax.
phpmyfaq/src/phpMyFAQ/Instance/Database/Sqlsrv.php (1)
177-181: LGTM!The table definition correctly uses SQL Server data types with proper Unicode support (NVARCHAR) and is consistent with other table definitions in this driver.
phpmyfaq/src/phpMyFAQ/Database/PdoPgsql.php (1)
221-221: LGTM!The table name addition is consistent with the plugin infrastructure being introduced across all database drivers.
phpmyfaq/src/phpMyFAQ/Database/PdoSqlite.php (1)
246-246: LGTM!The table name addition maintains consistency with the plugin infrastructure across all database drivers.
phpmyfaq/admin/assets/src/index.ts (2)
39-39: LGTM!The import is correctly placed with other configuration-related imports and follows the existing code structure.
171-172: LGTM!The plugin initialization is properly integrated into the DOMContentLoaded flow. The placement after Elasticsearch/OpenSearch configuration ensures core services are set up before plugin management is initialized.
phpmyfaq/admin/assets/src/api/index.ts (1)
15-15: LGTM!The new re-export follows the established barrel pattern and TypeScript coding standards (semicolons, single quotes). This correctly exposes the plugin API module to consumers of the API index.
phpmyfaq/admin/assets/src/configuration/index.ts (1)
6-6: LGTM!The re-export correctly exposes the plugins configuration module, maintaining consistency with the existing barrel pattern and TypeScript coding standards.
phpmyfaq/src/phpMyFAQ/Controller/Administration/PluginController.php (1)
42-45: LGTM!The
activePluginscomputation correctly builds a map of plugin names to their active status by iterating through all plugins and queryingisPluginActive()for each. This data structure aligns with the template's needs (as referenced in the AI summary) and follows a clear, straightforward pattern.Also applies to: 51-51
phpmyfaq/src/phpMyFAQ/Twig/Extensions/PluginTwigExtension.php (1)
31-31: The null-to-empty-string coercion appears safe for existing plugins.Examined all three example plugins (EnhancedExamplePlugin, HelloWorldPlugin, ReadingTimePlugin) and found none differentiate between
nulland''when handling event data. All plugins simply retrieve data via$event->getData()and use it directly without null checks. A repository-wide search found no plugins useis_null()or explicit null comparisons on event data. Sincestrip_tags(null)andstrip_tags('')produce identical results, and other plugins use the data in string contexts where both behave equivalently, the change to coerce null to an empty string poses no risk to existing plugins in this codebase.phpmyfaq/src/phpMyFAQ/Configuration.php (1)
507-514: LGTM!The change correctly passes the Configuration instance to the PluginManager constructor, which aligns with the updated constructor signature shown in the relevant code snippets. This enables the PluginManager to access configuration and database services for plugin lifecycle management and persistence.
phpmyfaq/src/phpMyFAQ/Instance/Database/Sqlite3.php (1)
177-181: LGTM!The new
faqdata_pluginstable schema is well-designed for storing plugin metadata:
- Plugin name as primary key ensures uniqueness
- Default inactive state (0) is a safe default for new plugins
- TEXT column for config provides flexibility for JSON-based configuration
The structure is consistent with other table definitions in this file.
phpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlite.php (1)
176-180: LGTM!The
faqdata_pluginstable structure is identical to the Sqlite3.php implementation, ensuring consistency across SQLite database drivers. The schema design is appropriate for plugin persistence.phpmyfaq/assets/templates/admin/configuration/plugins.twig (1)
64-104: Well-structured plugin configuration modal.The modal implementation is comprehensive and well-organized:
- Provides clear sections for description, implementation code, and configuration
- Handles the no-config case gracefully with an informative message
- Uses proper Bootstrap 5 modal patterns with accessibility attributes
- Dynamic form field generation aligns with the frontend plugin management API
The structure integrates well with the plugin management features added in this PR.
phpmyfaq/src/services.php (1)
300-303: LGTM! Clean factory-based DI pattern.The change to use a factory method for PluginManager instantiation properly delegates creation to the Configuration service, following dependency injection best practices.
phpmyfaq/src/phpMyFAQ/Setup/Update.php (2)
1128-1179: LGTM! Database schema is correctly defined across all supported drivers.The plugin table creation properly handles type differences across MySQL/MariaDB, PostgreSQL, SQLite, and SQL Server:
- Uses appropriate text types (LONGTEXT, TEXT, NVARCHAR(MAX))
- Applies correct integer types and defaults
- Maintains consistent PRIMARY KEY definitions
187-189: No ordering issues identified in the update flow.The execution order is safe.
applyUpdates420Alpha()only adds queries to the array without executing them,optimizeTables()queries existing tables (read-only, safe before execution), andexecuteQueries()executes all accumulated queries. The newfaqdata_pluginstable won't be included in the OPTIMIZE TABLE list during this upgrade, but that's harmless—it will be optimized in future updates. The version check inapplyUpdates420Alpha()(version < '4.2.0-alpha') already handles recovery from incomplete previous updates.phpmyfaq/admin/assets/src/configuration/plugins.ts (2)
90-91: Safe innerHTML usage - static analysis false positive.The static analysis tool flagged this line, but clearing
innerHTMLwith an empty string is safe and standard practice. This is not an XSS vulnerability.
22-50: Good error recovery pattern.The toggle handler properly reverts the UI state on failure and provides user feedback. This prevents UI state from becoming out of sync with the server state.
phpmyfaq/content/plugins/HelloWorld/HelloWorldPlugin.php (1)
87-107: LGTM!The event registration and handlers are well-structured. The
getConfig()method guarantees a non-null return value for this plugin, so accessing->greetingand->usernameis safe.phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (2)
227-230:isPluginActivemay not reflect DB state after toggle.This method checks
$this->loadedPluginswhich is populated duringloadPlugins(). However, after callingactivatePlugin()ordeactivatePlugin(), the DB is updated but$this->loadedPluginsis not refreshed until the next request.Consider if this is the intended behavior, or if you need to also update the in-memory state.
235-262: LGTM - SQL escaping is properly applied.The
savePluginConfigmethod correctly uses$db->escape()for all user-controlled values before interpolating them into SQL queries. The table name is safely constructed from internal constants.
thorsten
left a comment
thorsten
left a comment
There was a problem hiding this comment.
Formatting is not correct in some files.
| * This Source Code Form is subject to the terms of the Mozilla Public License, | ||
| * v. 2.0. If a copy of the MPL was not distributed with this file, You can | ||
| * obtain one at https://mozilla.org/MPL/2.0/. | ||
| * |
| }, | ||
| body: JSON.stringify({ | ||
| name, | ||
| active, |
|
|
||
| if (!result.success) { | ||
| target.checked = !active; // Revert | ||
| pushErrorNotification('Failed to update plugin status: ' + (result.message || 'Unknown error')); |
There was a problem hiding this comment.
The TranslationService could be used to have multi-lingual notifications.
| }); | ||
|
|
||
| // Configuration Modal | ||
| const configModalEl = document.getElementById('pluginConfigModal'); |
There was a problem hiding this comment.
I would name it pluginConfigModal
|
|
||
| Object.keys(configData).forEach((key) => { | ||
| const value = configData[key]; | ||
| const div = document.createElement('div'); |
There was a problem hiding this comment.
You can use addElement() to create HTML elements.
| $prefix . 'faquser_right', | ||
| $prefix . 'faqvisits', | ||
| $prefix . 'faqvoting', | ||
| $prefix . 'faqdata_plugins', |
There was a problem hiding this comment.
I would name the table "faqplugins"
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (1)
1-1: Fix formatting issues detected by pipeline.The pipeline detected formatting changes. Run the formatter to fix:
mago format phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php
🤖 Fix all issues with AI agents
In @phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php:
- Around line 1-98: The file has styling/formatting violations causing the
pipeline to fail; run the repository formatter (mago) on
phpMyFAQ/Controller/Administration/Api/PluginController.php or apply the
project's PHP coding standard (e.g., PSR-12) so the class PluginController and
its methods toggleStatus and saveConfig are consistently formatted (spacing,
indentation, docblocks, casts, and array syntax), then stage and commit the
formatted file so the CI passes.
In @phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php:
- Around line 267-293: The updatePluginStatus method currently builds SQL with
string concatenation and escape() (same issue as savePluginConfig); replace the
SELECT/UPDATE/INSERT calls in updatePluginStatus to use prepared statements with
bound parameters on $db (e.g. prepare + execute) instead of sprintf/escape,
binding the plugin name and active flag as parameters, then use the
prepared-statement result (or rowCount) to decide whether to run an UPDATE or
INSERT, and execute the appropriate prepared statement with bound values; ensure
you use the same DB API conventions used elsewhere in this class for
prepare/execute and error handling.
- Around line 235-262: The savePluginConfig method currently builds SQL with
sprintf and $db->escape (which is a no-op in PdoMysql), creating an SQL
injection risk; change savePluginConfig to use parameterized queries for the
SELECT, UPDATE and INSERT (use placeholders for name and config) and bind
$pluginName and the JSON-encoded $configData when executing; if the Database
driver does not expose PDO prepare/execute, add or use an abstraction method
(e.g., prepare/execute or queryWithParams) on the Database/driver so you can
call prepared statements from savePluginConfig instead of sprintf, or
alternatively harden PdoMysql::escape to actually escape input—prefer the
prepared-statement approach to fix the vulnerability.
🧹 Nitpick comments (4)
phpmyfaq/admin/assets/src/configuration/plugins.ts (1)
74-76: Use translated string for modal title.The modal title uses a hard-coded "Configuration: " prefix. This should be translated for consistency with the rest of the application.
♻️ Recommended refactor
+const translate = new TranslationService(); if (modalTitle && pluginName) { - modalTitle.textContent = 'Configuration: ' + pluginName; + modalTitle.textContent = translate.get('msgConfig') + ': ' + pluginName; }As per coding guidelines, use translation features for user-facing text.
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (3)
146-147: Use professional tone in code comments.The comment uses first-person narrative ("I will default"). Consider rephrasing to: "Defaults to inactive (false) for new plugins found on disk but not in DB."
201-202: Remove uncertain/informal comment.The comment expresses uncertainty ("actually, maybe keep it?") which doesn't belong in production code. If the current behavior is correct, update the comment to explain why incompatible plugins are removed from the array.
304-309: Consider more specific exception handling.Catching a generic
\Exceptionwill suppress all errors, not just missing table errors. If the database layer provides specific exceptions for "table not found" errors, catch those specifically.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
phpmyfaq/admin/assets/src/api/plugins.tsphpmyfaq/admin/assets/src/configuration/plugins.tsphpmyfaq/assets/templates/admin/configuration/plugins.twigphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlsrv.phpphpmyfaq/src/phpMyFAQ/Plugin/PluginManager.phpphpmyfaq/src/phpMyFAQ/Setup/Update.phpphpmyfaq/translations/language_de.phpphpmyfaq/translations/language_en.php
🚧 Files skipped from review as they are similar to previous changes (3)
- phpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlsrv.php
- phpmyfaq/src/phpMyFAQ/Setup/Update.php
- phpmyfaq/translations/language_de.php
🧰 Additional context used
📓 Path-based instructions (3)
**/phpmyfaq/src/**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
Use PER Coding Style 3.0 for PHP code
Files:
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.phpphpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript coding standards for TypeScript code
Use semicolons at the end of each statement
Use single quotes for strings
Use arrow functions for callbacks
Files:
phpmyfaq/admin/assets/src/api/plugins.tsphpmyfaq/admin/assets/src/configuration/plugins.ts
**/translations/**
📄 CodeRabbit inference engine (AGENTS.md)
Use UTF-8 encoding for all translation files
Files:
phpmyfaq/translations/language_en.php
🧠 Learnings (1)
📚 Learning: 2025-12-26T21:59:16.058Z
Learnt from: CR
Repo: thorsten/phpMyFAQ PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-26T21:59:16.058Z
Learning: Avoid hard-coded strings in code; use translation features for user-facing text
Applied to files:
phpmyfaq/admin/assets/src/configuration/plugins.ts
🧬 Code graph analysis (3)
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php (2)
phpmyfaq/src/phpMyFAQ/Session/Token.php (2)
Token(27-219)verifyToken(132-162)phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (5)
loadPlugins(87-206)getPlugins(350-353)activatePlugin(211-214)deactivatePlugin(219-222)savePluginConfig(235-262)
phpmyfaq/admin/assets/src/api/plugins.ts (3)
phpmyfaq/admin/assets/src/interfaces/response.ts (1)
Response(1-8)phpmyfaq/admin/assets/js/configuration.js (1)
response(49-56)phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (1)
savePluginConfig(235-262)
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (3)
phpmyfaq/src/phpMyFAQ/Configuration.php (2)
Configuration(47-530)getDb(134-137)phpmyfaq/src/phpMyFAQ/Plugin/PluginInterface.php (6)
getName(29-29)getConfig(54-54)registerEvents(59-59)getTranslationsPath(75-75)getStylesheets(67-67)getScripts(83-83)phpmyfaq/src/phpMyFAQ/Translation.php (2)
Translation(25-436)registerPluginTranslations(292-322)
🪛 ast-grep (0.40.3)
phpmyfaq/admin/assets/src/configuration/plugins.ts
[warning] 93-93: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: container.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🪛 GitHub Actions: phpMyFAQ Build
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php
[error] 1-1: mago format --dry-run detected formatting changes for the file. Apply fixes with the formatter. Command: mago format --dry-run
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php
[error] 1-1: mago format --dry-run detected formatting changes for the file. Apply fixes with the formatter. Command: mago format --dry-run
🔇 Additional comments (17)
phpmyfaq/translations/language_en.php (1)
1463-1474: LGTM! Translation updates support the new plugin management features.The translation key updates are consistent and well-structured. The changes from "Plugin name" to "Plugin Name" and "List of..." to "Plugin Listing" improve consistency, and the new keys (msgActions, msgConfig, msgNoPluginConfig, msgPluginImplementation, msgPluginReason) properly support the new UI components introduced in the template.
phpmyfaq/assets/templates/admin/configuration/plugins.twig (3)
34-42: Well-implemented plugin toggle with proper sanitization.The Active column correctly uses a sanitized plugin name for HTML IDs, preventing invalid IDs from plugins with special characters. The form-check-switch pattern is appropriate for toggle functionality, and the visually-hidden label maintains accessibility.
45-59: Configuration button properly passes plugin metadata.The button correctly includes all necessary data attributes (name, description, implementation, config) with proper HTML attribute escaping applied to the JSON config. The
e('html_attr')filter ensures special characters in JSON are safely encoded for use in HTML attributes.
65-105: Modal structure is well-organized and accessible.The plugin configuration modal follows Bootstrap best practices with proper ARIA attributes, a scrollable dialog for long content, and a clear structure separating description, implementation display, configuration form, and action buttons.
phpmyfaq/admin/assets/src/api/plugins.ts (2)
25-44: Excellent error handling and CSRF protection.The function properly includes CSRF protection via the X-CSRF-Token header and implements robust error handling by checking
response.okbefore parsing JSON. The error message extraction with fallback ensures useful feedback is provided to callers.
53-77: Configuration save function is well-implemented.The function mirrors the error handling pattern from
togglePluginStatusand correctly includes the CSRF token in both the header and body (as noted in the comment on line 67) for backend validation flexibility.phpmyfaq/admin/assets/src/configuration/plugins.ts (3)
93-93: innerHTML clearing is safe in this context.The static analysis tool flagged this line as a potential XSS risk, but setting
innerHTML = ''to clear the container is safe since no user input is being inserted. The subsequent dynamic content generation usescreateElementandappendChild, which are XSS-safe methods.
121-167: Excellent input type inference and form field generation.The dynamic form field generation now properly detects data types and creates appropriate HTML input elements (checkbox for booleans, number inputs for numeric values, email/date inputs based on heuristics, and textarea for long strings). This is a significant improvement over the basic text-only approach.
204-219: Configuration serialization handles multiple input types correctly.The form data collection properly distinguishes between checkboxes (boolean), number inputs (parsed as int/float), and text inputs, ensuring the configuration data is serialized with correct types for backend storage.
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php (2)
34-47: Excellent CSRF protection and input validation.The endpoint properly validates the CSRF token before processing and includes comprehensive JSON decode error handling with appropriate HTTP status codes (403 for auth failure, 400 for invalid input).
66-94: Configuration save endpoint is secure and robust.The endpoint implements proper security controls (CSRF validation accepting token from either body or header) and wraps the database operation in exception handling to return meaningful error responses (404 for missing plugin, 500 for save failures).
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (6)
55-60: LGTM: Constructor properly initializes dependencies.The dependency injection of Configuration is appropriate for the DB-driven plugin management features.
89-91: Early return prevents re-initialization per request.This ensures plugins are loaded once per request. Note that activation/deactivation changes will only take effect on the next request.
227-230: LGTM: Correct implementation with strict comparison.
407-429: Good security hardening for path traversal prevention.The path normalization and case-insensitive directory checks effectively prevent path traversal attacks while maintaining Windows compatibility.
458-480: Good security hardening for script registration.Consistent with stylesheet registration, this properly prevents path traversal with
realpath()canonicalization and case-insensitive checks.
211-222: No changes needed. The methods function as designed: the controller validates plugin existence before invocation, andupdatePluginStatusincludes database-level existence checking.Likely an incorrect or invalid review comment.
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (1)
1-1: Resolve the formatting issue flagged by the pipeline.The pipeline reports that
mago format --dry-rundetects formatting changes needed for this file. Please run the formatter to ensure PER Coding Style 3.0 compliance.#!/bin/bash # Run the formatter to fix the issue mago format phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.phpAs per coding guidelines, PER Coding Style 3.0 should be used for PHP code in this directory.
🤖 Fix all issues with AI agents
In @phpmyfaq/src/phpMyFAQ/Database/Pgsql.php:
- Around line 321-347: The prepare method uses a deterministic statement name
('pmf_stmt_' . md5($query)) which can collide when the same query is prepared
multiple times; update prepare(string $query, array $options = []) and related
execute(mixed $statement, array $params = []) to avoid collisions by tracking
prepared names or generating unique names per prepare call (e.g., append a
unique suffix or incrementing counter) and store the mapping on the Pgsql
instance (use a property like $preparedStatements to map query->stmtName or
stmtName->query); ensure prepare returns the unique statement name and execute
casts and uses that name as it currently does ((string)$statement) and keep the
$options parameter untouched for interface compatibility.
In @phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php:
- Around line 316-342: The execute() method currently ignores $params; fix by
handling both prepared resources and direct-query execution: keep sqlsrv_prepare
usage in prepare() for cases without runtime params, but have prepare() return
the original query string (or allow callers to pass a query string) when you
intend to supply parameters at execution time; then change execute(mixed
$statement, array $params = []) to detect if $statement is a resource and
$params is empty -> call sqlsrv_execute($statement), otherwise treat $statement
as a SQL string and call sqlsrv_query($this->conn, $statement, $params) so
parameters are actually passed to SQL Server; ensure you handle types (resource
vs string) and return boolean consistent with other drivers.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
phpmyfaq/admin/assets/src/api/plugins.tsphpmyfaq/admin/assets/src/configuration/plugins.tsphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.phpphpmyfaq/src/phpMyFAQ/Database/DatabaseDriver.phpphpmyfaq/src/phpMyFAQ/Database/Mysqli.phpphpmyfaq/src/phpMyFAQ/Database/PdoMysql.phpphpmyfaq/src/phpMyFAQ/Database/PdoPgsql.phpphpmyfaq/src/phpMyFAQ/Database/PdoSqlite.phpphpmyfaq/src/phpMyFAQ/Database/PdoSqlsrv.phpphpmyfaq/src/phpMyFAQ/Database/Pgsql.phpphpmyfaq/src/phpMyFAQ/Database/Sqlite3.phpphpmyfaq/src/phpMyFAQ/Database/Sqlsrv.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Mysqli.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoMysql.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoPgsql.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlite.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlsrv.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Pgsql.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Sqlite3.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Sqlsrv.phpphpmyfaq/src/phpMyFAQ/Plugin/PluginManager.phpphpmyfaq/src/phpMyFAQ/Setup/Update.phpphpmyfaq/translations/language_de.phpphpmyfaq/translations/language_en.php
🚧 Files skipped from review as they are similar to previous changes (13)
- phpmyfaq/src/phpMyFAQ/Instance/Database/Mysqli.php
- phpmyfaq/src/phpMyFAQ/Database/PdoSqlsrv.php
- phpmyfaq/src/phpMyFAQ/Instance/Database/Sqlite3.php
- phpmyfaq/src/phpMyFAQ/Instance/Database/PdoPgsql.php
- phpmyfaq/src/phpMyFAQ/Database/PdoPgsql.php
- phpmyfaq/src/phpMyFAQ/Database/PdoMysql.php
- phpmyfaq/translations/language_en.php
- phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php
- phpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlsrv.php
- phpmyfaq/src/phpMyFAQ/Instance/Database/PdoMysql.php
- phpmyfaq/src/phpMyFAQ/Database/PdoSqlite.php
- phpmyfaq/src/phpMyFAQ/Instance/Database/Sqlsrv.php
- phpmyfaq/translations/language_de.php
🧰 Additional context used
📓 Path-based instructions (2)
**/phpmyfaq/src/**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
Use PER Coding Style 3.0 for PHP code
Files:
phpmyfaq/src/phpMyFAQ/Database/Sqlite3.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlite.phpphpmyfaq/src/phpMyFAQ/Plugin/PluginManager.phpphpmyfaq/src/phpMyFAQ/Database/DatabaseDriver.phpphpmyfaq/src/phpMyFAQ/Database/Pgsql.phpphpmyfaq/src/phpMyFAQ/Database/Sqlsrv.phpphpmyfaq/src/phpMyFAQ/Setup/Update.phpphpmyfaq/src/phpMyFAQ/Database/Mysqli.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Pgsql.php
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript coding standards for TypeScript code
Use semicolons at the end of each statement
Use single quotes for strings
Use arrow functions for callbacks
Files:
phpmyfaq/admin/assets/src/api/plugins.tsphpmyfaq/admin/assets/src/configuration/plugins.ts
🧠 Learnings (2)
📚 Learning: 2025-12-26T21:59:16.059Z
Learnt from: CR
Repo: thorsten/phpMyFAQ PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-26T21:59:16.059Z
Learning: Applies to **/phpmyfaq/src/**/*.php : Use PER Coding Style 3.0 for PHP code
Applied to files:
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php
📚 Learning: 2025-12-26T21:59:16.058Z
Learnt from: CR
Repo: thorsten/phpMyFAQ PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-26T21:59:16.058Z
Learning: Avoid hard-coded strings in code; use translation features for user-facing text
Applied to files:
phpmyfaq/admin/assets/src/configuration/plugins.ts
🧬 Code graph analysis (7)
phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php (2)
phpmyfaq/src/phpMyFAQ/Database/DatabaseDriver.php (3)
prepare(148-148)query(50-50)execute(157-157)phpmyfaq/src/phpMyFAQ/Database/PdoSqlite.php (3)
prepare(316-319)query(289-308)execute(327-330)
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (4)
phpmyfaq/src/phpMyFAQ/Configuration.php (1)
Configuration(47-530)phpmyfaq/src/phpMyFAQ/Plugin/PluginInterface.php (6)
getName(29-29)getConfig(54-54)registerEvents(59-59)getTranslationsPath(75-75)getStylesheets(67-67)getScripts(83-83)phpmyfaq/src/phpMyFAQ/Translation.php (2)
Translation(25-436)registerPluginTranslations(292-322)phpmyfaq/src/phpMyFAQ/Database.php (1)
getTablePrefix(138-141)
phpmyfaq/src/phpMyFAQ/Database/DatabaseDriver.php (6)
phpmyfaq/src/phpMyFAQ/Database/Mysqli.php (3)
prepare(326-329)query(298-317)execute(338-361)phpmyfaq/src/phpMyFAQ/Database/PdoMysql.php (3)
prepare(291-294)query(264-283)execute(302-305)phpmyfaq/src/phpMyFAQ/Database/PdoPgsql.php (3)
prepare(291-294)query(264-283)execute(302-305)phpmyfaq/src/phpMyFAQ/Database/PdoSqlite.php (3)
prepare(316-319)query(289-308)execute(327-330)phpmyfaq/src/phpMyFAQ/Database/Pgsql.php (3)
prepare(328-335)query(103-122)execute(344-347)phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php (3)
prepare(323-326)query(204-221)execute(335-342)
phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php (1)
phpmyfaq/src/phpMyFAQ/Database/DatabaseDriver.php (3)
prepare(148-148)query(50-50)execute(157-157)
phpmyfaq/src/phpMyFAQ/Setup/Update.php (1)
phpmyfaq/src/phpMyFAQ/Database.php (1)
getTablePrefix(138-141)
phpmyfaq/admin/assets/src/api/plugins.ts (1)
phpmyfaq/admin/assets/src/interfaces/response.ts (1)
Response(1-8)
phpmyfaq/admin/assets/src/configuration/plugins.ts (4)
phpmyfaq/assets/src/utils/TranslationService.ts (1)
TranslationService(3-19)phpmyfaq/admin/assets/src/api/plugins.ts (2)
togglePluginStatus(25-44)savePluginConfig(53-77)phpmyfaq/assets/src/utils/helper.ts (1)
addElement(34-50)phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (1)
savePluginConfig(235-256)
🪛 ast-grep (0.40.4)
phpmyfaq/admin/assets/src/configuration/plugins.ts
[warning] 98-98: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: container.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🪛 GitHub Actions: phpMyFAQ Build
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php
[error] 1-1: Mago format --dry-run reported formatting changes needed for this file.
🪛 PHPMD (2.15.0)
phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php
314-314: Avoid unused parameters such as '$options'. (undefined)
(UnusedFormalParameter)
phpmyfaq/src/phpMyFAQ/Database/Pgsql.php
328-328: Avoid unused parameters such as '$options'. (undefined)
(UnusedFormalParameter)
phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php
335-335: Avoid unused parameters such as '$params'. (undefined)
(UnusedFormalParameter)
phpmyfaq/src/phpMyFAQ/Database/Mysqli.php
326-326: Avoid unused parameters such as '$options'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (26)
phpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlite.php (1)
176-180: LGTM! Plugin table schema is well-designed.The
faqpluginstable schema is appropriate for plugin management with a string primary key, boolean flag, and JSON config storage. The schema is consistent with SQLite conventions.phpmyfaq/src/phpMyFAQ/Database/DatabaseDriver.php (2)
141-148: LGTM! Prepared statement support properly abstracted.The
prepare()method appropriately usesmixedreturn type to accommodate different driver implementations (PDOStatement, mysqli_stmt, string for pg_prepare, resource for sqlsrv_prepare). This is a necessary enhancement for SQL injection prevention.
150-157: LGTM! Execute method completes the prepared statement API.The
execute()method design is sound, accepting the mixed-type statement fromprepare()and parameter array, returning a boolean. This enables secure parameterized queries across all database drivers.phpmyfaq/src/phpMyFAQ/Instance/Database/Pgsql.php (1)
180-184: LGTM! Consistent plugin table schema for PostgreSQL.The
faqpluginstable definition is consistent with the SQLite implementation and uses appropriate PostgreSQL data types. The schema correctly supports plugin state and configuration persistence.phpmyfaq/src/phpMyFAQ/Setup/Update.php (2)
187-188: LGTM! Correct placement in update sequence.The 4.2 alpha updates are appropriately positioned before table optimization, consistent with the update pattern for other versions.
1128-1179: Resolve the formatting issue flagged by the pipeline.The pipeline reports that
mago format --dry-runstill detects formatting changes needed for this file, despite a past review comment indicating this was addressed. Please run the formatter to ensure compliance.#!/bin/bash # Run the formatter to fix the issue mago format phpmyfaq/src/phpMyFAQ/Setup/Update.phpLikely an incorrect or invalid review comment.
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (9)
55-60: LGTM! Proper dependency injection.The constructor now accepts
Configurationas a dependency, enabling database-backed plugin state management. The readonly modifier ensures immutability.
89-91: LGTM! Idempotency guard prevents duplicate loading.The early return when plugins are already loaded prevents duplicate registration and unnecessary database queries.
104-148: LGTM! Robust DB-backed plugin state integration.The database integration correctly:
- Fetches plugin states and configs
- Applies activation status
- Merges JSON config with type coercion via reflection
- Handles missing config gracefully with
is_array()check- Defaults new plugins to inactive
The reflection-based type coercion for int/bool/float is well-guarded with try-catch.
153-205: LGTM! Activation logic properly gates plugin loading.The activation check correctly ensures only active plugins with satisfied dependencies are loaded. The
method_existsguards for translations, stylesheets, and scripts provide good defensive programming against incomplete plugin implementations.
208-230: LGTM! Clean lifecycle API.The activation/deactivation methods correctly update database state. Note that activation changes take effect on the next
loadPlugins()call, which is appropriate for maintaining system stability without runtime plugin reloading.
232-256: Excellent! SQL injection vulnerability has been fixed.The critical security issue flagged in past reviews has been properly addressed. The code now uses prepared statements with parameter binding (
prepare()with?placeholders andexecute()with parameter arrays) instead of string concatenation withescape(). This is the correct and secure approach.
258-282: Excellent! SQL injection vulnerability has been fixed here too.The critical security issue in
updatePluginStatushas been properly resolved using prepared statements with parameter binding, matching the secure approach insavePluginConfig.
284-308: LGTM! Safe database query with good error handling.The direct query is safe since
$tableis constructed from the internal table prefix, not user input. The try-catch appropriately handles scenarios where the table doesn't exist yet (e.g., during installation/upgrade).
396-469: LGTM! Asset registration with appropriate security controls.The stylesheet and script registration methods properly:
- Normalize paths and handle Windows/Unix differences
- Prevent directory traversal with
realpath()and case-insensitivestr_starts_with()checks- Validate file existence before registration
- Store web-relative paths for template use
The
realpath()fallback using the?:operator handles edge cases gracefully.phpmyfaq/src/phpMyFAQ/Database/Pgsql.php (1)
317-317: LGTM!The
faqpluginstable is correctly added to the list of table names.phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php (1)
312-312: LGTM!The
faqpluginstable is correctly added to the list of table names.phpmyfaq/src/phpMyFAQ/Database/Mysqli.php (2)
255-255: LGTM!The
faqpluginstable is correctly added to the list of table names.
319-361: Well-implemented prepared statement support with automatic type binding.The implementation correctly handles parameter type detection and binding. The fallback to
'b'(blob) for non-standard types is reasonable for edge cases.Minor note: The unused
$optionsparameter is acceptable as it's required by theDatabaseDriverinterface.phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php (2)
273-273: LGTM!The
faqpluginstable is correctly added to the list of table names.
307-345: Well-implemented prepared statement support with proper SQLite type binding.The implementation correctly uses 1-based indexing for
bindValue()and properly detects SQLite data types. The unused$optionsparameter is acceptable as it's required by theDatabaseDriverinterface.phpmyfaq/admin/assets/src/api/plugins.ts (2)
1-44: LGTM - CSRF protection and error handling properly implemented.The function correctly includes the CSRF token in the header and checks
response.okbefore parsing the response. The error handling properly attempts to parse error details from the response body.As per coding guidelines, single quotes and semicolons are correctly used.
46-77: LGTM with a minor observation on duplicate CSRF token.The CSRF token is sent in both the header (
X-CSRF-Token) and the body (csrf). The comment on line 67 indicates this is intentional for backend validation. If only one location is needed, consider removing the redundancy.phpmyfaq/admin/assets/src/configuration/plugins.ts (3)
22-58: Well-implemented plugin toggle functionality with proper error handling.The implementation correctly:
- Uses the TranslationService for localized notifications (as suggested in past comments)
- Passes CSRF token to API calls
- Reverts checkbox state on failure
- Handles both API errors and exceptions
60-172: Good modal implementation with dynamic form field generation.The configuration modal properly:
- Uses
addElement()helper as suggested- Generates appropriate input types (checkbox for boolean, number for numeric values, email/date for matching key names)
- Handles textarea for long text values
- Shows/hides the save button based on config presence
The static analysis warning about
container.innerHTML = ''on line 99 is a false positive - setting innerHTML to an empty string to clear the container is safe.
175-226: Solid form serialization and save implementation.The save handler correctly:
- Parses checkbox values as booleans
- Parses number inputs as integers or floats
- Handles NaN edge case by defaulting to 0
- Reloads the page on success to reflect changes
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
phpmyfaq/src/phpMyFAQ/Database/Pgsql.phpphpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php
🧰 Additional context used
📓 Path-based instructions (1)
**/phpmyfaq/src/**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
Use PER Coding Style 3.0 for PHP code
Files:
phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.phpphpmyfaq/src/phpMyFAQ/Database/Pgsql.php
🧬 Code graph analysis (1)
phpmyfaq/src/phpMyFAQ/Database/Pgsql.php (2)
phpmyfaq/src/phpMyFAQ/Database/PdoPgsql.php (3)
prepare(291-294)query(264-283)execute(302-305)phpmyfaq/src/phpMyFAQ/Database/DatabaseDriver.php (3)
prepare(148-148)query(50-50)execute(157-157)
🪛 PHPMD (2.15.0)
phpmyfaq/src/phpMyFAQ/Database/Pgsql.php
333-333: Avoid unused parameters such as '$options'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (5)
phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php (2)
312-312: LGTM!The addition of the
faqpluginstable to the table names array is correct and aligns with the plugin system feature.
316-350: Implementation correctly addresses SQL Server parameter binding constraints and complies with PER Coding Style 3.0.The revised approach properly handles SQL Server's requirement that parameters must be passed as references at prepare-time. By returning the query string when placeholders exist and deferring binding to
sqlsrv_query()inexecute(), parameters are now correctly bound.Consistency check across drivers confirms this architecture is justified: Mysqli, Sqlite3, and Pgsql each use their underlying PHP extension's native API, which allow parameter binding after prepare. SQL Server's API requires a different pattern, hence the conditional return type. All implementations correctly use their extension's capabilities.
One edge case: passing parameters to
execute()with a prepared resource (no placeholders) silently returnsfalse. This is correct behavior—you cannot bind parameters to a statement without placeholders—though consider adding a warning log if unexpected.phpmyfaq/src/phpMyFAQ/Database/Pgsql.php (3)
55-58: LGTM! Property is well-defined.The
$preparedStatementsproperty provides a clean mechanism to track prepared statements and ensure unique naming.
322-322: LGTM! Table name addition is correct.The
faqpluginstable is properly added to support the plugin management functionality.
326-341: Implementation resolves the past review concern about statement name collisions.The counter-based approach (
count($this->preparedStatements) + 1) ensures eachprepare()call generates a unique statement name, eliminating the collision issue raised in the previous review wheremd5($query)would produce identical names when preparing the same query multiple times.The unused
$optionsparameter is acceptable—it's required by theDatabaseDriverinterface for cross-driver consistency.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (2)
398-420: Path traversal vulnerability and code duplication.Critical security issue: The case-insensitive path validation (lines 412, 462) using
strtolower()is intended for Windows compatibility but introduces a vulnerability on case-sensitive filesystems. An attacker could potentially craft paths that bypass the security check using mixed case (e.g.,../../Etc/passwd).Refactoring: The path normalization and validation logic is duplicated between
registerPluginStylesheetsandregisterPluginScripts. Extract this into a private helper method.🔒 Proposed security fix
- // Normalize the path for comparison (slashes and case) - $realPluginDir = str_replace('\\', '/', realpath($pluginDir) ?: $pluginDir); - $realPluginDir = rtrim($realPluginDir, '/') . '/'; - - foreach ($stylesheets as $stylesheet) { - $fullPath = $pluginDir . '/' . $stylesheet; - $absolutePath = str_replace('\\', '/', realpath($fullPath) ?: $fullPath); - - // Security check: Must be inside the plugin directory (case-insensitive for Windows) - if (str_starts_with(strtolower($absolutePath), strtolower($realPluginDir)) && file_exists($fullPath)) { - $validatedStylesheets[] = 'content/plugins/' . $pluginName . '/' . $stylesheet; - } - } + foreach ($stylesheets as $stylesheet) { + if ($validatedPath = $this->validatePluginAssetPath($pluginName, $pluginDir, $stylesheet)) { + $validatedStylesheets[] = $validatedPath; + } + }Add a new private method:
/** * Validates and normalizes a plugin asset path * * @return string|null Returns web-relative path if valid, null otherwise */ private function validatePluginAssetPath(string $pluginName, string $pluginDir, string $assetPath): ?string { // Normalize slashes $pluginDir = str_replace('\\', '/', $pluginDir); $assetPath = str_replace('\\', '/', $assetPath); // Build full path and resolve it $fullPath = $pluginDir . '/' . $assetPath; $realFullPath = realpath($fullPath); $realPluginDir = realpath($pluginDir); // Both must resolve successfully if ($realFullPath === false || $realPluginDir === false) { return null; } // Normalize for comparison $realFullPath = str_replace('\\', '/', $realFullPath); $realPluginDir = str_replace('\\', '/', rtrim($realPluginDir, '/')) . '/'; // Security check: case-sensitive prefix match if (!str_starts_with($realFullPath, $realPluginDir)) { return null; } return 'content/plugins/' . $pluginName . '/' . $assetPath; }This fix:
- Uses case-sensitive comparison (safer)
- Requires both paths to resolve via
realpath()(prevents path traversal)- Eliminates code duplication
Also applies to: 448-470
1-524: Pipeline failure: Code formatting required.The pipeline reports that Mago formatting is required. Run
mago formatto fix code style issues before merging.#!/bin/bash # Run Mago formatter to fix code style mago format phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.phpphpmyfaq/src/phpMyFAQ/Database/Pgsql.php (1)
147-158: Potential null dereference when callingpg_result_status().If
pg_query()returnsfalseon line 147,pg_result_status($result)on line 153 will be called withfalse, which may cause unexpected behavior or a warning.Proposed fix
$result = pg_query($this->conn, $query); if (!$result) { $this->sqlLog .= $this->error(); + return false; } if (pg_result_status($result) === PGSQL_COMMAND_OK) { return true; } return $result;
🤖 Fix all issues with AI agents
In @phpmyfaq/src/phpMyFAQ/Database/Mysqli.php:
- Around line 394-397: When $statement->stmt->execute() succeeds, do not
directly assign the return of $statement->stmt->get_result() to
$statement->result because get_result() can return false; instead call
get_result(), check if it === false and handle that case by setting
$statement->result to null (or an appropriate empty value) and/or falling back
to non-result metadata (e.g. $statement->stmt->affected_rows or store_result()
as applicable); update the code around $statement->stmt->execute() /
$statement->stmt->get_result() so downstream callers of $statement->result can
safely handle no-result queries or environments without mysqlnd.
In @phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php:
- Around line 366-376: The binding loop in Sqlite3.php treats booleans as text
because there is no is_bool check; update the foreach over $params to detect
is_bool($param) and bind it as SQLITE3_INTEGER (casting the value to int) when
calling $statement->stmt->bindValue so booleans are stored as 0/1; keep existing
branches for is_int/is_float/is_null and ensure the boolean branch uses
SQLITE3_INTEGER and (int)$param.
In @phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php:
- Around line 351-364: The file contains two consecutive PHPDoc blocks for the
prepare() method; remove the duplicate block so only one PHPDoc remains
immediately above the prepare() method declaration—prefer keeping the more
specific block that returns "SqlsrvStatement|false" and delete the earlier,
redundant block to avoid duplicate documentation.
In @phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php:
- Around line 114-143: The JSON decode of $dbPlugins[$pluginName]['config'] may
fail silently; after calling json_decode(...) assign its result to $configArray,
then check json_last_error() and log or throw via the plugin manager logger when
decoding fails (include the raw config and json_last_error_msg()); only proceed
if json_decode returned an array. Also extend the ReflectionProperty type
handling in the loop over $configArray (where you use $rp = new
\ReflectionProperty($configObject, $key) and $typeName = $type->getName()) to
explicitly handle 'string' (cast to (string)) and 'array' (ensure value is an
array, e.g., by json_decoding the individual value or validating/is_array and
falling back to an empty array) and keep the existing int/bool/float cases, so
property assignments on $configObject->$key preserve expected types and
malformed inputs are reported.
- Around line 287-308: The getPluginsFromDatabase method builds a query with
direct table-name interpolation and catches a broad \Exception; change it to
build/execute the SELECT using the database API's safe methods (e.g., prepared
statement or the DB wrapper's safe table quoting) instead of raw string
interpolation of \phpMyFAQ\Database::getTablePrefix(), and replace the
catch(\Exception $e) with the specific database exception type thrown by the DB
layer so only missing-table errors are handled (rethrow or let others bubble).
Locate getPluginsFromDatabase, the $table construction that uses
\phpMyFAQ\Database::getTablePrefix(), and the $db->query / $db->fetchObject
usage to update the query execution and exception type.
- Around line 211-230: activatePlugin and deactivatePlugin only change DB via
updatePluginStatus but don't update runtime $this->loadedPlugins, so
isPluginActive can return stale data; fix by mutating $this->loadedPlugins
immediately after updating the DB: in activatePlugin(string $pluginName) call
updatePluginStatus($pluginName, true) then add $pluginName to
$this->loadedPlugins if not present, and in deactivatePlugin(string $pluginName)
call updatePluginStatus($pluginName, false) then remove $pluginName from
$this->loadedPlugins if present; alternatively (or additionally) provide a
public reloadPlugins() method that calls loadPlugins() and document that admins
must reload for changes to take effect—use the existing updatePluginStatus,
activatePlugin, deactivatePlugin, isPluginActive, loadPlugins and
$this->loadedPlugins symbols to locate where to apply the change.
🧹 Nitpick comments (8)
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (3)
89-91: Consider thread-safety implications.The early return check is not atomic. If the application architecture evolves to support concurrent plugin loading (e.g., async workers), this could lead to race conditions. For now, PHP's request-scoped execution model makes this safe.
235-256: Add input validation and consider extracting upsert logic.The method lacks validation for the
$pluginNameparameter (could contain unexpected characters) and the$configDataarray (could be too large or contain non-serializable data).Additionally, the "check existence then INSERT or UPDATE" pattern is duplicated with
updatePluginStatus(). Consider extracting this into a privateupsertPlugin()helper method to reduce duplication.♻️ Suggested improvements
Add validation:
public function savePluginConfig(string $pluginName, array $configData): void { + if (empty($pluginName) || preg_match('/[^a-zA-Z0-9_-]/', $pluginName)) { + throw new \InvalidArgumentException('Invalid plugin name'); + } + $jsonConfig = json_encode($configData); + if ($jsonConfig === false) { + throw new \RuntimeException('Failed to encode config data: ' . json_last_error_msg()); + }Extract upsert pattern to reduce duplication with
updatePluginStatus().
261-282: Consider transaction wrapping for atomic DB updates.DB operations that check-then-update/insert should ideally be wrapped in a transaction to prevent race conditions if multiple processes attempt to modify the same plugin simultaneously.
💡 Example with transaction
private function updatePluginStatus(string $pluginName, bool $active): void { $db = $this->configuration->getDb(); $table = \phpMyFAQ\Database::getTablePrefix() . 'faqplugins'; $activeInt = $active ? 1 : 0; + + $db->beginTransaction(); + try { // Check if exists $select = sprintf('SELECT name FROM %s WHERE name = ?', $table); // ... rest of logic ... $stmt = $db->prepare($query); $db->execute($stmt, $params); + $db->commit(); + } catch (\Exception $e) { + $db->rollBack(); + throw $e; + } }phpmyfaq/src/phpMyFAQ/Database/Pgsql.php (2)
30-59: Consider movingPgsqlStatementto its own file.The
PgsqlStatementclass is defined inline withinPgsql.php. For PSR-4 autoloading compliance and maintainability, consider extracting it tophpmyfaq/src/phpMyFAQ/Database/PgsqlStatement.php.
361-376: Unused$optionsparameter inprepare().The
$optionsparameter is declared but never used. This is intentional for interface consistency with other drivers (PDO uses options), but consider adding a PHPDoc@SuppressWarnings(PHPMD.UnusedFormalParameter)annotation to silence the static analysis warning.Suppress unused parameter warning
/** * Prepares a statement for execution and returns a statement object. * * @param string $query The SQL query * @param array $options The driver options * @return PgsqlStatement|false + * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ public function prepare(string $query, array $options = []): PgsqlStatement|falsephpmyfaq/src/phpMyFAQ/Database/Mysqli.php (1)
30-54: Consider movingMysqliStatementto its own file.Same as
PgsqlStatement, this class should ideally be in its own file for PSR-4 compliance.phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php (1)
25-49: Consider movingSqlite3Statementto its own file.Consistent with other drivers, consider extracting to a separate file.
phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php (1)
27-56: Consider movingSqlsrvStatementto its own file.Consistent with other drivers, consider extracting to a separate file.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
phpmyfaq/src/phpMyFAQ/Database/Mysqli.phpphpmyfaq/src/phpMyFAQ/Database/Pgsql.phpphpmyfaq/src/phpMyFAQ/Database/Sqlite3.phpphpmyfaq/src/phpMyFAQ/Database/Sqlsrv.phpphpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php
🧰 Additional context used
📓 Path-based instructions (1)
**/phpmyfaq/src/**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
Use PER Coding Style 3.0 for PHP code
Files:
phpmyfaq/src/phpMyFAQ/Database/Sqlite3.phpphpmyfaq/src/phpMyFAQ/Database/Pgsql.phpphpmyfaq/src/phpMyFAQ/Database/Mysqli.phpphpmyfaq/src/phpMyFAQ/Plugin/PluginManager.phpphpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php
🪛 GitHub Actions: phpMyFAQ Build
phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php
[error] 1-1: Mago formatting required. File needs to be formatted. Run 'mago format' to fix code style issues in this file.
phpmyfaq/src/phpMyFAQ/Database/Pgsql.php
[error] 1-1: Mago formatting required. File needs to be formatted. Run 'mago format' to fix code style issues in this file.
phpmyfaq/src/phpMyFAQ/Database/Mysqli.php
[error] 1-1: Mago formatting required. File needs to be formatted. Run 'mago format' to fix code style issues in this file.
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php
[error] 1-1: Mago formatting required. File needs to be formatted. Run 'mago format' to fix code style issues in this file.
phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php
[error] 1-1: Mago formatting required. File needs to be formatted. Run 'mago format' to fix code style issues in this file.
🪛 PHPMD (2.15.0)
phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php
344-344: Avoid unused parameters such as '$options'. (undefined)
(UnusedFormalParameter)
phpmyfaq/src/phpMyFAQ/Database/Pgsql.php
368-368: Avoid unused parameters such as '$options'. (undefined)
(UnusedFormalParameter)
phpmyfaq/src/phpMyFAQ/Database/Mysqli.php
356-356: Avoid unused parameters such as '$options'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (10)
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (2)
55-60: LGTM! Clean dependency injection.The constructor properly accepts the Configuration dependency using PHP 8 promoted properties, enabling DB-backed plugin management.
153-204: Activation gating logic looks solid.The conditional registration based on active state is correctly implemented with appropriate guards for optional plugin methods. The dependency checking preserves the original incompatibility handling behavior.
phpmyfaq/src/phpMyFAQ/Database/Pgsql.php (1)
1-1: Pipeline failure: Mago formatting required.The CI pipeline indicates this file needs formatting. Run
mago formatto fix code style issues.phpmyfaq/src/phpMyFAQ/Database/Mysqli.php (2)
378-400: Parameter binding works correctly.The type inference logic for
bind_param()correctly maps PHP types to mysqli type specifiers (i/d/s/b). The implementation handles the common use cases well.Note: Using
'b'(blob) as fallback for non-scalar types might not work as expected since blob parameters require special handling withsend_long_data(). Consider throwing an exception for unsupported types instead.
1-1: Pipeline failure: Mago formatting required.Run
mago formatto fix code style issues in this file.phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php (2)
337-351: Consistent implementation with other drivers.The
prepare()method follows the same pattern as other drivers. The unused$optionsparameter maintains interface consistency.
1-1: Pipeline failure: Mago formatting required.Run
mago formatto fix code style issues in this file.phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php (3)
399-405: Parameterized queries executed viasqlsrv_query()with direct parameter binding.When
statement->statementis a string (from the?placeholder path), parameters are passed directly tosqlsrv_query(). This is correct for SQL Server's parameterized query support - the driver handles parameter binding safely. However, this mixing of approaches (prepared statements vs. direct parameterized queries) may cause confusion.
1-1: Pipeline failure: Mago formatting required.Run
mago formatto fix code style issues in this file.
365-376: Remove the SQL injection concern; the implementation is secure.While the
prepare()method does behave differently from other drivers (returning a raw SQL string when?is detected, rather than a prepared statement resource), this pattern is intentional for thesqlsrvextension. Theexecute()method correctly handles both cases—when it receives a string statement, it usessqlsrv_query()with the parameters array, which thesqlsrvextension properly binds separately to prevent SQL injection. This is the correct approach for thesqlsrvAPI. No security risk exists here.If code consistency across drivers is a concern, note that this class is already marked
@deprecatedwith plans for removal in v5.0 in favor of PDO.Likely an incorrect or invalid review comment.
| /** | ||
| * Prepares a statement for execution and returns a statement object. | ||
| * | ||
| * @param string $query The SQL query | ||
| * @param array $options The driver options | ||
| * @return resource|string|false | ||
| */ | ||
| /** | ||
| * Prepares a statement for execution and returns a statement object. | ||
| * | ||
| * @param string $query The SQL query | ||
| * @param array $options The driver options | ||
| * @return SqlsrvStatement|false | ||
| */ |
There was a problem hiding this comment.
Duplicate PHPDoc block.
There are two consecutive PHPDoc blocks for the prepare() method. Remove the duplicate.
Proposed fix
- /**
- * Prepares a statement for execution and returns a statement object.
- *
- * @param string $query The SQL query
- * @param array $options The driver options
- * @return resource|string|false
- */
/**
* Prepares a statement for execution and returns a statement object.
*
* @param string $query The SQL query
* @param array $options The driver options
* @return SqlsrvStatement|false
*/
public function prepare(string $query, array $options = []): SqlsrvStatement|false🤖 Prompt for AI Agents
In @phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php around lines 351 - 364, The file
contains two consecutive PHPDoc blocks for the prepare() method; remove the
duplicate block so only one PHPDoc remains immediately above the prepare()
method declaration—prefer keeping the more specific block that returns
"SqlsrvStatement|false" and delete the earlier, redundant block to avoid
duplicate documentation.
| public function activatePlugin(string $pluginName): void | ||
| { | ||
| $this->updatePluginStatus($pluginName, true); | ||
| } | ||
|
|
||
| /** | ||
| * Deactivates a plugin | ||
| */ | ||
| public function deactivatePlugin(string $pluginName): void | ||
| { | ||
| $this->updatePluginStatus($pluginName, false); | ||
| } | ||
|
|
||
| /** | ||
| * Checks if a plugin is active | ||
| */ | ||
| public function isPluginActive(string $pluginName): bool | ||
| { | ||
| return in_array($pluginName, $this->loadedPlugins, true); | ||
| } |
There was a problem hiding this comment.
Runtime state is not synchronized with DB changes.
The activatePlugin and deactivatePlugin methods only update the database but don't modify the runtime state ($this->loadedPlugins). This means isPluginActive() will return stale information until the next request when loadPlugins() runs again.
Consider either:
- Updating
$this->loadedPluginsimmediately (add/remove from array) - Documenting that plugin state changes require a reload/new request
- Providing a
reloadPlugins()method to refresh state
The current behavior could confuse administrators who toggle plugins and don't see immediate effects.
🔄 Example fix for immediate state update
public function activatePlugin(string $pluginName): void
{
$this->updatePluginStatus($pluginName, true);
+ if (!in_array($pluginName, $this->loadedPlugins, true)) {
+ $this->loadedPlugins[] = $pluginName;
+ }
}
public function deactivatePlugin(string $pluginName): void
{
$this->updatePluginStatus($pluginName, false);
+ $key = array_search($pluginName, $this->loadedPlugins, true);
+ if ($key !== false) {
+ unset($this->loadedPlugins[$key]);
+ $this->loadedPlugins = array_values($this->loadedPlugins);
+ }
}Note: Full activation/deactivation would also require registering/unregistering events and assets, which is more complex.
🤖 Prompt for AI Agents
In @phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php around lines 211 - 230,
activatePlugin and deactivatePlugin only change DB via updatePluginStatus but
don't update runtime $this->loadedPlugins, so isPluginActive can return stale
data; fix by mutating $this->loadedPlugins immediately after updating the DB: in
activatePlugin(string $pluginName) call updatePluginStatus($pluginName, true)
then add $pluginName to $this->loadedPlugins if not present, and in
deactivatePlugin(string $pluginName) call updatePluginStatus($pluginName, false)
then remove $pluginName from $this->loadedPlugins if present; alternatively (or
additionally) provide a public reloadPlugins() method that calls loadPlugins()
and document that admins must reload for changes to take effect—use the existing
updatePluginStatus, activatePlugin, deactivatePlugin, isPluginActive,
loadPlugins and $this->loadedPlugins symbols to locate where to apply the
change.
| private function getPluginsFromDatabase(): array | ||
| { | ||
| $db = $this->configuration->getDb(); | ||
| $table = \phpMyFAQ\Database::getTablePrefix() . 'faqplugins'; | ||
|
|
||
| // Ensure table exists to avoid crashes during update/install if not yet run | ||
| try { | ||
| $result = $db->query("SELECT name, active, config FROM $table"); | ||
| } catch (\Exception $e) { | ||
| // Table might not exist yet | ||
| return []; | ||
| } | ||
|
|
||
| $plugins = []; | ||
| while ($row = $db->fetchObject($result)) { | ||
| $plugins[$row->name] = [ | ||
| 'active' => $row->active, | ||
| 'config' => $row->config | ||
| ]; | ||
| } | ||
| return $plugins; | ||
| } |
There was a problem hiding this comment.
SQL injection risk and overly broad exception handling.
Critical: Line 294 uses direct string interpolation for the table name in the query. While getTablePrefix() is likely safe, this pattern is risky and inconsistent with the prepared statement usage elsewhere in this class. If table prefix ever comes from user input or configuration, this becomes a critical SQL injection vulnerability.
Major: The catch block captures all \Exception instances, which could mask unrelated errors (e.g., connection failures, permission issues). This should catch specific database exceptions.
🔒 Proposed fixes
private function getPluginsFromDatabase(): array
{
$db = $this->configuration->getDb();
$table = \phpMyFAQ\Database::getTablePrefix() . 'faqplugins';
// Ensure table exists to avoid crashes during update/install if not yet run
try {
- $result = $db->query("SELECT name, active, config FROM $table");
+ $query = sprintf('SELECT name, active, config FROM %s', $table);
+ $result = $db->query($query);
- } catch (\Exception $e) {
+ } catch (\phpMyFAQ\Database\DatabaseException $e) {
// Table might not exist yet
+ error_log("Plugin table not found or query failed: " . $e->getMessage());
return [];
}Note: Use the specific database exception class that your framework provides instead of the generic \Exception.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php around lines 287 - 308, The
getPluginsFromDatabase method builds a query with direct table-name
interpolation and catches a broad \Exception; change it to build/execute the
SELECT using the database API's safe methods (e.g., prepared statement or the DB
wrapper's safe table quoting) instead of raw string interpolation of
\phpMyFAQ\Database::getTablePrefix(), and replace the catch(\Exception $e) with
the specific database exception type thrown by the DB layer so only
missing-table errors are handled (rethrow or let others bubble). Locate
getPluginsFromDatabase, the $table construction that uses
\phpMyFAQ\Database::getTablePrefix(), and the $db->query / $db->fetchObject
usage to update the query execution and exception type.
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 (1)
phpmyfaq/src/phpMyFAQ/Database/Mysqli.php (1)
1-1: Remove diff markers from lines 402–404.The file contains literal
+characters at the start of lines 402–404 that are breaking PHP parsing. These diff markers must be removed:Lines 402–404
$result = $statement->stmt->get_result(); -+ if ($result !== false) { -+ $statement->result = $result; -+ } + if ($result !== false) { + $statement->result = $result; + } }
🤖 Fix all issues with AI agents
In @docs/plugins.md:
- Line 33: Replace the extra/odd whitespace in the sentence containing "values
in the file" with a single standard ASCII space so the phrase reads "values in
the file"; locate the sentence in the docs/plugins.md text (the line that
currently reads "These predefined values in the file form the basis...") and
normalize the spacing (remove any non-breaking or zero-width space) between
"values" and "in".
In @phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php:
- Around line 34-37: The call to Token::getInstance(...)->verifyToken currently
passes the CSRF token as the first argument and the page id second; swap the
parameters so the page identifier 'admin-plugins' is the first argument and the
token variable $csrfToken is the second when calling verifyToken (keep using
Token::getInstance($this->session) and the same return/JsonResponse logic).
- Around line 81-84: In PluginController (Administration\Api\PluginController)
the call to Token::getInstance(...)->verifyToken currently passes ($csrfToken,
'admin-plugins') with parameters reversed; change the call to pass the token
name first and the token value second (i.e., verifyToken('admin-plugins',
$csrfToken)) so the CSRF check uses the correct arguments; also check and fix
the identical swap in the toggleStatus handler where verifyToken is used.
In @phpmyfaq/src/phpMyFAQ/Database/Mysqli.php:
- Around line 289-290: The array listing table names contains a duplicate entry
"$prefix . 'faqplugins'" on consecutive lines; remove the redundant line so
"$prefix . 'faqplugins'" only appears once in the table names array (ensure any
related usages still rely on a single entry and keep the list unique).
- Around line 169-176: In Mysqli.php remove the duplicate unwrapping check for
MysqliStatement: there are two identical blocks checking "if ($result instanceof
MysqliStatement) { $result = $result->result; }" — delete the second occurrence
so $result is unwrapped only once (keep the first block that performs the unwrap
on MysqliStatement) and ensure no other code relies on the redundant branch.
In @phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php:
- Around line 365-392: The pipeline reports a formatting discrepancy in the
execute method of Sqlite3 (phpMyFAQ/Database/Sqlite3.php) around the bool param
handling; there is no logic bug—run the project's code formatter (mago format as
used by CI, or the configured PHP formatter/CS fixer) on this file to apply the
expected whitespace/indentation changes, then stage the formatted file and
update the PR; target the execute(mixed $statement, array $params = []) method
where the boolean branch sets $type and $param to ensure the file matches the
formatter's output.
- Around line 307-308: The returned table list includes a duplicate entry for
$prefix . 'faqplugins'; locate the array (in the Sqlite3 class method that
builds/returns the table list, e.g., the method that constructs the $prefix .
'faqplugins' entries) and remove the repeated $prefix . 'faqplugins' element so
each table name appears only once, ensuring any downstream code receives a
unique list of tables.
- Around line 142-148: In Sqlite3.php inside the code handling $result (the
block that checks "if ($result instanceof Sqlite3Statement) { $result =
$result->result; }"), remove the duplicated second occurrence so the unwrapping
only happens once; locate the duplicate checks against Sqlite3Statement for the
$result variable and delete the redundant block to avoid dead code and preserve
the single unwrap behavior.
In @phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php:
- Line 146: The CI flagged formatting issues in
phpMyFAQ/Plugin/PluginManager.php around the PluginManager class (specifically
near the blocks at the locations corresponding to lines 146, 194, and 290); run
the project formatter (mago format) on that file, fix any
whitespace/indentation/bracing inconsistencies reported, verify the file formats
cleanly, and commit the formatted file so the pipeline passes.
- Around line 128-138: In PluginManager:: (inside the switch that handles
$typeName) replace the filter_var(..., FILTER_VALIDATE_BOOLEAN) usage with an
explicit boolean cast so boolean config values use (bool) $value; update the
case 'bool' branch to convert truthy/falsy values via (bool) $value (or add
explicit string checks if you need stricter "true"/"false" semantics) to avoid
FILTER_VALIDATE_BOOLEAN returning null for some inputs.
🧹 Nitpick comments (2)
docs/plugins.md (2)
84-102: Clarify the purpose and usage ofgetImplementation()return value.The method returns a Twig template string rather than HTML or documentation. Without context, it's unclear how this return value is intended to be used by the plugin system. Does the backend render this as a template, display it as-is, or use it for documentation purposes?
Recommend adding a clarifying comment above this method or expanding the documentation section to explain the contract.
164-164: Good contextual note about Extended Description and Implementation fields.The note effectively ties the example code (particularly the
getAdvDescription()andgetImplementation()methods) to their backend UI manifestation. However, consider moving or restating this context earlier—either in the method documentation or immediately after introducing the new getter methods—so readers understand their purpose before encountering the full implementation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/plugins.mdphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.phpphpmyfaq/src/phpMyFAQ/Database/Mysqli.phpphpmyfaq/src/phpMyFAQ/Database/Sqlite3.phpphpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php
🧰 Additional context used
📓 Path-based instructions (1)
**/phpmyfaq/src/**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
Use PER Coding Style 3.0 for PHP code
Files:
phpmyfaq/src/phpMyFAQ/Database/Sqlite3.phpphpmyfaq/src/phpMyFAQ/Database/Mysqli.phpphpmyfaq/src/phpMyFAQ/Plugin/PluginManager.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php
🧬 Code graph analysis (3)
phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php (4)
phpmyfaq/src/phpMyFAQ/Database/Mysqli.php (4)
__construct(35-39)fetchAll(44-53)fetchAll(167-187)execute(377-408)phpmyfaq/src/phpMyFAQ/Instance/Database/Sqlite3.php (1)
__construct(356-359)phpmyfaq/src/phpMyFAQ/Database/DatabaseDriver.php (2)
fetchAll(77-77)execute(157-157)phpmyfaq/src/phpMyFAQ/Database/PdoSqlite.php (2)
fetchAll(118-125)execute(327-330)
phpmyfaq/src/phpMyFAQ/Database/Mysqli.php (3)
phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php (6)
__construct(30-34)fetchAll(39-48)fetchAll(140-160)prepare(349-356)query(207-222)execute(365-392)phpmyfaq/src/phpMyFAQ/Database/DatabaseDriver.php (4)
fetchAll(77-77)prepare(148-148)query(50-50)execute(157-157)phpmyfaq/src/phpMyFAQ/Database/PdoMysql.php (4)
fetchAll(117-124)prepare(291-294)query(264-283)execute(302-305)
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php (2)
phpmyfaq/src/phpMyFAQ/Session/Token.php (2)
Token(27-219)verifyToken(132-162)phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (5)
loadPlugins(87-208)getPlugins(341-344)activatePlugin(213-216)deactivatePlugin(221-224)savePluginConfig(237-258)
🪛 GitHub Actions: phpMyFAQ Build
phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php
[error] 377-377: mago format: formatting changes detected in file.
phpmyfaq/src/phpMyFAQ/Database/Mysqli.php
[error] 1-1: Failed to parse file: Unexpected token 'If'.
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php
[error] 146-146: mago format: formatting changes detected in file.
[error] 194-194: mago format: formatting changes detected in file.
[error] 290-290: mago format: formatting changes detected in file.
🪛 PHPMD (2.15.0)
phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php
349-349: Avoid unused parameters such as '$options'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (14)
docs/plugins.md (2)
12-12: Good addition of activation/deactivation workflow.The addition clearly documents that plugins must be activated in the backend and can be toggled without complete uninstallation, which aligns well with the PR objectives for extending the plugin system's configuration and activation capabilities.
15-32: Configuration section is well-structured.The new section 9.2 clearly explains the
PluginConfigurationInterfaceand provides a concrete example using PHP 8's Constructor Property Promotion. The explanation of how predefined values form the basis for backend configuration options is helpful. Note: Line 33 has a minor spacing issue (flagged separately).phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php (2)
25-49: LGTM!The
Sqlite3Statementwrapper class follows the same pattern as other database driver statement wrappers (e.g.,MysqliStatement,PgsqlStatement). The use of promoted properties is clean, and the deprecation notice properly guides users to the standardizedDatabaseDriver::fetchAll()method.
342-356: LGTM!The
prepare()method correctly wraps the native SQLite3 prepare functionality and returns theSqlite3Statementwrapper. The$optionsparameter is required by theDatabaseDriverinterface contract, so the PHPMD unused parameter warning is a false positive.phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php (1)
34-34: Verify CSRF token retrieval inconsistency.The two endpoints handle CSRF tokens differently:
toggleStatus(Line 34): retrieves token from header onlysaveConfig(Line 81): retrieves token from payload with header fallbackEnsure this difference is intentional and aligns with your security requirements.
Also applies to: 81-81
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (6)
55-60: LGTM!Clean dependency injection using constructor property promotion. The Configuration dependency is properly injected as a readonly property.
89-91: LGTM!Good optimization to prevent redundant plugin loading.
171-195: LGTM!Excellent use of
method_existsguards to handle optional plugin methods gracefully. This defensive approach prevents runtime errors when plugins don't implement all optional features.
237-258: LGTM!The database methods are well-implemented:
- Proper use of parameterized queries to prevent SQL injection
- Good error handling in
getPluginsFromDatabasefor missing table during installation- Consistent check-then-upsert pattern in
savePluginConfigandupdatePluginStatus- Correct table prefix usage
Also applies to: 263-284, 289-310
405-422: LGTM!Excellent path security implementation in both
registerPluginStylesheetsandregisterPluginScripts:
- Path normalization handles Windows and Unix separators
- Case-insensitive validation for Windows compatibility
- Prevents path traversal by ensuring paths remain within plugin directory
- File existence verification before registration
Also applies to: 455-472
115-119: LGTM!Good error handling for invalid JSON config. The plugin continues loading without the corrupted config, and the error is logged for debugging.
phpmyfaq/src/phpMyFAQ/Database/Mysqli.php (3)
30-54: LGTM! Consistent implementation of the statement wrapper.The MysqliStatement class follows the same pattern as other drivers (e.g., Sqlite3Statement) and correctly wraps the prepared statement with an optional result. The deprecation notice on fetchAll() appropriately directs users to DatabaseDriver::fetchAll().
354-368: LGTM! Clean implementation of the prepare method.The method correctly wraps mysqli::prepare and returns the appropriate MysqliStatement wrapper. The unused
$optionsparameter maintains interface consistency across database drivers.
370-408: LGTM! Robust implementation of the execute method.The method correctly:
- Validates the statement type
- Infers parameter types (int, float, string, blob) for binding
- Executes the statement and conditionally stores the result
The implementation is consistent with the DatabaseDriver interface and handles edge cases appropriately.
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In @phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php:
- Line 82: The if statement in PluginController::... (line with
Token::getInstance($this->session)->verifyToken('admin-plugins', $csrfToken))
has the return statement on the same line causing a formatting/PER style
violation; split the control flow onto multiple lines: place the return new
JsonResponse([...], 403); on the next line, ensure the if uses braces or a
properly formatted single-line block per project style, and fix indentation to
match surrounding code so the Token::getInstance(...)->verifyToken check and the
JsonResponse return are clearly separated.
- Around line 71-83: In saveConfig(), move CSRF validation before full JSON
parsing: first retrieve the CSRF token from the request header (or, if the token
must be supplied in the JSON body, extract/validate only the token field from
the raw request content without json_decoding the entire payload) and call
Token::getInstance($this->session)->verifyToken('admin-plugins', $csrfToken);
only after the token is verified, proceed to call $request->getContent() and
json_decode() and validate the decoded array; mirror the approach used in
toggleStatus() where CSRF is verified prior to parsing to avoid processing
untrusted JSON.
In @phpmyfaq/src/phpMyFAQ/Database/Mysqli.php:
- Around line 378-391: The type-inference must explicitly handle nulls, booleans
and unsupported types before calling $statement->stmt->bind_param: convert
booleans to integers (1/0) and use type 'i'; leave null values as PHP null and
assign them a safe bind type (e.g., 's' or keep as null-aware binding) rather
than 'b'; and reject or serialize arrays/objects (throw an exception or convert
to string) instead of falling back to 'b'. Update the loop that builds $types
and the corresponding $params normalization so nulls and booleans are normalized
and unsupported types are validated prior to invoking bind_param on
$statement->stmt.
In @phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php:
- Around line 351-376: Remove the duplicate/incorrect docblock above the prepare
method so only the correct docblock remains (the one that documents @return
SqlsrvStatement|false); keep the second docblock and delete the first
resource|string|false block, and update the remaining docblock to mention that
the $options parameter is currently unused for interface compatibility (or add a
brief @param note) so it’s clear why $options is present in prepare(string
$query, array $options = []) while the implementation passes options to
sqlsrv_prepare.
In @phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php:
- Around line 201-203: Remove the TODO-style exploratory comments in
PluginManager where incompatible plugins are handled and make the code comment
concise and production-ready: replace the two informal lines around the unset
call with a single clear comment such as "Remove incompatible plugin from the
plugins list" and keep the existing unset($this->plugins[$plugin->getName()]);
call unchanged so the behavior and references to $this->plugins,
$plugin->getName() and unset remain explicit and documented.
- Around line 165-166: Remove the stale inline comment "Apply DB config
overrides here if possible" in the PluginManager class; it is misleading because
DB config overrides are already applied earlier (see the earlier block where the
DB config is merged/applied), so delete that comment near the plugin
initialization/DB logic to avoid confusion for future maintainers.
- Around line 149-152: Replace the informal comment that reads "I will default
to inactive (false) for new plugins found on disk but not in DB." with a more
professional phrasing such as "Default to inactive (false) for new plugins found
on disk but not in DB." near the $isActive assignment in the PluginManager class
so the comment remains clear and formal without changing the logic that sets
$isActive = false.
- Around line 114-147: The code silently proceeds with invalid JSON and uses
heavy reflection for type coercion; update the block handling json_decode so
that if json_last_error() !== JSON_ERROR_NONE you log the error with
json_last_error_msg() and immediately skip applying that config (continue to
next plugin) or explicitly reset to the plugin default config from
$plugin->getConfig(); then prefer an explicit deserialization path by checking
if the config class (get_class($plugin->getConfig())) exposes a static
fromArray(array $data): static and call that to build the config object, and
only if fromArray does not exist fall back to the current
ReflectionProperty-based coercion (ReflectionProperty, getType(),
ReflectionNamedType) as a last-resort fallback.
- Around line 170-172: Remove the unnecessary method_exists() guards around
interface methods in PluginManager: directly call
$plugin->getTranslationsPath(), $plugin->getStylesheets(), and
$plugin->getScripts() (remove the checks that wrap those calls at the locations
using method_exists). These methods are defined on PluginInterface, so eliminate
the defensive conditionals and invoke the methods directly on the $plugin
instance (retaining any existing handling of the returned values).
🧹 Nitpick comments (9)
phpmyfaq/src/phpMyFAQ/Database/Pgsql.php (1)
361-376: Consider documenting the unused$optionsparameter.The
$optionsparameter is defined but never used in the implementation. If this is intentional for interface compatibility withDatabaseDriver, consider adding a comment to clarify.📝 Optional clarification comment
/** * Prepares a statement for execution and returns a statement object. * * @param string $query The SQL query - * @param array $options The driver options + * @param array $options The driver options (unused for PostgreSQL) * @return PgsqlStatement|false */ public function prepare(string $query, array $options = []): PgsqlStatement|falsephpmyfaq/src/phpMyFAQ/Database/Mysqli.php (2)
30-54: Consider removing the deprecated method from the new class.While the
MysqliStatementwrapper class serves its purpose, including a deprecatedfetchAll()method in a newly introduced class is unusual. Since the mainMysqliclass already handles this viafetchAll(mixed $result)(lines 169-171), consider removing this method to avoid confusion and reduce maintenance burden.♻️ Proposed refactor
class MysqliStatement { public function __construct( public \mysqli_stmt $stmt, public mixed $result = null, ) { } - - /** - * @deprecated Use DatabaseDriver::fetchAll() instead. - */ - public function fetchAll(): array - { - $ret = []; - if ($this->result) { - while ($row = $this->result->fetch_object()) { - $ret[] = $row; - } - } - return $ret; - } }
349-363: Consider error handling consistency.The
$optionsparameter is unused (though likely required by theDatabaseDriverinterface). More importantly, this method returnsfalseon failure without logging errors or throwing exceptions, which differs from thequery()method (lines 336-339) that throwsmysqli_sql_exception. Consider whether this inconsistency in error handling is intentional.phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php (2)
59-63: Consider error handling for plugin activation/deactivation.The calls to
activatePlugin()anddeactivatePlugin()are not wrapped in a try-catch block. If these methods encounter database errors or other exceptions, they will bubble up unhandled, resulting in a 500 error without a proper JSON response.🛡️ Proposed error handling
- if ($active) { - $pluginManager->activatePlugin($name); - } else { - $pluginManager->deactivatePlugin($name); - } - - return new JsonResponse(['success' => true]); + try { + if ($active) { + $pluginManager->activatePlugin($name); + } else { + $pluginManager->deactivatePlugin($name); + } + } catch (\Exception $e) { + return new JsonResponse([ + 'success' => false, + 'message' => 'Failed to update plugin status: ' . $e->getMessage(), + ], 500); + } + + return new JsonResponse(['success' => true]);
31-105: Consider extracting common patterns to reduce duplication.Both
toggleStatus()andsaveConfig()share similar patterns:
- PluginManager initialization and loading (lines 39-40, 85-86)
- JSON payload parsing and validation (lines 42-50, 71-79)
- Plugin existence validation (lines 52-57, 88-93)
Extracting these into private helper methods would improve maintainability and ensure consistent error handling across endpoints.
Example extraction:
private function parseJsonRequest(Request $request): array|JsonResponse { $content = $request->getContent(); $data = json_decode($content, true); if (json_last_error() !== JSON_ERROR_NONE || !is_array($data)) { return new JsonResponse([ 'success' => false, 'message' => 'Invalid JSON payload: ' . json_last_error_msg(), ], 400); } return $data; } private function validatePluginExists(string $pluginName, $pluginManager): ?JsonResponse { if (!array_key_exists($pluginName, $pluginManager->getPlugins())) { return new JsonResponse(['success' => false, 'message' => 'Plugin not found'], 404); } return null; }phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (4)
154-156: Remove redundant comment.Lines 154-156 state "Allow checking if it IS active. But we only REGISTER events and LOAD scripts if active." This comment is redundant with the immediately following code at line 157 (
if ($isActive && ...)). The code is self-documenting.
235-256: Consider transaction handling for database operations.The
savePluginConfig()andupdatePluginStatus()methods perform SELECT followed by conditional INSERT/UPDATE without transaction boundaries. If concurrent requests occur, this could lead to race conditions or inconsistent state (e.g., one request checks for existence while another is inserting).Additionally, the SELECT-then-INSERT/UPDATE pattern could be optimized using
INSERT ... ON DUPLICATE KEY UPDATE(MySQL) or equivalent upsert operations, reducing round trips.🔄 Example with transaction and upsert
public function savePluginConfig(string $pluginName, array $configData): void { $jsonConfig = json_encode($configData); $db = $this->configuration->getDb(); $table = \phpMyFAQ\Database::getTablePrefix() . 'faqplugins'; // Begin transaction $db->beginTransaction(); try { // Use INSERT ... ON DUPLICATE KEY UPDATE for MySQL // or equivalent UPSERT for other databases $upsert = sprintf( 'INSERT INTO %s (name, active, config) VALUES (?, 0, ?) ON DUPLICATE KEY UPDATE config = VALUES(config)', $table ); $stmt = $db->prepare($upsert); $db->execute($stmt, [$pluginName, $jsonConfig]); $db->commit(); } catch (\Exception $e) { $db->rollback(); throw $e; } }Note: Adjust the upsert syntax based on your database driver (PostgreSQL uses
ON CONFLICT, SQLite usesINSERT OR REPLACE).Also applies to: 261-282
294-294: Mixed query styles for consistency.Line 294 uses
$db->query()for a SELECT statement, while other database operations in this class use$db->prepare()with$db->execute(). While this specific query has no parameters and is safe, maintaining consistent style throughout the class improves readability.- $result = $db->query("SELECT name, active, config FROM $table"); + $select = sprintf('SELECT name, active, config FROM %s', $table); + $stmt = $db->prepare($select); + $db->execute($stmt); + $result = $stmt;
398-470: Consider consolidating duplicate asset registration logic.The
registerPluginStylesheets()andregisterPluginScripts()methods have nearly identical implementations. Consider extracting the common path validation logic into a private helper method to reduce duplication and ensure consistent security checks.♻️ Example consolidation
private function validatePluginAssets(string $pluginName, array $assets, string $type): array { $pluginDir = $this->getPluginDirectory($pluginName); $validated = []; $realPluginDir = str_replace('\\', '/', realpath($pluginDir) ?: $pluginDir); $realPluginDir = rtrim($realPluginDir, '/') . '/'; foreach ($assets as $asset) { $fullPath = $pluginDir . '/' . $asset; $absolutePath = str_replace('\\', '/', realpath($fullPath) ?: $fullPath); if (str_starts_with(strtolower($absolutePath), strtolower($realPluginDir)) && file_exists($fullPath)) { $validated[] = 'content/plugins/' . $pluginName . '/' . $asset; } } return $validated; } private function registerPluginStylesheets(string $pluginName, array $stylesheets): void { $validated = $this->validatePluginAssets($pluginName, $stylesheets, 'stylesheet'); if ($validated !== []) { $this->pluginStylesheets[$pluginName] = $validated; } } private function registerPluginScripts(string $pluginName, array $scripts): void { $validated = $this->validatePluginAssets($pluginName, $scripts, 'script'); if ($validated !== []) { $this->pluginScripts[$pluginName] = $validated; } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/plugins.mdphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.phpphpmyfaq/src/phpMyFAQ/Database/Mysqli.phpphpmyfaq/src/phpMyFAQ/Database/Pgsql.phpphpmyfaq/src/phpMyFAQ/Database/Sqlite3.phpphpmyfaq/src/phpMyFAQ/Database/Sqlsrv.phpphpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php
🧰 Additional context used
📓 Path-based instructions (1)
**/phpmyfaq/src/**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
Use PER Coding Style 3.0 for PHP code
Files:
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.phpphpmyfaq/src/phpMyFAQ/Database/Mysqli.phpphpmyfaq/src/phpMyFAQ/Database/Sqlsrv.phpphpmyfaq/src/phpMyFAQ/Database/Pgsql.phpphpmyfaq/src/phpMyFAQ/Plugin/PluginManager.phpphpmyfaq/src/phpMyFAQ/Database/Sqlite3.php
🧠 Learnings (1)
📚 Learning: 2025-12-26T21:59:16.059Z
Learnt from: CR
Repo: thorsten/phpMyFAQ PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-26T21:59:16.059Z
Learning: Applies to **/phpmyfaq/src/**/*.php : Use PER Coding Style 3.0 for PHP code
Applied to files:
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.phpphpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php
🧬 Code graph analysis (3)
phpmyfaq/src/phpMyFAQ/Database/Mysqli.php (1)
phpmyfaq/src/phpMyFAQ/Database/DatabaseDriver.php (4)
fetchAll(77-77)prepare(148-148)query(50-50)execute(157-157)
phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php (3)
phpmyfaq/src/phpMyFAQ/Instance/Database/Pgsql.php (1)
__construct(358-361)phpmyfaq/src/phpMyFAQ/Instance/Database/Sqlsrv.php (1)
__construct(356-359)phpmyfaq/src/phpMyFAQ/Database/DatabaseDriver.php (3)
fetchAll(77-77)prepare(148-148)query(50-50)
phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php (6)
phpmyfaq/src/phpMyFAQ/Database/Mysqli.php (7)
__construct(35-39)fetchAll(44-53)fetchAll(167-183)fetchArray(148-151)prepare(356-363)query(328-347)execute(372-403)phpmyfaq/src/phpMyFAQ/Database/Pgsql.php (7)
__construct(35-39)fetchAll(49-58)fetchAll(182-198)fetchArray(255-263)prepare(368-376)query(139-158)execute(385-395)phpmyfaq/src/phpMyFAQ/Instance/Database/Sqlite3.php (1)
__construct(356-359)phpmyfaq/src/phpMyFAQ/Database/DatabaseDriver.php (5)
fetchAll(77-77)fetchArray(67-67)prepare(148-148)query(50-50)execute(157-157)phpmyfaq/src/phpMyFAQ/Database/PdoMysql.php (5)
fetchAll(117-124)fetchArray(98-101)prepare(291-294)query(264-283)execute(302-305)phpmyfaq/src/phpMyFAQ/Database/PdoSqlite.php (5)
fetchAll(118-125)fetchArray(98-102)prepare(316-319)query(289-308)execute(327-330)
🪛 GitHub Actions: phpMyFAQ Build
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php
[error] 79-79: mago format --dry-run detected formatting differences. Exit code 1. Run 'mago format' to apply changes to this file.
🪛 PHPMD (2.15.0)
phpmyfaq/src/phpMyFAQ/Database/Mysqli.php
356-356: Avoid unused parameters such as '$options'. (undefined)
(UnusedFormalParameter)
phpmyfaq/src/phpMyFAQ/Database/Pgsql.php
368-368: Avoid unused parameters such as '$options'. (undefined)
(UnusedFormalParameter)
phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php
344-344: Avoid unused parameters such as '$options'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (20)
phpmyfaq/src/phpMyFAQ/Database/Pgsql.php (5)
30-59: LGTM! Clean statement wrapper implementation.The
PgsqlStatementclass provides a clean wrapper for prepared statements with appropriate deprecation of the localfetchAll()method in favor of the driver-level method.
86-89: LGTM! Appropriate storage for prepared statements.The private array for tracking prepared statements is well-placed and supports the new prepared statement functionality.
184-186: LGTM! Consistent unwrapping pattern.The unwrapping logic correctly extracts the underlying result from
PgsqlStatementinstances, consistent with the pattern used in other database drivers.
357-357: LGTM! Plugin table added.The addition of
faqpluginsto the table enumeration aligns with the PR's plugin system enhancement.
378-395: LGTM! Clean prepared statement execution.The
execute()method correctly:
- Casts the statement to string via
__toString()- Executes with
pg_execute()- Assigns the result back to the statement object
- Returns a boolean status
phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php (4)
27-56: LGTM! Robust statement wrapper.The
SqlsrvStatementclass appropriately handles both resource and string statement types with defensive__toString()logic.
147-149: LGTM! Consistent unwrapping pattern.The unwrapping logic matches the pattern used in other database drivers.
347-347: LGTM! Plugin table added.Consistent with the plugin system enhancement across all database drivers.
378-408: LGTM! Dual-mode execution logic is sound.The
execute()method appropriately handles both execution modes:
- Resource-based prepared statements (lines 391-396)
- String-based parameterized queries (lines 399-405)
The type guard at lines 387-389 ensures only
SqlsrvStatementinstances are processed.phpmyfaq/src/phpMyFAQ/Database/Mysqli.php (2)
169-171: LGTM! Good backward compatibility approach.The unwrapping of
MysqliStatementallows the new prepared statement API to integrate seamlessly with the existingfetchAll()interface.
285-285: LGTM!The addition of the
faqpluginstable aligns with the plugin system enhancement.phpmyfaq/src/phpMyFAQ/Database/Sqlite3.php (5)
25-49: LGTM! Clean statement wrapper implementation.The Sqlite3Statement class appropriately wraps SQLite3Stmt and provides a clear migration path via the deprecated fetchAll() method.
140-156: Proper handling of statement wrapper.The updated fetchAll() correctly unwraps Sqlite3Statement instances to access the underlying result.
303-303: LGTM! Plugin table added.The addition of the faqplugins table aligns with the PR's plugin system enhancement.
337-351: Correct interface implementation; PHPMD warning is a false positive.The unused
$optionsparameter is required for DatabaseDriver interface compliance. The native SQLite3::prepare() doesn't accept options, so this parameter cannot be used.
353-387: Code implementation is correct for actual usage patterns in the codebase.The concern about associative array keys is theoretical. All callers in PluginManager.php (lines 244, 250, 254, 270, 281) pass numeric arrays constructed with square bracket syntax. The
foreach ($params as $index => $param)pattern with$index + 1binding correctly handles positional parameter binding, which is the standard approach across all database driver implementations (Sqlite3.php, Pgsql.php, Mysqli.php). No changes needed.docs/plugins.md (1)
12-12: Documentation enhancements look good!The documentation updates clearly explain the new plugin activation and configuration features. The example code with the new getter methods (getDescription, getAuthor, getAdvDescription, getImplementation) provides helpful guidance for plugin developers.
Also applies to: 21-21, 33-33, 84-104, 166-166
phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php (3)
55-60: Good dependency injection pattern.The constructor now properly accepts the
Configurationdependency, which enables database operations for plugin state management. The use ofreadonlyensures immutability.
89-91: LGTM: Guard against multiple invocations.The early return prevents duplicate plugin loading if
loadPlugins()is called multiple times, which is good defensive programming.
398-420: Appreciate the path security measures.The path normalization and validation logic (lines 403-413 and 453-463) properly guards against path traversal attacks and handles Windows case-insensitivity. The security checks ensure assets remain within plugin directories.
Also applies to: 448-470
| $content = $request->getContent(); | ||
| $data = json_decode($content, true); | ||
|
|
||
| if (json_last_error() !== JSON_ERROR_NONE || !is_array($data)) { | ||
| return new JsonResponse([ | ||
| 'success' => false, | ||
| 'message' => 'Invalid JSON payload: ' . json_last_error_msg(), | ||
| ], 400); | ||
| } | ||
|
|
||
| $csrfToken = $data['csrf'] ?? $request->headers->get('X-CSRF-Token'); | ||
| if (!Token::getInstance($this->session)->verifyToken('admin-plugins', $csrfToken)) { return new JsonResponse(['success' => false, 'message' => 'Invalid CSRF token'], 403); | ||
| } |
There was a problem hiding this comment.
Security: CSRF validation should occur before JSON parsing.
In saveConfig(), JSON parsing happens at lines 71-79 before CSRF validation at lines 81-83. This differs from toggleStatus(), where CSRF validation occurs first (line 34) before JSON parsing (line 42).
Parsing untrusted JSON before validating CSRF allows attackers to potentially exploit JSON parsing vulnerabilities or trigger denial-of-service through malformed payloads without a valid CSRF token.
🔒 Proposed fix
public function saveConfig(Request $request): JsonResponse
{
+ $csrfToken = $request->headers->get('X-CSRF-Token');
+ if (!Token::getInstance($this->session)->verifyToken('admin-plugins', $csrfToken)) {
+ return new JsonResponse(['success' => false, 'message' => 'Invalid CSRF token'], 403);
+ }
+
$content = $request->getContent();
$data = json_decode($content, true);
if (json_last_error() !== JSON_ERROR_NONE || !is_array($data)) {
return new JsonResponse([
'success' => false,
'message' => 'Invalid JSON payload: ' . json_last_error_msg(),
], 400);
}
- $csrfToken = $data['csrf'] ?? $request->headers->get('X-CSRF-Token');
- if (!Token::getInstance($this->session)->verifyToken('admin-plugins', $csrfToken)) {
- return new JsonResponse(['success' => false, 'message' => 'Invalid CSRF token'], 403);
- }
-
$pluginManager = $this->container->get(id: 'phpmyfaq.plugin.plugin-manager');Note: If the CSRF token must come from the JSON payload for specific reasons, validate it exists before parsing the entire payload.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $content = $request->getContent(); | |
| $data = json_decode($content, true); | |
| if (json_last_error() !== JSON_ERROR_NONE || !is_array($data)) { | |
| return new JsonResponse([ | |
| 'success' => false, | |
| 'message' => 'Invalid JSON payload: ' . json_last_error_msg(), | |
| ], 400); | |
| } | |
| $csrfToken = $data['csrf'] ?? $request->headers->get('X-CSRF-Token'); | |
| if (!Token::getInstance($this->session)->verifyToken('admin-plugins', $csrfToken)) { return new JsonResponse(['success' => false, 'message' => 'Invalid CSRF token'], 403); | |
| } | |
| $csrfToken = $request->headers->get('X-CSRF-Token'); | |
| if (!Token::getInstance($this->session)->verifyToken('admin-plugins', $csrfToken)) { | |
| return new JsonResponse(['success' => false, 'message' => 'Invalid CSRF token'], 403); | |
| } | |
| $content = $request->getContent(); | |
| $data = json_decode($content, true); | |
| if (json_last_error() !== JSON_ERROR_NONE || !is_array($data)) { | |
| return new JsonResponse([ | |
| 'success' => false, | |
| 'message' => 'Invalid JSON payload: ' . json_last_error_msg(), | |
| ], 400); | |
| } |
🧰 Tools
🪛 GitHub Actions: phpMyFAQ Build
[error] 79-79: mago format --dry-run detected formatting differences. Exit code 1. Run 'mago format' to apply changes to this file.
🤖 Prompt for AI Agents
In @phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php
around lines 71 - 83, In saveConfig(), move CSRF validation before full JSON
parsing: first retrieve the CSRF token from the request header (or, if the token
must be supplied in the JSON body, extract/validate only the token field from
the raw request content without json_decoding the entire payload) and call
Token::getInstance($this->session)->verifyToken('admin-plugins', $csrfToken);
only after the token is verified, proceed to call $request->getContent() and
json_decode() and validate the decoded array; mirror the approach used in
toggleStatus() where CSRF is verified prior to parsing to avoid processing
untrusted JSON.
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/PluginController.php
Show resolved
Hide resolved
| if (!empty($params)) { | ||
| $types = ''; | ||
| foreach ($params as $param) { | ||
| if (is_int($param)) { | ||
| $types .= 'i'; | ||
| } elseif (is_float($param)) { | ||
| $types .= 'd'; | ||
| } elseif (is_string($param)) { | ||
| $types .= 's'; | ||
| } else { | ||
| $types .= 'b'; | ||
| } | ||
| } | ||
| $statement->stmt->bind_param($types, ...$params); |
There was a problem hiding this comment.
Handle null and boolean parameter types explicitly.
The type inference logic treats all non-int/float/string values as blob ('b'), which is problematic:
- null values: MySQLi doesn't have a specific null type for
bind_param(). Binding null as blob will not work correctly. Null parameters typically need special handling (e.g., converted to string 'NULL' or handled via nullable type binding). - boolean values: Binding booleans as blob could lead to unexpected behavior. Consider converting to integer (1/0) or string.
- Other types (arrays, objects): Would fail when bound as blob.
This could cause data integrity issues or runtime errors when these types are passed as parameters.
🔧 Proposed fix
if (!empty($params)) {
$types = '';
foreach ($params as $param) {
- if (is_int($param)) {
+ if (is_null($param)) {
+ $types .= 's'; // bind null as empty string or handle separately
+ } elseif (is_bool($param)) {
+ $types .= 'i'; // bind boolean as integer (0 or 1)
+ } elseif (is_int($param)) {
$types .= 'i';
} elseif (is_float($param)) {
$types .= 'd';
} elseif (is_string($param)) {
$types .= 's';
} else {
$types .= 'b';
}
}
+ // Convert booleans to integers for binding
+ $params = array_map(fn($p) => is_bool($p) ? (int)$p : $p, $params);
$statement->stmt->bind_param($types, ...$params);
}Note: Null handling may require additional logic depending on your use case. Consider validating parameter types before binding or documenting expected types.
🤖 Prompt for AI Agents
In @phpmyfaq/src/phpMyFAQ/Database/Mysqli.php around lines 378 - 391, The
type-inference must explicitly handle nulls, booleans and unsupported types
before calling $statement->stmt->bind_param: convert booleans to integers (1/0)
and use type 'i'; leave null values as PHP null and assign them a safe bind type
(e.g., 's' or keep as null-aware binding) rather than 'b'; and reject or
serialize arrays/objects (throw an exception or convert to string) instead of
falling back to 'b'. Update the loop that builds $types and the corresponding
$params normalization so nulls and booleans are normalized and unsupported types
are validated prior to invoking bind_param on $statement->stmt.
| /** | ||
| * Prepares a statement for execution and returns a statement object. | ||
| * | ||
| * @param string $query The SQL query | ||
| * @param array $options The driver options | ||
| * @return resource|string|false | ||
| */ | ||
| /** | ||
| * Prepares a statement for execution and returns a statement object. | ||
| * | ||
| * @param string $query The SQL query | ||
| * @param array $options The driver options | ||
| * @return SqlsrvStatement|false | ||
| */ | ||
| public function prepare(string $query, array $options = []): SqlsrvStatement|false | ||
| { | ||
| if (str_contains($query, '?')) { | ||
| return new SqlsrvStatement($query); | ||
| } | ||
|
|
||
| $stmt = sqlsrv_prepare($this->conn, $query, [], $options); | ||
| if ($stmt) { | ||
| return new SqlsrvStatement($stmt); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Remove duplicate docblock.
Lines 351-357 contain a duplicate and inconsistent docblock. The first docblock incorrectly specifies the return type as @return resource|string|false while the correct one at lines 358-364 specifies @return SqlsrvStatement|false.
🧹 Proposed fix to remove duplicate docblock
- /**
- * Prepares a statement for execution and returns a statement object.
- *
- * @param string $query The SQL query
- * @param array $options The driver options
- * @return resource|string|false
- */
/**
* Prepares a statement for execution and returns a statement object.
*
* @param string $query The SQL query
* @param array $options The driver options
* @return SqlsrvStatement|false
*/
public function prepare(string $query, array $options = []): SqlsrvStatement|falseAdditionally, like in Pgsql.php, the $options parameter is unused. If this is for interface compatibility, consider documenting it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Prepares a statement for execution and returns a statement object. | |
| * | |
| * @param string $query The SQL query | |
| * @param array $options The driver options | |
| * @return resource|string|false | |
| */ | |
| /** | |
| * Prepares a statement for execution and returns a statement object. | |
| * | |
| * @param string $query The SQL query | |
| * @param array $options The driver options | |
| * @return SqlsrvStatement|false | |
| */ | |
| public function prepare(string $query, array $options = []): SqlsrvStatement|false | |
| { | |
| if (str_contains($query, '?')) { | |
| return new SqlsrvStatement($query); | |
| } | |
| $stmt = sqlsrv_prepare($this->conn, $query, [], $options); | |
| if ($stmt) { | |
| return new SqlsrvStatement($stmt); | |
| } | |
| return false; | |
| } | |
| /** | |
| * Prepares a statement for execution and returns a statement object. | |
| * | |
| * @param string $query The SQL query | |
| * @param array $options The driver options | |
| * @return SqlsrvStatement|false | |
| */ | |
| public function prepare(string $query, array $options = []): SqlsrvStatement|false | |
| { | |
| if (str_contains($query, '?')) { | |
| return new SqlsrvStatement($query); | |
| } | |
| $stmt = sqlsrv_prepare($this->conn, $query, [], $options); | |
| if ($stmt) { | |
| return new SqlsrvStatement($stmt); | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In @phpmyfaq/src/phpMyFAQ/Database/Sqlsrv.php around lines 351 - 376, Remove the
duplicate/incorrect docblock above the prepare method so only the correct
docblock remains (the one that documents @return SqlsrvStatement|false); keep
the second docblock and delete the first resource|string|false block, and update
the remaining docblock to mention that the $options parameter is currently
unused for interface compatibility (or add a brief @param note) so it’s clear
why $options is present in prepare(string $query, array $options = []) while the
implementation passes options to sqlsrv_prepare.
| if (!empty($dbPlugins[$pluginName]['config']) && $plugin->getConfig()) { | ||
| $configArray = json_decode($dbPlugins[$pluginName]['config'], true); | ||
| if (json_last_error() !== JSON_ERROR_NONE) { | ||
| error_log("Failed to decode config for plugin {$pluginName}: " . json_last_error_msg()); | ||
| continue; | ||
| } | ||
| if (is_array($configArray)) { | ||
| $configObject = $plugin->getConfig(); | ||
| foreach ($configArray as $key => $value) { | ||
| if (property_exists($configObject, $key)) { | ||
| try { | ||
| $rp = new \ReflectionProperty($configObject, $key); | ||
| if ($type = $rp->getType()) { | ||
| $typeName = $type instanceof \ReflectionNamedType ? $type->getName() : null; | ||
| switch ($typeName) { | ||
| case 'int': | ||
| $value = (int) $value; | ||
| break; | ||
| case 'bool': | ||
| $value = (bool) $value; | ||
| break; | ||
| case 'float': | ||
| $value = (float) $value; | ||
| break; | ||
| } | ||
| } | ||
| } catch (\ReflectionException $e) { | ||
| // Fallback to direct assignment if reflection fails | ||
| } | ||
| $configObject->$key = $value; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Complex configuration merging logic with silent error handling.
The configuration merging logic (lines 114-147) is quite complex, using reflection for type coercion. Two concerns:
-
Silent failure: Line 117-118 logs JSON decode errors but continues execution with an invalid config. This could lead to unexpected behavior when the plugin expects valid configuration.
-
Complexity: The reflection-based type coercion adds cognitive load and performance overhead. Consider whether this dynamic approach is necessary, or if a more explicit deserialization pattern would be clearer.
💡 Alternative approach
Consider having plugin configuration classes implement a fromArray(array $data): static method for controlled deserialization:
if (!empty($dbPlugins[$pluginName]['config']) && $plugin->getConfig()) {
$configArray = json_decode($dbPlugins[$pluginName]['config'], true);
if (json_last_error() !== JSON_ERROR_NONE) {
error_log("Failed to decode config for plugin {$pluginName}: " . json_last_error_msg());
continue; // Skip this plugin or use default config
}
if (is_array($configArray)) {
$configClass = get_class($plugin->getConfig());
if (method_exists($configClass, 'fromArray')) {
$configObject = $configClass::fromArray($configArray);
// Use the properly deserialized config
}
}
}This pushes the responsibility of type conversion to the plugin configuration class, making it explicit and testable.
🤖 Prompt for AI Agents
In @phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php around lines 114 - 147, The
code silently proceeds with invalid JSON and uses heavy reflection for type
coercion; update the block handling json_decode so that if json_last_error() !==
JSON_ERROR_NONE you log the error with json_last_error_msg() and immediately
skip applying that config (continue to next plugin) or explicitly reset to the
plugin default config from $plugin->getConfig(); then prefer an explicit
deserialization path by checking if the config class
(get_class($plugin->getConfig())) exposes a static fromArray(array $data):
static and call that to build the config object, and only if fromArray does not
exist fall back to the current ReflectionProperty-based coercion
(ReflectionProperty, getType(), ReflectionNamedType) as a last-resort fallback.
| // I will default to inactive (false) for new plugins found on disk but not in DB. | ||
|
|
||
| $isActive = false; | ||
| } |
There was a problem hiding this comment.
Improve comment professionalism.
Line 149 uses the personal pronoun "I will default to inactive" which is unprofessional in production code. Consider rephrasing to: "Default to inactive (false) for new plugins found on disk but not in DB."
🤖 Prompt for AI Agents
In @phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php around lines 149 - 152,
Replace the informal comment that reads "I will default to inactive (false) for
new plugins found on disk but not in DB." with a more professional phrasing such
as "Default to inactive (false) for new plugins found on disk but not in DB."
near the $isActive assignment in the PluginManager class so the comment remains
clear and formal without changing the logic that sets $isActive = false.
| // Apply DB config overrides here if possible | ||
| } |
There was a problem hiding this comment.
Remove stale comment.
The comment "Apply DB config overrides here if possible" (lines 165-166) is stale. DB config overrides are already applied earlier at lines 114-147. This could confuse future maintainers.
🤖 Prompt for AI Agents
In @phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php around lines 165 - 166,
Remove the stale inline comment "Apply DB config overrides here if possible" in
the PluginManager class; it is misleading because DB config overrides are
already applied earlier (see the earlier block where the DB config is
merged/applied), so delete that comment near the plugin initialization/DB logic
to avoid confusion for future maintainers.
| if (method_exists($plugin, 'getTranslationsPath')) { | ||
| $translationsPath = $plugin->getTranslationsPath(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the PluginInterface definition
fd -type f -name "*.php" | xargs rg "interface PluginInterface" -lRepository: thorsten/phpMyFAQ
Length of output: 297
🏁 Script executed:
# Once we find the interface, examine it
rg "interface PluginInterface" -A 50 --type phpRepository: thorsten/phpMyFAQ
Length of output: 3890
🏁 Script executed:
# Also check the PluginManager file to see the full context
cat -n phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php | head -250 | tail -150Repository: thorsten/phpMyFAQ
Length of output: 7579
🏁 Script executed:
cat -n phpmyfaq/src/phpMyFAQ/Plugin/PluginInterface.phpRepository: thorsten/phpMyFAQ
Length of output: 2976
Remove unnecessary method_exists() guards for interface methods.
Lines 170-172, 187-189, and 192-194 use method_exists() to guard calls to getTranslationsPath(), getStylesheets(), and getScripts(). All three methods are defined in PluginInterface and are part of the contract that any plugin must implement. These guards are unnecessary and add defensive code clutter. Remove the checks and call the methods directly since the interface guarantees their presence.
🤖 Prompt for AI Agents
In @phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php around lines 170 - 172,
Remove the unnecessary method_exists() guards around interface methods in
PluginManager: directly call $plugin->getTranslationsPath(),
$plugin->getStylesheets(), and $plugin->getScripts() (remove the checks that
wrap those calls at the locations using method_exists). These methods are
defined on PluginInterface, so eliminate the defensive conditionals and invoke
the methods directly on the $plugin instance (retaining any existing handling of
the returned values).
| // Remove plugin from the plugins array since it's incompatible (actually, maybe keep it but mark inactive?) | ||
| // Original code removed it. I'll stick to original behavior for incompatible ones. | ||
| unset($this->plugins[$plugin->getName()]); |
There was a problem hiding this comment.
Clean up TODO-style comments.
Lines 201-202 contain exploratory comments ("actually, maybe keep it but mark inactive?" and "I'll stick to original behavior") that read like internal notes during development. These should be resolved or removed before merging to production.
🤖 Prompt for AI Agents
In @phpmyfaq/src/phpMyFAQ/Plugin/PluginManager.php around lines 201 - 203,
Remove the TODO-style exploratory comments in PluginManager where incompatible
plugins are handled and make the code comment concise and production-ready:
replace the two informal lines around the unset call with a single clear comment
such as "Remove incompatible plugin from the plugins list" and keep the existing
unset($this->plugins[$plugin->getName()]); call unchanged so the behavior and
references to $this->plugins, $plugin->getName() and unset remain explicit and
documented.
2 participants
Feature: Extension of the plugin system to include configuration and activation options
Summary by CodeRabbit
New Features
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.