Open
Conversation
📝 WalkthroughWalkthroughReplaces the legacy Application bootstrap with a Symfony Kernel-based HTTP lifecycle across all entry points, adds two Symfony dev dependencies, introduces Kernel, event listeners, controller DI/resolver changes, moves language/exception handling into listeners, removes Application and its tests, and updates/adds many controller implementations and tests. Changes
Sequence DiagramssequenceDiagram
participant Client as Client
participant Entry as EntryPoint
participant Kernel as Kernel
participant Dispatcher as EventDispatcher
participant Router as RouterListener
participant Lang as LanguageListener
participant ControllerInjector as ControllerContainerListener
participant Resolver as ContainerControllerResolver
participant Controller as Controller
participant ApiEx as ApiExceptionListener
participant WebEx as WebExceptionListener
participant Response as Response
Client->>Entry: HTTP request
Entry->>Kernel: new Kernel(routingContext, debug)
Entry->>Kernel: handle(Request)
activate Kernel
Kernel->>Dispatcher: dispatch KernelEvents::REQUEST
Dispatcher->>Router: RouterListener handles request
Router-->>Dispatcher: sets _controller/_route attrs (if matched)
Dispatcher->>Lang: LanguageListener initializes translations
Lang-->>Dispatcher: translations ready
Dispatcher->>ControllerInjector: CONTROLLER event
ControllerInjector->>Controller: setContainer(...) (if AbstractController)
Kernel->>Resolver: resolve controller callable
Resolver-->>Kernel: callable
Kernel->>Controller: invoke callable
alt controller returns Response
Controller-->>Kernel: Response
else controller throws
Controller-->>Kernel: Exception
Kernel->>Dispatcher: dispatch KernelEvents::EXCEPTION
Dispatcher->>ApiEx: ApiExceptionListener (for API paths)
ApiEx-->>Kernel: ProblemDetails JSON Response
Dispatcher->>WebEx: WebExceptionListener (for web paths)
WebEx-->>Kernel: HTML/redirect Response
end
Kernel-->>Entry: Response
Entry->>Response: send()
deactivate Kernel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Controller/AbstractController.php`:
- Around line 100-116: The authorization check in initializeFromContainer() is
only run once due to the containerInitialized guard, causing isSecured() to use
the fallback container's user; update the flow so isSecured() is re-evaluated
when the kernel injects its container: either remove/adjust the
containerInitialized guard in initializeFromContainer() so isSecured() runs
after you refresh $this->configuration, $this->currentUser and $this->session,
or have ControllerContainerListener::setContainer() explicitly call the
controller's isSecured() (or reset containerInitialized there) after swapping
the container; reference initializeFromContainer, isSecured,
containerInitialized, ControllerContainerListener, setContainer and
phpmyfaq.user.current_user when making the change.
In `@phpmyfaq/src/phpMyFAQ/EventListener/LanguageListener.php`:
- Around line 100-102: In LanguageListener's catch block you currently discard
the original stack by doing throw new Exception($exception->getMessage());
instead either rethrow the original exception ($exception) or construct the new
Exception passing the original as the previous parameter (new
Exception($exception->getMessage(), 0, $exception)) so the original stack trace
and context are preserved; update the catch that references $exception in
LanguageListener.php accordingly.
In `@phpmyfaq/src/phpMyFAQ/EventListener/RouterListener.php`:
- Around line 49-54: The RouterListener currently calls UrlMatcher::match()
without handling its ResourceNotFoundException and MethodNotAllowedException;
wrap the match() call in a try/catch in RouterListener:: (where RequestContext,
UrlMatcher and $urlMatcher->match($request->getPathInfo()) are used), catch
ResourceNotFoundException and rethrow a
Symfony\Component\HttpKernel\Exception\NotFoundHttpException (passing the
original exception as the previous), and catch MethodNotAllowedException and
rethrow a Symfony\Component\HttpKernel\Exception\MethodNotAllowedHttpException
(including the allowed methods from the MethodNotAllowedException and set the
original as the previous) so routing errors are converted to proper HTTP
exceptions consumed by exception listeners.
In `@phpmyfaq/src/phpMyFAQ/EventListener/WebExceptionListener.php`:
- Line 53: The redirect for UnauthorizedHttpException in WebExceptionListener
currently uses a relative URL ('./login') which can resolve to incorrect nested
paths; update the RedirectResponse instantiation in the
UnauthorizedHttpException branch to use an absolute path (e.g., '/login') or
build the login path from the request base URL (use the request object available
in WebExceptionListener to compute baseUrl + '/login') so the redirect always
targets the intended login page.
In `@phpmyfaq/src/phpMyFAQ/Kernel.php`:
- Around line 117-121: The catch around $phpFileLoader->load(resource:
'services.php') currently swallows failures; update Kernel::boot (the try/catch
for $phpFileLoader->load) to fail fast by re-throwing the caught exception (or
wrap and throw a RuntimeException with the original as previous) so boot stops
with a clear error, or if you intentionally want soft-fail, implement the
alternative guard: ensure subsequent calls like
$this->container->has('phpmyfaq.event_dispatcher') before calling
$this->container->get(...) and handle the missing service path (return null or
throw a clear, contextual exception). Ensure the fix references the same loader
call ($phpFileLoader->load(resource: 'services.php')) and the downstream access
point ($this->container->get('phpmyfaq.event_dispatcher')) so the behavior is
consistent.
In `@tests/phpMyFAQ/Functional/KernelRoutingTest.php`:
- Around line 43-64: The createKernelStack method currently accepts an unused
$isApi parameter; either remove it or make it meaningful—preferably use it: when
$isApi is true, return a kernel wrapper that sets the _api_context request
attribute before delegating to the real HttpKernel so listeners like
ApiExceptionListener detect admin-API context; locate createKernelStack,
ApiExceptionListener and the HttpKernel instantiation and implement a small
delegating kernel (or a RequestStack pre-population) that injects _api_context
on incoming Request objects prior to calling HttpKernel::handle.
In `@tests/phpMyFAQ/Functional/WebTestCase.php`:
- Around line 109-119: The local variable $response assigned from
getInternalResponse() in getResponse() is unused and should be removed: delete
the call and the $response variable, leaving getResponse() to return the stored
$this->response or null as currently intended (or, if the intention was to
prefer the internal response, replace the early return logic to use the value
from getInternalResponse()); update getResponse() accordingly and ensure no
other code relies on the removed call to getInternalResponse().
🧹 Nitpick comments (12)
phpmyfaq/src/phpMyFAQ/EventListener/RouterListener.php (1)
44-47: Slightly misleading comment — sub-requests are already filtered above.Line 38 already returns early for sub-requests, so the "by sub-request" part of this comment is inaccurate. Consider rewording to something like
// Skip if already matched (e.g., controller pre-set in tests or by another listener).phpmyfaq/src/phpMyFAQ/EventListener/WebExceptionListener.php (1)
95-110:error_logfor server errors is functional but limited.For a Kernel-based architecture, consider injecting a PSR-3
LoggerInterface(e.g., Monolog, which is already a project dependency) for structured logging with context. This would enable log levels, handlers, and better production observability.tests/phpMyFAQ/EventListener/ControllerContainerListenerTest.php (1)
77-79: Consider$this->expectNotToPerformAssertions()instead ofassertTrue(true).PHPUnit provides
expectNotToPerformAssertions()to explicitly signal that a test's success criterion is "no exception thrown." It avoids the "risky test" warning more idiomatically.Proposed fix
- // Should not throw or error + // Should not throw or error — no assertions needed + $this->expectNotToPerformAssertions(); $listener->onKernelController($event); - $this->assertTrue(true);phpmyfaq/src/phpMyFAQ/EventListener/ApiExceptionListener.php (1)
89-93: Use imported short names instead of FQCNs for consistency.
Requestis not imported at the top of the file (onlyResponseis), andThrowableis used with the global prefix. For consistency withWebExceptionListenerand PER style, add the missing imports and use short names.Proposed fix
Add these to the import block:
use Symfony\Component\HttpFoundation\Request; use Throwable;Then update the method signature:
private function createProblemDetailsResponse( - \Symfony\Component\HttpFoundation\Request $request, + Request $request, int $status, - \Throwable $throwable, + Throwable $throwable, string $defaultDetail, ): Response {phpmyfaq/src/phpMyFAQ/Kernel.php (1)
132-149: Route caching has two overlapping debug-mode checks.Line 139 checks both
$this->debug(kernel flag) andEnvironment::isDebugMode()(global static). If these can diverge, the behavior is unclear. If they always agree, one is redundant. Consider unifying the debug-mode source of truth to avoid subtle cache-staleness bugs.phpmyfaq/src/phpMyFAQ/EventListener/ControllerContainerListener.php (1)
36-48: Redundant container initialization ininitializeFromContainer()
AbstractController::__construct()callsinitializeFromContainer()which retrieves configuration, user, and session from the container (lines 106-108) and sets up Twig (line 110). When the listener callssetContainer(), these assignments happen again. WhileisSecured()is guarded by the$containerInitializedflag to prevent double execution, the container lookups and Twig setup still re-run unnecessarily.Consider guarding the configuration/user/session retrieval with the existing flag to avoid redundant work:
protected function initializeFromContainer(): void { if ($this->container === null) { return; } if (!$this->containerInitialized) { $this->configuration = $this->container->get(id: 'phpmyfaq.configuration'); $this->currentUser = $this->container->get(id: 'phpmyfaq.user.current_user'); $this->session = $this->container->get(id: 'session'); TwigWrapper::setTemplateSetName($this->configuration->getTemplateSet()); $this->containerInitialized = true; $this->isSecured(); } }tests/phpMyFAQ/EventListener/WebExceptionListenerTest.php (1)
1-2: Missingdeclare(strict_types=1)statement.The other test file (
ApiExceptionListenerTest.php) also omits this, but the production code and thePhpMyFaqTestKernel.phptest file include it. For consistency and to catch type coercion bugs early, consider addingdeclare(strict_types=1);after the opening<?phptag.Proposed fix
<?php + +declare(strict_types=1); namespace phpMyFAQ\EventListener;tests/phpMyFAQ/EventListener/ApiExceptionListenerTest.php (2)
74-85: AddassertNotNullbefore dereferencing the response.In
testHandlesApiRequestsByPath(line 52) you assert$this->assertNotNull($response)before accessing its content, but intestHandlesUnauthorizedException,testHandlesForbiddenException,testHandlesBadRequestException, andtestHandlesGenericException, the response is used directly without a null guard. If the listener fails to set a response, these tests would produce a confusingTypeErroronnull->getContent()rather than a clear assertion failure.Example fix for this method (apply similarly to others)
$response = $event->getResponse(); + $this->assertNotNull($response); $content = json_decode($response->getContent(), true);
1-2: Missingdeclare(strict_types=1)— same as WebExceptionListenerTest.tests/phpMyFAQ/Functional/WebTestCase.php (1)
85-119: Consider separatingHttpKernelBrowserinto its own file.Having two classes (
WebTestCaseandHttpKernelBrowser) in a single file violates PSR-4 autoloading conventions (one class per file). While pragmatic for test utilities,HttpKernelBrowseris referenced externally (e.g., as a type hint inWebTestCase::$clientandcreateClient), so it would benefit from its own file for discoverability and autoloading correctness.phpmyfaq/src/phpMyFAQ/Controller/AbstractController.php (2)
81-95: Double initialization: constructor work is discarded when kernel callssetContainer().When running under the Kernel, the constructor eagerly builds a fallback
ContainerBuilder, loads services, and pullsconfiguration/currentUser/session— all of which are immediately overwritten bysetContainer(). This is wasteful and, more importantly,isSecured()can throwUnauthorizedHttpExceptionduring the constructor before the kernel has a chance to inject the real container.Consider making the constructor a no-op (or lazy) when a kernel is in use. For example, defer
createContainer()andinitializeFromContainer()until the first access, or skip the constructor initialization entirely when the kernel lifecycle is expected.♻️ Sketch: lazy initialization
public function __construct() { - $this->container = $this->createContainer(); - $this->initializeFromContainer(); + // Defer initialization — if the Kernel is active, + // setContainer() will be called before the action method runs. } +/** + * Ensures the container and services are initialized (lazy fallback). + */ +protected function ensureInitialized(): void +{ + if ($this->container !== null) { + return; + } + $this->container = $this->createContainer(); + $this->initializeFromContainer(); +} public function setContainer(ContainerInterface $container): void { $this->container = $container; $this->initializeFromContainer(); }Then have subclasses or the
render()/getTwigWrapper()paths call$this->ensureInitialized()so the fallback only triggers when no kernel container was injected.
337-351: Pre-existing: swallowed exception increateContainer()could lead to cryptic failures.If loading
services.phpfails, the exception is echoed (line 344) but execution continues with an incomplete container. WheninitializeFromContainer()then calls$this->container->get('phpmyfaq.configuration'), it will throw an unrelated "service not found" error, masking the root cause. Consider re-throwing or logging the exception properly — especially now that this path serves as the fallback for non-kernel usage.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
phpmyfaq/src/phpMyFAQ/Controller/Api/CommentController.php (1)
143-143:⚠️ Potential issue | 🟡 MinorRoute name missing action segment per naming convention.
The coding guideline requires API routes to follow
api.{resource}.{action}. The current nameapi.commentsomits the action. It should beapi.comments.list(or similar).Also, the OpenAPI
pathon line 49 references{faqId}while the#[Route]uses{recordId}— this mismatch will confuse API consumers reading the generated docs.Proposed fix
- #[Route(path: 'v3.2/comments/{recordId}', name: 'api.comments', methods: ['GET'])] + #[Route(path: 'v3.2/comments/{recordId}', name: 'api.comments.list', methods: ['GET'])]And on line 49:
- path: '/api/v3.2/comments/{faqId}', + path: '/api/v3.2/comments/{recordId}',As per coding guidelines, "API routes should follow the naming convention: api.{resource}.{action}".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Api/CommentController.php` at line 143, The route attribute #[Route(path: 'v3.2/comments/{recordId}', name: 'api.comments', methods: ['GET'])] violates the naming convention and has a path-parameter name mismatch with the OpenAPI docs; update the Route name to include the action (e.g., change name to 'api.comments.list') and make the path parameter consistent with the OpenAPI usage by using the same identifier (either change '{recordId}' to '{faqId}' or update the OpenAPI path to '{recordId}') so routing and generated docs match (refer to the Route attribute on the CommentController and the OpenAPI path that currently references {faqId}).phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php (2)
306-312:⚠️ Potential issue | 🔴 CriticalBug: missing
returndiscards the 404 response ingetPopular().Line 309 calls
$this->json(...)but doesn'treturnit. When the result is empty, execution falls through and returns a 200 response on Line 312 instead of a 404. Compare withgetLatest()(Line 357) andgetSticky()(Line 456), which correctly usereturn.🐛 Proposed fix
if ((is_countable($result) ? count($result) : 0) === 0) { - $this->json($result, Response::HTTP_NOT_FOUND); + return $this->json($result, Response::HTTP_NOT_FOUND); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php` around lines 306 - 312, In getPopular() in FaqController, the 404 branch calls $this->json($result, Response::HTTP_NOT_FOUND) but does not return it, so execution continues and a 200 is returned; fix by adding a return to that call (i.e., return $this->json(...)) so the method returns the 404 response early—mirror the behavior used in getLatest() and getSticky().
401-407:⚠️ Potential issue | 🔴 CriticalBug: missing
returndiscards the 404 response ingetTrending().Same issue as
getPopular()— the 404 response on Line 404 is not returned, so the method always returns 200.🐛 Proposed fix
if ((is_countable($result) ? count($result) : 0) === 0) { - $this->json($result, Response::HTTP_NOT_FOUND); + return $this->json($result, Response::HTTP_NOT_FOUND); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php` around lines 401 - 407, In getTrending(), the 404 response created by $this->json($result, Response::HTTP_NOT_FOUND) is never returned, so the method always falls through to return HTTP 200; fix by returning the 404 response (add a return before $this->json(...)) when (is_countable($result) ? count($result) : 0) === 0 so the method exits early and does not continue to the final return; locate the check around $result = array_values($this->faqStatistics->getTrendingData()) and update the conditional to return the 404 response.tests/phpMyFAQ/Controller/Frontend/Api/VotingControllerTest.php (1)
144-162:⚠️ Potential issue | 🟡 MinorTest that accepts both success and failure can never fail — provides no meaningful assertion.
This try/catch pattern makes the test pass regardless of outcome: if the controller returns a response, it asserts status is 200 or 400; if it throws, it asserts the message is non-empty. Since the controller is instantiated without a container,
$this->container->get(...)will always throw, so the success branch is dead code.Consider either:
- Setting up the container/mocks so the happy path is actually exercised and asserting a specific outcome.
- If the intent is only to verify that validation passes for valid input, test up to the point where the container is needed and use
expectExceptionfor the container failure explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Controller/Frontend/Api/VotingControllerTest.php` around lines 144 - 162, The test testCreateWithValidVoteValueReturnsJsonResponseOrThrowsException is too permissive and masks failures; either (A) fully exercise the happy path by wiring/mocking the controller's dependencies (provide a mock container or inject required services into VotingController, e.g., mock the container->get calls used inside VotingController::create) and then assert a single expected response status/body, or (B) if you only want to verify validation up to container access, rewrite the test to use expectException/expectExceptionMessage for the container failure and assert validation behavior before the container call (call the validation method directly or inspect the request handling up to that point). Ensure you remove the try/catch and target either a deterministic response assertion or an explicit expected exception so the test cannot trivially pass both ways.
🧹 Nitpick comments (13)
phpmyfaq/src/phpMyFAQ/Controller/Api/SearchController.php (1)
38-48: Transitional DI constructor looks reasonable for the migration.The nullable parameter with container fallback is a pragmatic approach during the migration to Symfony Kernel DI. Once the migration is complete and all services are injected, consider removing the fallback path and making
$searchnon-nullable.♻️ Post-migration cleanup suggestion
Once full Kernel-based DI is in place:
- private readonly Search $search; - - public function __construct(?Search $search = null) - { - parent::__construct(); - $resolvedSearch = $search ?? $this->container?->get(id: 'phpmyfaq.search'); - if (!$resolvedSearch instanceof Search) { - throw new \RuntimeException('Search service "phpmyfaq.search" is not available.'); - } - $this->search = $resolvedSearch; - } + public function __construct( + private readonly Search $search, + ) { + parent::__construct(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Api/SearchController.php` around lines 38 - 48, The constructor currently accepts a nullable Search and falls back to $this->container->get('phpmyfaq.search') with a runtime check; after migration make the dependency explicit by changing the constructor to require a non-null Search (constructor(Search $search)), remove the container fallback and the instanceof/RuntimeException check, and directly assign $this->search = $search in the SearchController constructor so the service is provided by Symfony DI.phpmyfaq/src/phpMyFAQ/User/UserSession.php (1)
163-163: Inconsistent: creates a newRequest::createFromGlobals()instead of reusing$requestfrom line 136.This line constructs a fresh request object while
$requestis already available in scope. Given the PR's goal of moving toward explicit request handling via the Kernel, this is a good candidate for cleanup.- $remoteAddress = Request::createFromGlobals()->getClientIp(); + $remoteAddress = $request->getClientIp();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/User/UserSession.php` at line 163, Replace the new Request::createFromGlobals() call with the existing $request instance to avoid creating a fresh request; use $request->getClientIp() to populate $remoteAddress in the UserSession class (replace the Request::createFromGlobals()->getClientIp() expression), ensuring the method consistently uses the injected/available $request variable already in scope.phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php (2)
46-71: Hybrid constructor DI: acceptable as transitional, but plan to remove container fallback.The nullable-params-with-container-fallback pattern works during migration, but once the Symfony Kernel wiring is complete, the constructor should rely solely on autowired dependencies (non-nullable) and drop the
$this->container?->get(...)fallback. This avoids hidden coupling to service IDs and makes misconfiguration fail fast at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php` around lines 46 - 71, Update FaqController::__construct to remove the nullable parameters and container fallback: make the constructor accept non-nullable Faq, Tags, FaqStatistics and FaqMetaData parameters (no default nulls), remove all uses of $this->container?->get(...) and the RuntimeException block, and directly assign the injected instances to $this->faq, $this->tags, $this->faqStatistics and $this->faqMetaData so misconfiguration surfaces at compile time instead of using service IDs at runtime.
562-564: NullableRequestwithcreateFromGlobals()fallback undermines kernel request lifecycle.In a Symfony Kernel architecture, the
Requestis always provided by the kernel to the controller. Making it nullable and falling back toRequest::createFromGlobals()bypasses middleware, event listeners, and any request decoration the kernel performs. If this is solely for testability, prefer injecting a constructedRequestin tests instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php` around lines 562 - 564, The list method currently accepts a nullable Request and calls Request::createFromGlobals(), which bypasses the Symfony kernel lifecycle; change the signature of list to require a non-null Request (public function list(Request $request): JsonResponse) and remove the fallback line (Request::createFromGlobals()) inside the method; update any callers/tests to explicitly construct and pass a Request to FaqController::list (or mock the Request) so middleware/event listeners still run under the kernel and tests remain deterministic.phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/VotingController.php (1)
62-68: Redundant checks remain on line 70 after these new validations.After the guards on lines 62–68, we already know
$faqId > 0,$voteis set, and$voteis in[1, 5]. Line 70's conditionisset($vote) && ... && $vote > 0 && $vote < 6is now redundant — the only meaningful part left is$rating->check($faqId, $userIp).Consider simplifying line 70 to only check the rating/IP guard:
♻️ Suggested simplification
- if (isset($vote) && $rating->check($faqId, $userIp) && $vote > 0 && $vote < 6) { + if ($rating->check($faqId, $userIp)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/VotingController.php` around lines 62 - 68, The redundant isset/value range check should be removed from the conditional at the call to $rating->check: since earlier guards already ensure $faqId > 0, isset($vote) and $vote is between 1 and 5, simplify the condition on the line using $rating->check($faqId, $userIp) to only invoke the rating/IP guard (i.e., call $rating->check($faqId, $userIp) and act on its boolean result) and eliminate the duplicated checks against $vote and $faqId in VotingController (method handling the vote).phpmyfaq/src/phpMyFAQ/Controller/Api/GroupController.php (1)
129-146: Sorting assumes default ASC order fromgetAllGroups— worth a note.The code only applies
usortfor DESC; for ASC it trusts the order returned bygetAllGroups. If that method's ordering ever changes, this will silently break. This is acceptable for now given there's only one sort field, but a brief inline comment clarifying the assumption would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Api/GroupController.php` around lines 129 - 146, Add a short inline comment in GroupController::getAllGroups handling (around the usort block where $sort->getOrderSql() is checked) stating that ASC order relies on the ordering returned by getAllGroups and that only DESC is enforced via usort; reference $sort->getOrderSql(), usort($allGroups, ...), and getAllGroups so future maintainers know the ASC assumption and can update the code if getAllGroups' ordering changes.phpmyfaq/src/phpMyFAQ/Controller/Api/GlossaryController.php (1)
134-138: Remove unnecessary dead condition check.
Language::setLanguageByAcceptLanguage()declares astringreturn type and always returns a string, making the!== falsecheck always true. Theifguard is dead code and can be simplified:Suggested simplification
- $currentLanguage = $this->language->setLanguageByAcceptLanguage(); - - if ($currentLanguage !== false) { - $this->glossary->setLanguage($currentLanguage); - } + $currentLanguage = $this->language->setLanguageByAcceptLanguage(); + $this->glossary->setLanguage($currentLanguage);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Api/GlossaryController.php` around lines 134 - 138, The code checks the return of Language::setLanguageByAcceptLanguage() against false even though the method's signature returns string, so remove the dead conditional: call $this->language->setLanguageByAcceptLanguage(), capture (or inline) the resulting string into $currentLanguage, and directly pass it to $this->glossary->setLanguage($currentLanguage) (or call $this->glossary->setLanguage($this->language->setLanguageByAcceptLanguage())) and delete the unnecessary if guard; update any related variable usage accordingly.phpmyfaq/src/phpMyFAQ/Controller/ContainerControllerResolver.php (1)
38-59: Optimize controller instantiation by overridinginstantiateController()instead.
parent::getController()instantiates the controller via reflection (callinginstantiateController()internally), and then if the class is registered in the DI container (line 52), the first instance is discarded and a second one fetched. This means every container-registered controller is instantiated twice per request.Override the protected
instantiateController()method instead — this is the idiomatic Symfony extension point for customizing instantiation and avoids the double-creation:♻️ Suggested approach using instantiateController()
- #[\Override] - public function getController(Request $request): callable|false + #[\Override] + protected function instantiateController(string $class): object { - $controller = parent::getController($request); - - if ($controller === false) { - return false; - } - - // Handle array-style callables [object, method] - if (is_array($controller) && isset($controller[0]) && is_object($controller[0])) { - $controllerClass = $controller[0]::class; - - if ($this->container->has($controllerClass)) { - $controller[0] = $this->container->get($controllerClass); - } - - return $controller; - } - - return $controller; + if ($this->container->has($class)) { + return $this->container->get($class); + } + + return parent::instantiateController($class); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/ContainerControllerResolver.php` around lines 38 - 59, The current getController() in ContainerControllerResolver calls parent::getController() which instantiates controller instances and then swaps in the container-provided instance, causing double-instantiation for container-registered controllers; instead override the protected instantiateController() method in ContainerControllerResolver and implement the container lookup there: if $this->container->has($controllerClass) return $this->container->get($controllerClass) else delegate to parent::instantiateController(...); keep getController() simple (remove the array/object replacement logic) so controllers are instantiated once via the new instantiateController() override and references to container->has and container->get use the controller class name derived from the callable (matching the current logic).tests/phpMyFAQ/EventListener/WebExceptionListenerTest.php (1)
51-62: Consider adding a test forNotFoundHttpExceptionin addition toResourceNotFoundException.The listener handles both
ResourceNotFoundExceptionandNotFoundHttpExceptionin the same match arm. Currently onlyResourceNotFoundExceptionis tested. Adding aNotFoundHttpExceptiontest would confirm both exception types produce a 404.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/EventListener/WebExceptionListenerTest.php` around lines 51 - 62, Add a new unit test mirroring testHandlesResourceNotFoundException that verifies NotFoundHttpException is handled as a 404: create a Request (like Request::create('/nonexistent-page.html')), build the exception event using the same helper createEvent($request, new NotFoundHttpException('not found')), call $this->listener->onKernelException($event), then assert $event->getResponse() is not null and its status equals Response::HTTP_NOT_FOUND; reference the existing testHandlesResourceNotFoundException, the createEvent helper, the onKernelException method on the listener, and the NotFoundHttpException class.phpmyfaq/src/phpMyFAQ/EventListener/ApiExceptionListener.php (1)
90-94:Requestis referenced by FQCN instead of using an import.
\Symfony\Component\HttpFoundation\Requestis used inline while other Symfony types are imported at the top. Add the import for consistency.Proposed fix
Add to imports:
use Symfony\Component\HttpFoundation\Exception\BadRequestException; +use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response;Then update the method signature:
private function createProblemDetailsResponse( - \Symfony\Component\HttpFoundation\Request $request, + Request $request, int $status,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/EventListener/ApiExceptionListener.php` around lines 90 - 94, The method createProblemDetailsResponse currently types the $request parameter with the fully-qualified class name \Symfony\Component\HttpFoundation\Request while other Symfony types are imported; add a top-level import "use Symfony\Component\HttpFoundation\Request;" and update the createProblemDetailsResponse signature to type-hint Request (i.e. replace \Symfony\Component\HttpFoundation\Request $request with Request $request) to keep imports consistent with the rest of the file.tests/phpMyFAQ/Functional/WebTestCase.php (1)
85-112: Consider extractingHttpKernelBrowserinto its own file.Having two classes (
WebTestCaseandHttpKernelBrowser) in a single file works but goes against the one-class-per-file convention in PSR-4 autoloading. If the test autoloader mapsphpMyFAQ\Functional\HttpKernelBrowserto this file via a classmap that's fine, but if another test ever needs to referenceHttpKernelBrowserindependently, it won't be auto-discoverable by filename.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Functional/WebTestCase.php` around lines 85 - 112, Extract the HttpKernelBrowser class into its own file: create a new file declaring the same namespace and define class HttpKernelBrowser with the existing properties and methods (constructor, doRequest, getResponse) and required use/imports (Response, Request, HttpKernelInterface, AbstractBrowser, History, CookieJar). Remove the class from the WebTestCase file and update any references there to import phpMyFAQ\Functional\HttpKernelBrowser; ensure PSR-4/autoloader or tests' classmap can discover the new file and run tests to verify nothing else breaks.phpmyfaq/src/phpMyFAQ/Kernel.php (1)
136-153:$configurationmay benullwhen passed toRouteCollectionBuilder.Line 138 uses the null-safe operator, meaning
$configurationcan benullif the service isn't registered. This null is then passed toRouteCollectionBuilderon Lines 146 and 151. IfRouteCollectionBuilderdoesn't handle a null configuration, this will throw an unrelated error.Since
buildContainer()now fails fast ifservices.phpcan't load, the service should be available in practice — but the null-safe operator signals uncertainty. Consider either removing the?->(since the container is guaranteed non-null afterbuildContainer()) or adding a guard.Proposed fix — remove unnecessary null-safe operator
- $configuration = $this->container?->get(id: 'phpmyfaq.configuration'); + $configuration = $this->container->get(id: 'phpmyfaq.configuration');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Kernel.php` around lines 136 - 153, In loadRoutes(), $configuration is retrieved using the null-safe operator ($this->container?->get(...)) which allows $configuration to be null and then passes it to RouteCollectionBuilder; either remove the unnecessary null-safe operator and call $this->container->get('phpmyfaq.configuration') since buildContainer() guarantees the container exists, or add an explicit guard that throws a clear exception if $this->container or the configuration service is missing before constructing RouteCollectionBuilder (referencing loadRoutes(), $configuration, RouteCollectionBuilder and buildContainer()).phpmyfaq/src/phpMyFAQ/EventListener/LanguageListener.php (1)
94-102: Redundant try/catch — just re-throws the same exception.The catch block simply re-throws the original exception unchanged, making the entire try/catch a no-op. Remove it to reduce noise.
Proposed fix
private function initializeTranslation(string $currentLanguage): void { Strings::init($currentLanguage); - try { - Translation::create() - ->setTranslationsDir(PMF_TRANSLATION_DIR) - ->setDefaultLanguage(defaultLanguage: 'en') - ->setCurrentLanguage($currentLanguage) - ->setMultiByteLanguage(); - } catch (Exception $exception) { - throw $exception; - } + Translation::create() + ->setTranslationsDir(PMF_TRANSLATION_DIR) + ->setDefaultLanguage(defaultLanguage: 'en') + ->setCurrentLanguage($currentLanguage) + ->setMultiByteLanguage(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/EventListener/LanguageListener.php` around lines 94 - 102, The try/catch around the Translation initialization is redundant because the catch only rethrows the same Exception; remove the entire try { ... } catch (Exception $exception) { throw $exception; } block and leave the Translation::create()->setTranslationsDir(PMF_TRANSLATION_DIR)->setDefaultLanguage(defaultLanguage: 'en')->setCurrentLanguage($currentLanguage)->setMultiByteLanguage(); call to let exceptions bubble naturally; update any surrounding indentation/whitespace accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/GroupController.php`:
- Line 111: The route name on the Route attribute in GroupController (currently
'api.groups') does not follow the api.{resource}.{action} convention; update the
Route attribute for the v3.2 groups GET endpoint to use the name
'api.groups.list' (replace the current 'api.groups') so it matches other list
endpoints like api.categories.list and api.faqs.list.
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/NewsController.php`:
- Line 119: The route name in the Route attribute on the NewsController endpoint
currently uses "api_news_list" which doesn't follow the required
api.{resource}.{action} convention; update the Route attribute's name value to
"api.news.list" (i.e., replace api_news_list with api.news.list) so the route
follows the dot-separated convention used across the API.
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/OpenQuestionController.php`:
- Line 133: The route attribute on the OpenQuestionController method uses an
underscore name and an absolute path; update the Route attribute to follow the
codebase convention (dot-separated name and relative path). Change
#[Route('/api/v3.2/open-questions', name: 'api_open_questions', methods:
['GET'])] to use a relative path like 'v3.2/open-questions' and a dot-separated
name such as 'api.open_questions.list' (or match existing resource naming e.g.,
'api.open-questions.list' if hyphens are used elsewhere), keeping methods:
['GET']; apply this edit on the Route attribute in OpenQuestionController (the
method annotated with #[Route(...)]). Ensure the new name matches other API
route naming patterns (e.g., api.categories.list) and update any code/tests
referencing the old route name.
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/TagController.php`:
- Line 122: The route attribute on TagController (#[Route('/api/v3.2/tags',
name: 'api.tags', methods: ['GET'])]) uses the wrong route name; update the name
to follow the api.{resource}.{action} convention (change 'api.tags' to
'api.tags.list') so the route in TagController matches the required naming
scheme for the GET list action.
In `@phpmyfaq/src/phpMyFAQ/EventListener/LanguageListener.php`:
- Around line 77-80: In LanguageListener (class LanguageListener) replace the
two require calls that load PMF_TRANSLATION_DIR . '/language_en.php' and
PMF_TRANSLATION_DIR . '/language_' . strtolower($currentLanguage) . '.php' with
require_once to avoid redeclaration errors; update both the default English
include and the conditional language include (inside the
Language::isASupportedLanguage check) to use require_once so repeated loads from
other bootstrap paths or tests are safe.
In `@phpmyfaq/src/phpMyFAQ/EventListener/WebExceptionListener.php`:
- Around line 82-98: The try-block invokes PageNotFoundController::index via
ControllerResolver/ArgumentResolver but the controller needs the container
($this->container) to render the styled 404, so it will fail and fall back to
plain-text handleErrorResponse; fix by either (A) ensuring the controller
instance has the container before calling it (resolve controller instance and
call setContainer(...) or otherwise inject the container) so
PageNotFoundController::index can call $this->render(), or (B) delegate to the
HttpKernel as a sub-request (create a Request clone with _route/_controller set
and call $kernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST)) to let
the framework resolve/inject dependencies and return the normal styled 404;
update the code around ControllerResolver/ArgumentResolver accordingly and
remove dead invocation if you choose to always use the fallback.
In `@tests/phpMyFAQ/Controller/AbstractControllerTest.php`:
- Around line 457-504: The anonymous test controller currently calls the
AbstractController parent constructor (new class() extends AbstractController
{}), which triggers createContainer() and initializeFromContainer(); fix by
overriding the constructor in the anonymous class (define __construct() as an
empty method) so the parent constructor is not invoked, then rely on
setContainer() to inject the test containers; update the instantiation used in
testSetContainerReEvaluatesIsSecuredWhenContainerChanges() to use the anonymous
class with the empty __construct() to avoid fallback container initialization.
---
Outside diff comments:
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/CommentController.php`:
- Line 143: The route attribute #[Route(path: 'v3.2/comments/{recordId}', name:
'api.comments', methods: ['GET'])] violates the naming convention and has a
path-parameter name mismatch with the OpenAPI docs; update the Route name to
include the action (e.g., change name to 'api.comments.list') and make the path
parameter consistent with the OpenAPI usage by using the same identifier (either
change '{recordId}' to '{faqId}' or update the OpenAPI path to '{recordId}') so
routing and generated docs match (refer to the Route attribute on the
CommentController and the OpenAPI path that currently references {faqId}).
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php`:
- Around line 306-312: In getPopular() in FaqController, the 404 branch calls
$this->json($result, Response::HTTP_NOT_FOUND) but does not return it, so
execution continues and a 200 is returned; fix by adding a return to that call
(i.e., return $this->json(...)) so the method returns the 404 response
early—mirror the behavior used in getLatest() and getSticky().
- Around line 401-407: In getTrending(), the 404 response created by
$this->json($result, Response::HTTP_NOT_FOUND) is never returned, so the method
always falls through to return HTTP 200; fix by returning the 404 response (add
a return before $this->json(...)) when (is_countable($result) ? count($result) :
0) === 0 so the method exits early and does not continue to the final return;
locate the check around $result =
array_values($this->faqStatistics->getTrendingData()) and update the conditional
to return the 404 response.
In `@tests/phpMyFAQ/Controller/Frontend/Api/VotingControllerTest.php`:
- Around line 144-162: The test
testCreateWithValidVoteValueReturnsJsonResponseOrThrowsException is too
permissive and masks failures; either (A) fully exercise the happy path by
wiring/mocking the controller's dependencies (provide a mock container or inject
required services into VotingController, e.g., mock the container->get calls
used inside VotingController::create) and then assert a single expected response
status/body, or (B) if you only want to verify validation up to container
access, rewrite the test to use expectException/expectExceptionMessage for the
container failure and assert validation behavior before the container call (call
the validation method directly or inspect the request handling up to that
point). Ensure you remove the try/catch and target either a deterministic
response assertion or an explicit expected exception so the test cannot
trivially pass both ways.
---
Nitpick comments:
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php`:
- Around line 46-71: Update FaqController::__construct to remove the nullable
parameters and container fallback: make the constructor accept non-nullable Faq,
Tags, FaqStatistics and FaqMetaData parameters (no default nulls), remove all
uses of $this->container?->get(...) and the RuntimeException block, and directly
assign the injected instances to $this->faq, $this->tags, $this->faqStatistics
and $this->faqMetaData so misconfiguration surfaces at compile time instead of
using service IDs at runtime.
- Around line 562-564: The list method currently accepts a nullable Request and
calls Request::createFromGlobals(), which bypasses the Symfony kernel lifecycle;
change the signature of list to require a non-null Request (public function
list(Request $request): JsonResponse) and remove the fallback line
(Request::createFromGlobals()) inside the method; update any callers/tests to
explicitly construct and pass a Request to FaqController::list (or mock the
Request) so middleware/event listeners still run under the kernel and tests
remain deterministic.
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/GlossaryController.php`:
- Around line 134-138: The code checks the return of
Language::setLanguageByAcceptLanguage() against false even though the method's
signature returns string, so remove the dead conditional: call
$this->language->setLanguageByAcceptLanguage(), capture (or inline) the
resulting string into $currentLanguage, and directly pass it to
$this->glossary->setLanguage($currentLanguage) (or call
$this->glossary->setLanguage($this->language->setLanguageByAcceptLanguage()))
and delete the unnecessary if guard; update any related variable usage
accordingly.
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/GroupController.php`:
- Around line 129-146: Add a short inline comment in
GroupController::getAllGroups handling (around the usort block where
$sort->getOrderSql() is checked) stating that ASC order relies on the ordering
returned by getAllGroups and that only DESC is enforced via usort; reference
$sort->getOrderSql(), usort($allGroups, ...), and getAllGroups so future
maintainers know the ASC assumption and can update the code if getAllGroups'
ordering changes.
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/SearchController.php`:
- Around line 38-48: The constructor currently accepts a nullable Search and
falls back to $this->container->get('phpmyfaq.search') with a runtime check;
after migration make the dependency explicit by changing the constructor to
require a non-null Search (constructor(Search $search)), remove the container
fallback and the instanceof/RuntimeException check, and directly assign
$this->search = $search in the SearchController constructor so the service is
provided by Symfony DI.
In `@phpmyfaq/src/phpMyFAQ/Controller/ContainerControllerResolver.php`:
- Around line 38-59: The current getController() in ContainerControllerResolver
calls parent::getController() which instantiates controller instances and then
swaps in the container-provided instance, causing double-instantiation for
container-registered controllers; instead override the protected
instantiateController() method in ContainerControllerResolver and implement the
container lookup there: if $this->container->has($controllerClass) return
$this->container->get($controllerClass) else delegate to
parent::instantiateController(...); keep getController() simple (remove the
array/object replacement logic) so controllers are instantiated once via the new
instantiateController() override and references to container->has and
container->get use the controller class name derived from the callable (matching
the current logic).
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/VotingController.php`:
- Around line 62-68: The redundant isset/value range check should be removed
from the conditional at the call to $rating->check: since earlier guards already
ensure $faqId > 0, isset($vote) and $vote is between 1 and 5, simplify the
condition on the line using $rating->check($faqId, $userIp) to only invoke the
rating/IP guard (i.e., call $rating->check($faqId, $userIp) and act on its
boolean result) and eliminate the duplicated checks against $vote and $faqId in
VotingController (method handling the vote).
In `@phpmyfaq/src/phpMyFAQ/EventListener/ApiExceptionListener.php`:
- Around line 90-94: The method createProblemDetailsResponse currently types the
$request parameter with the fully-qualified class name
\Symfony\Component\HttpFoundation\Request while other Symfony types are
imported; add a top-level import "use Symfony\Component\HttpFoundation\Request;"
and update the createProblemDetailsResponse signature to type-hint Request (i.e.
replace \Symfony\Component\HttpFoundation\Request $request with Request
$request) to keep imports consistent with the rest of the file.
In `@phpmyfaq/src/phpMyFAQ/EventListener/LanguageListener.php`:
- Around line 94-102: The try/catch around the Translation initialization is
redundant because the catch only rethrows the same Exception; remove the entire
try { ... } catch (Exception $exception) { throw $exception; } block and leave
the
Translation::create()->setTranslationsDir(PMF_TRANSLATION_DIR)->setDefaultLanguage(defaultLanguage:
'en')->setCurrentLanguage($currentLanguage)->setMultiByteLanguage(); call to let
exceptions bubble naturally; update any surrounding indentation/whitespace
accordingly.
In `@phpmyfaq/src/phpMyFAQ/Kernel.php`:
- Around line 136-153: In loadRoutes(), $configuration is retrieved using the
null-safe operator ($this->container?->get(...)) which allows $configuration to
be null and then passes it to RouteCollectionBuilder; either remove the
unnecessary null-safe operator and call
$this->container->get('phpmyfaq.configuration') since buildContainer()
guarantees the container exists, or add an explicit guard that throws a clear
exception if $this->container or the configuration service is missing before
constructing RouteCollectionBuilder (referencing loadRoutes(), $configuration,
RouteCollectionBuilder and buildContainer()).
In `@phpmyfaq/src/phpMyFAQ/User/UserSession.php`:
- Line 163: Replace the new Request::createFromGlobals() call with the existing
$request instance to avoid creating a fresh request; use $request->getClientIp()
to populate $remoteAddress in the UserSession class (replace the
Request::createFromGlobals()->getClientIp() expression), ensuring the method
consistently uses the injected/available $request variable already in scope.
In `@tests/phpMyFAQ/EventListener/WebExceptionListenerTest.php`:
- Around line 51-62: Add a new unit test mirroring
testHandlesResourceNotFoundException that verifies NotFoundHttpException is
handled as a 404: create a Request (like
Request::create('/nonexistent-page.html')), build the exception event using the
same helper createEvent($request, new NotFoundHttpException('not found')), call
$this->listener->onKernelException($event), then assert $event->getResponse() is
not null and its status equals Response::HTTP_NOT_FOUND; reference the existing
testHandlesResourceNotFoundException, the createEvent helper, the
onKernelException method on the listener, and the NotFoundHttpException class.
In `@tests/phpMyFAQ/Functional/WebTestCase.php`:
- Around line 85-112: Extract the HttpKernelBrowser class into its own file:
create a new file declaring the same namespace and define class
HttpKernelBrowser with the existing properties and methods (constructor,
doRequest, getResponse) and required use/imports (Response, Request,
HttpKernelInterface, AbstractBrowser, History, CookieJar). Remove the class from
the WebTestCase file and update any references there to import
phpMyFAQ\Functional\HttpKernelBrowser; ensure PSR-4/autoloader or tests'
classmap can discover the new file and run tests to verify nothing else breaks.
phpmyfaq/src/phpMyFAQ/Controller/Api/OpenQuestionController.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/AttachmentController.php (1)
41-42:⚠️ Potential issue | 🟠 MajorHTTP method mismatch:
delete()action usesmethods: ['GET']but frontend sendsDELETE.The frontend TypeScript code (
phpmyfaq/admin/assets/src/api/attachment.tsline 19-20) sends a DELETE request to this endpoint, but the route at line 41 only accepts GET. This breaks the API contract and violates REST conventions. The method should bemethods: ['DELETE'].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/AttachmentController.php` around lines 41 - 42, The Route attribute on the delete action is using methods: ['GET'] while the controller method delete(Request $request): JsonResponse is intended to handle DELETE requests; update the Route attribute (#[Route(path: 'content/attachments', name: 'admin.api.content.attachments', methods: [...])]) to use methods: ['DELETE'] so the route accepts DELETE requests and matches the frontend behavior and REST conventions.phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/CaptchaController.php (1)
29-46:⚠️ Potential issue | 🔴 CriticalIncorrect type injection:
Captchais a static factory, not a service withgetCaptchaImage()method.The
Captchaclass only contains a staticgetInstance()method that returnsBuiltinCaptcha|GoogleRecaptcha. It cannot be instantiated directly or injected as a service. The type hint on line 29 should beCaptchaInterfaceinstead, since bothBuiltinCaptchaandGoogleRecaptchaimplement that interface and provide thegetCaptchaImage()method. Calling$this->captcha->getCaptchaImage()on line 46 will fail at runtime because theCaptchaclass has no such method.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/CaptchaController.php` around lines 29 - 46, The constructor currently type-hints the static factory Captcha which has no getCaptchaImage(); change the constructor signature in CaptchaController::__construct to accept the CaptchaInterface (the interface implemented by BuiltinCaptcha and GoogleRecaptcha) and store it as $this->captcha so that the call to $this->captcha->getCaptchaImage() in renderImage() uses the concrete service implementing getCaptchaImage(); update any DI/config to inject the concrete CaptchaInterface implementation instead of the Captcha factory.phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/CategoryController.php (1)
76-86:⚠️ Potential issue | 🔴 CriticalUndefined variables
$categoryPermissionand$categoryImage— will cause a fatal error.Lines 83–85 reference local variables
$categoryPermissionand$categoryImagethat don't exist in this scope. The DI refactor at lines 72–74 correctly uses$this->categoryImageand$this->categoryOrder, but this block was missed.This will throw an "undefined variable" fatal error at runtime when deleting a category with exactly one language translation.
🐛 Proposed fix
- $categoryPermission->delete(Permission::USER, [(int) $data->categoryId]); - $categoryPermission->delete(Permission::GROUP, [(int) $data->categoryId]); - $categoryImage->delete(); + $this->categoryPermission->delete(Permission::USER, [(int) $data->categoryId]); + $this->categoryPermission->delete(Permission::GROUP, [(int) $data->categoryId]); + $this->categoryImage->delete();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/CategoryController.php` around lines 76 - 86, The code references undefined local variables $categoryPermission and $categoryImage inside the language-count conditional, causing a fatal error; replace those references with the injected properties used elsewhere (e.g. $this->categoryPermission->delete(Permission::USER, [(int) $data->categoryId]); $this->categoryPermission->delete(Permission::GROUP, [(int) $data->categoryId]); and $this->categoryImage->delete();) so the DI-backed services (categoryPermission and categoryImage) are used consistently with the existing refactor and avoid undefined variable errors while preserving the existing delete calls.phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ElasticsearchController.php (1)
136-152:⚠️ Potential issue | 🟡 Minor
healthcheck()should use the injected$this->elasticsearchinstead of a container lookup.
Elasticsearchis already injected via the constructor (Line 38), buthealthcheck()still fetches it from the container at Line 141. This is inconsistent withcreate(),drop(), andimport()which all use the injected property.Proposed fix
public function healthcheck(): JsonResponse { $this->userIsAuthenticated(); - /** `@var` Elasticsearch $elasticsearch */ - $elasticsearch = $this->container->get(id: 'phpmyfaq.instance.elasticsearch'); - - $isAvailable = $elasticsearch->isAvailable(); + $isAvailable = $this->elasticsearch->isAvailable(); return $this->json(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ElasticsearchController.php` around lines 136 - 152, The healthcheck() method incorrectly fetches Elasticsearch from the container using $this->container->get('phpmyfaq.instance.elasticsearch'); replace that lookup with the injected property $this->elasticsearch so healthcheck() calls $this->elasticsearch->isAvailable() (consistent with create(), drop(), and import()); remove the local $elasticsearch variable and use the injected instance when building the response and status decision.phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php (2)
283-290:⚠️ Potential issue | 🔴 CriticalBug: missing
return— 404 response is silently discarded ingetPopular().On line 286,
$this->json(...)is called withoutreturn, so when there are no popular FAQs the method falls through and returns HTTP 200 with an empty array instead of the intended 404. Compare withgetLatest()(line 334) andgetSticky()(line 433) which correctly usereturn.Proposed fix
if ((is_countable($result) ? count($result) : 0) === 0) { - $this->json($result, Response::HTTP_NOT_FOUND); + return $this->json($result, Response::HTTP_NOT_FOUND); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php` around lines 283 - 290, In getPopular() fix the missing return: when $result is empty the method currently calls $this->json($result, Response::HTTP_NOT_FOUND) without returning, so execution falls through and returns HTTP 200; change the behavior by returning that response (add a return before $this->json) so the 404 is actually sent, mirroring getLatest() and getSticky().
378-385:⚠️ Potential issue | 🔴 CriticalBug: missing
return— 404 response is silently discarded ingetTrending().Same issue as
getPopular(): line 381 creates a 404 response but does not return it.Proposed fix
if ((is_countable($result) ? count($result) : 0) === 0) { - $this->json($result, Response::HTTP_NOT_FOUND); + return $this->json($result, Response::HTTP_NOT_FOUND); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php` around lines 378 - 385, In getTrending() the 404 Response created by $this->json($result, Response::HTTP_NOT_FOUND) is not returned, so the method continues and always returns the 200 response; update getTrending() to return the 404 response when $result is empty (i.e., change the conditional to return $this->json(...)) so the method returns early with Response::HTTP_NOT_FOUND instead of falling through to the final return of Response::HTTP_OK.
🧹 Nitpick comments (18)
phpmyfaq/src/phpMyFAQ/Controller/Api/NewsController.php (1)
133-133:Newsis instantiated manually rather than injected.This is likely pre-existing, but with the move to a Symfony Kernel, services like
Newscould be registered in the container and injected via constructor or method parameters, improving testability and consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Api/NewsController.php` at line 133, The controller currently instantiates News directly with "new News($this->configuration)"; instead, change NewsController to receive the News service via dependency injection (either add a __construct(News $news) storing it on a private property or type-hint the News parameter on the action method), remove the manual instantiation, and use the injected $this->news (or parameter) everywhere; also ensure the News service is registered in the Symfony service container (autowired or explicit service definition) so the controller receives it automatically.phpmyfaq/src/phpMyFAQ/Faq.php (1)
447-451: Inconsistent type casting ofrecords.numberOfRecordsPerPage.Line 450 correctly casts to
(int), but the same config value is read on line 274 ($numPerPage) without any cast and is subsequently used in arithmetic (line 354) and comparison (line 375). Consider casting at line 274 as well for consistency and type safety.Proposed fix
- $numPerPage = $this->configuration->get(item: 'records.numberOfRecordsPerPage'); + $numPerPage = (int) $this->configuration->get(item: 'records.numberOfRecordsPerPage');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Faq.php` around lines 447 - 451, The config value records.numberOfRecordsPerPage is cast to (int) later when passed to Pagination but is read earlier into $numPerPage without casting, causing inconsistent types when $numPerPage is used in arithmetic/comparisons; update the assignment that sets $numPerPage to cast the configuration read (e.g., $numPerPage = (int) $this->configuration->get(item: 'records.numberOfRecordsPerPage')) so all uses of $numPerPage are integer-safe and consistent with the Pagination call.phpmyfaq/src/phpMyFAQ/Controller/SitemapController.php (1)
111-116: Consider passingactiveOnly: truetogetAllPages()to filter at the DB level.
getAllPages()supports anactiveOnlyparameter (seeCustomPage::getAllPages(bool $activeOnly = false)inCustomPage.php), which would avoid fetching inactive pages from the database and the manual check on line 114.Suggested change
- $pages = $this->customPage->getAllPages(); + $pages = $this->customPage->getAllPages(activeOnly: true); foreach ($pages as $page) { - if ($page['active'] !== 'y') { - continue; - } - $urls[] = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/SitemapController.php` around lines 111 - 116, Call CustomPage::getAllPages with activeOnly true from SitemapController instead of fetching all pages and filtering in PHP: replace the call to $this->customPage->getAllPages() in SitemapController::generate (or the method containing the shown loop) with $this->customPage->getAllPages(true) so the DB returns only active pages and remove the manual if ($page['active'] !== 'y') continue; check inside the foreach.tests/phpMyFAQ/Controller/SitemapControllerTest.php (1)
19-19:$twigproperty and stub are unused.The
Environmentstub (line 40) and property (line 19) are never passed to theSitemapControllerconstructor or used in any test assertion. This is dead test code.Suggested cleanup
class SitemapControllerTest extends TestCase { - private Environment $twig; private FaqStatistics $faqStatistics; private CustomPage $customPage; private SitemapController $controller;- $this->twig = $this->createStub(Environment::class); $this->faqStatistics = $this->createStub(FaqStatistics::class);And remove the unused import:
-use Twig\Environment;Also applies to: 40-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Controller/SitemapControllerTest.php` at line 19, The test class SitemapControllerTest declares a private Environment $twig property and creates an Environment stub in the test setup but neither the $twig property nor that stub are passed to SitemapController or used in assertions, so remove the dead test code: delete the private Environment $twig declaration and the Environment stub creation in the test (references to $twig and the Environment::class stub inside SitemapControllerTest and its setUp), and also remove the unused Environment import so imports and setup only include objects actually passed to SitemapController or asserted in tests.tests/phpMyFAQ/Controller/Api/SearchControllerTest.php (1)
17-153: Consider extracting acreateController()helper for consistency.
$this->createStub(Search::class)is repeated in every test method. Other test files in this PR (e.g.,UserControllerTest,ContactControllerTest) use a sharedcreateController()helper. Extracting it would reduce duplication and align with the project pattern.Suggested refactor
class SearchControllerTest extends TestCase { + private function createController(): SearchController + { + return new SearchController($this->createStub(Search::class)); + } + public function testSearchReturnsJsonResponse(): void { $request = new Request(['q' => 'test']); - $controller = new SearchController($this->createStub(Search::class)); + $controller = $this->createController(); $response = $controller->search($request); // ... and so on for each test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Controller/Api/SearchControllerTest.php` around lines 17 - 153, Extract a private helper method createController() in this test class that returns a SearchController constructed with $this->createStub(Search::class), replace all inline constructions like new SearchController($this->createStub(Search::class)) with calls to createController(), and update tests to use that helper (keep existing method names: SearchController, createStub, and Search) to reduce duplication and match the pattern used in UserControllerTest/ContactControllerTest.phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/CategoryController.php (1)
107-107: Inconsistent dependency access:permissions()andupdateOrder()still use container lookups.Lines 107 and 159 use
$this->container->get(...)to resolvephpmyfaq.category.permissionandphpmyfaq.category.order, while the constructor already injects$this->categoryPermissionand$this->categoryOrder. Consider using the injected properties for consistency.♻️ Proposed fix for permissions() (line 107)
- $categoryPermission = $this->container->get(id: 'phpmyfaq.category.permission'); + $categoryPermission = $this->categoryPermission;♻️ Proposed fix for updateOrder() (line 159)
- $categoryOrder = $this->container->get(id: 'phpmyfaq.category.order'); - $categoryOrder->setCategoryTree($data->categoryTree); + $this->categoryOrder->setCategoryTree($data->categoryTree);And update subsequent usages of
$categoryOrderin the method to$this->categoryOrder.Also applies to: 159-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/CategoryController.php` at line 107, Replace direct container lookups inside the permissions() and updateOrder() methods with the already-injected properties: use $this->categoryPermission instead of $this->container->get('phpmyfaq.category.permission') in permissions(), and use $this->categoryOrder instead of $this->container->get('phpmyfaq.category.order') in updateOrder(); also update any subsequent local variable usages ($categoryOrder) in updateOrder() to reference $this->categoryOrder to keep dependency access consistent with the constructor injection.tests/phpMyFAQ/Controller/Api/GlossaryControllerTest.php (2)
127-147: Misleading test name:testListReturnsEmptyArrayOn404assertsHTTP_OK.The test name suggests it verifies 404 behavior, but Line 138 asserts
Response::HTTP_OK. Consider renaming to reflect actual behavior, e.g.testListReturnsSuccessWithDataStructure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Controller/Api/GlossaryControllerTest.php` around lines 127 - 147, The test name testListReturnsEmptyArrayOn404 is misleading because it asserts Response::HTTP_OK; rename the test method to reflect actual behavior (e.g., testListReturnsSuccessWithDataStructure or testListReturnsEmptyArrayWithOkStatus) and update any references; keep the body unchanged (using GlossaryController->list($request), assertions on status code and json structure) so the method name matches the behavior.
1-3: Missingdeclare(strict_types=1);.All other test files in this PR include
declare(strict_types=1);. This file should be consistent.Proposed fix
<?php +declare(strict_types=1); + namespace phpMyFAQ\Controller\Api;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Controller/Api/GlossaryControllerTest.php` around lines 1 - 3, Add the missing strict types directive to this test file: insert declare(strict_types=1); immediately after the opening <?php tag and before the namespace declaration (namespace phpMyFAQ\Controller\Api;) in GlossaryControllerTest.php so it matches the other tests in the PR.tests/phpMyFAQ/Controller/Administration/Api/ElasticsearchControllerTest.php (1)
50-57: Unused$requestvariable in all test methods.
$request = new Request()is created but never passed to the controller methods (create(),drop(),import(),statistics(),healthcheck()take noRequestparameter). This is dead code repeated in all five tests.Example fix (apply to all test methods)
public function testCreateRequiresAuthentication(): void { - $request = new Request(); $controller = new ElasticsearchController($this->elasticsearch, $this->faq, $this->customPage); $this->expectException(\Exception::class); $controller->create(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Controller/Administration/Api/ElasticsearchControllerTest.php` around lines 50 - 57, Remove the unused $request = new Request() statement from all test methods in ElasticsearchControllerTest (e.g., testCreateRequiresAuthentication, testDropRequiresAuthentication, testImportRequiresAuthentication, testStatisticsRequiresAuthentication, testHealthcheckRequiresAuthentication) since ElasticsearchController::create(), ::drop(), ::import(), ::statistics(), and ::healthcheck() do not accept a Request and the variable is dead code; simply delete the $request line from each test to eliminate the unused variable warnings.phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php (1)
538-541: Nullable$requestwithcreateFromGlobals()fallback is fragile.Falling back to
Request::createFromGlobals()bypasses the Kernel-managed request lifecycle and may produce unexpected results (e.g., missing route attributes, different query parameters in tests vs. production). Consider making$requestrequired and ensuring the Kernel always provides it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php` around lines 538 - 541, The list method currently accepts a nullable Request and falls back to Request::createFromGlobals(), which bypasses the Kernel lifecycle; change the signature of list to require a Request (remove the "?"/default null) so Symfony's HttpKernel injects the proper Request and remove the fallback line that calls Request::createFromGlobals(); update any callers/tests to pass a Request to Api/FaqController::list or adjust tests to use the Kernel client to obtain the routed Request instead of calling the controller directly.tests/phpMyFAQ/Controller/Api/FaqControllerTest.php (1)
56-61: Consider extracting acreateController()helper to reduce duplication.The same 4-stub controller instantiation is repeated in every test method (20+ times). The sibling test at
tests/phpMyFAQ/Controller/Frontend/Api/FaqControllerTest.phpalready uses acreateController()pattern. Applying the same here would improve readability and make future constructor changes easier.Suggested helper
+ private function createController(): FaqController + { + return new FaqController( + $this->createStub(Faq::class), + $this->createStub(Tags::class), + $this->createStub(FaqStatistics::class), + $this->createStub(FaqMetaData::class), + ); + }Then replace all inline instantiations with
$controller = $this->createController();.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Controller/Api/FaqControllerTest.php` around lines 56 - 61, Add a private createController() helper in the FaqControllerTest that returns a new FaqController constructed with the four stubs; locate the repeated inline instantiations of FaqController (using createStub(Faq::class), createStub(Tags::class), createStub(FaqStatistics::class), createStub(FaqMetaData::class)) and replace them with $this->createController(). Ensure the helper method name is createController and it constructs the FaqController the same way so all test methods call $this->createController() instead of duplicating the constructor logic.phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/QuestionController.php (1)
58-58: Route name does not follow the frontend naming convention.Same issue as
Frontend\Api\FaqController: the route nameapi.private.question.createshould follow thepublic.{resource}.{action}convention per coding guidelines for controllers underController/Frontend/.As per coding guidelines: "Frontend routes should follow the naming convention: public.{resource}.{action}"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/QuestionController.php` at line 58, The route attribute on the QuestionController create endpoint uses the wrong name; update the Route attribute on the method in phpMyFAQ\Controller\Frontend\Api\QuestionController that currently has path 'question/create' and name 'api.private.question.create' to follow the frontend convention (e.g. change the name to 'public.question.create') so it matches the public.{resource}.{action} pattern used by Frontend controllers.phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/AutoCompleteController.php (1)
50-50: Route name does not follow the frontend naming convention.Same pattern as other
Frontend/Apicontrollers:api.private.autocompleteshould followpublic.{resource}.{action}.As per coding guidelines: "Frontend routes should follow the naming convention: public.{resource}.{action}"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/AutoCompleteController.php` at line 50, The route name on the Route attribute in AutoCompleteController is using api.private.autocomplete which violates the frontend naming convention; update the Route attribute (the #[Route(path: 'autocomplete', name: 'api.private.autocomplete', methods: ['GET'])] on the AutoCompleteController) to use the frontend convention public.{resource}.{action} (e.g., public.autocomplete.index or public.autocomplete.search as appropriate for this endpoint) so the name follows public.{resource}.{action}.phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/CommentController.php (1)
66-66: Route nameapi.private.commentdoesn't follow thepublic.{resource}.{action}convention for frontend routes.Since this file is under
Controller/Frontend/Api/, the convention requirespublic.{resource}.{action}(e.g.,public.comment.create). This appears to be pre-existing, but worth flagging for alignment during this refactor wave.As per coding guidelines: "Frontend routes should follow the naming convention: public.{resource}.{action}"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/CommentController.php` at line 66, Route name on the CommentController route attribute uses api.private.comment which breaks frontend naming rules; update the Route attribute on CommentController (the #[Route(path: 'comment/create', name: 'api.private.comment', methods: ['POST'])] declaration) to use the frontend convention, e.g. name: 'public.comment.create', and then search for and update any usages of the old route name ('api.private.comment') in templates, redirects or route references to the new name to keep callers working.phpmyfaq/src/phpMyFAQ/EventListener/LanguageListener.php (1)
94-102: Redundant try-catch block — the exception is simply re-thrown.The
catchblock catchesExceptionand immediately re-throws it without modification. This try-catch serves no purpose and can be removed.♻️ Proposed simplification
- try { - Translation::create() - ->setTranslationsDir(PMF_TRANSLATION_DIR) - ->setDefaultLanguage(defaultLanguage: 'en') - ->setCurrentLanguage($currentLanguage) - ->setMultiByteLanguage(); - } catch (Exception $exception) { - throw $exception; - } + Translation::create() + ->setTranslationsDir(PMF_TRANSLATION_DIR) + ->setDefaultLanguage(defaultLanguage: 'en') + ->setCurrentLanguage($currentLanguage) + ->setMultiByteLanguage();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/EventListener/LanguageListener.php` around lines 94 - 102, In LanguageListener (around the Translation::create() call) remove the redundant try-catch that catches Exception only to re-throw it; instead invoke Translation::create()->setTranslationsDir(PMF_TRANSLATION_DIR)->setDefaultLanguage('en')->setCurrentLanguage($currentLanguage)->setMultiByteLanguage(); directly (i.e., delete the catch block and surrounding try so exceptions bubble naturally).phpmyfaq/src/phpMyFAQ/EventListener/WebExceptionListener.php (1)
116-131:error_logfor unhandled exceptions — consider structured logging.Using
error_log()directly works but loses structure (no log level, no context array). If a PSR-3 logger is available in the container, it would provide better observability. This isn't blocking, just a future improvement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/EventListener/WebExceptionListener.php` around lines 116 - 131, The current handleServerError method uses error_log which loses structure; instead obtain and use a PSR-3 logger (e.g. inject Psr\Log\LoggerInterface into the class and store as $this->logger) and replace the error_log call in handleServerError with a structured $this->logger->error(...) call that includes the message and a context array (e.g. ['exception' => $throwable, 'file' => $throwable->getFile(), 'line' => $throwable->getLine()]); ensure the class constructor or factory sets the logger property and keep the existing return via handleErrorResponse unchanged.phpmyfaq/src/services.php (1)
710-714: Minor: Batch comment numbering skips from 3 to 6.Line 710 says "Batch 6" but the previous batches are numbered 1, 2, 3. Likely batches 4 and 5 were removed or not yet added. Consider renaming to "Batch 4" for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/services.php` around lines 710 - 714, The batch comment above the RootSitemapController service registration is mislabeled "Batch 6"; update that comment to "Batch 4" (or the appropriate next sequential batch) to match the surrounding batch numbering; locate the comment immediately preceding the RootSitemapController::class registration in the services->set(...) block and change only the comment text so it reflects the correct batch number.tests/phpMyFAQ/EventListener/WebExceptionListenerTest.php (1)
103-119: Suppressingerror_logviaini_set— consider restoring in afinallyblock.If the test fails or an exception is thrown between the two
ini_setcalls, the original error log setting won't be restored.♻️ Safer error_log suppression
// Suppress error_log output $originalErrorLog = ini_get('error_log'); ini_set('error_log', '/dev/null'); - $this->listener->onKernelException($event); - - ini_set('error_log', $originalErrorLog); + try { + $this->listener->onKernelException($event); + } finally { + ini_set('error_log', $originalErrorLog); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/EventListener/WebExceptionListenerTest.php` around lines 103 - 119, The test testHandlesGenericException temporarily changes the PHP error_log setting with ini_set('error_log', '/dev/null') but restores it only after listener->onKernelException and can leak if an exception occurs; wrap the ini_set restore in a try/finally so the original ini_get('error_log') value is always restored (capture originalErrorLog before the try, call $this->listener->onKernelException($event) inside the try, and call ini_set('error_log', $originalErrorLog) in the finally) to ensure error_log is reset even on failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/GlossaryController.php`:
- Around line 126-130: The code currently guards on ($currentLanguage !== false)
but Language::setLanguageByAcceptLanguage() always returns a string, so replace
that dead check with a non-empty-string guard or remove it based on intended
behavior: call Language::setLanguageByAcceptLanguage() into $currentLanguage and
either (A) only call $this->glossary->setLanguage($currentLanguage) when
$currentLanguage !== '' (i.e. use a non-empty string check) or (B) if
setLanguage() is allowed to accept an empty string, remove the guard entirely
and always call $this->glossary->setLanguage($currentLanguage); update the code
around setLanguageByAcceptLanguage() and setLanguage accordingly.
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/NewsController.php`:
- Around line 120-122: The list(?Request $request = null) method should stop
creating a separate Request via Request::createFromGlobals(); change the
signature to list(Request $request): JsonResponse, remove the null default and
the fallback line ($request ??= Request::createFromGlobals()), and ensure any
subsequent code uses the injected $request (which now comes from the Symfony
kernel and contains listener-set attributes) — update references in the list()
body accordingly to match the non-nullable Request.
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/AbstractFrontController.php`:
- Line 89: In AbstractFrontController::getHeader() the 'metaDescription' array
entry references an undefined local $metaDescription; either initialize
$metaDescription earlier in the method or remove the dead variable and use the
configuration directly. Fix by replacing 'metaDescription' => $metaDescription
?? $this->configuration->get('seo.description') with a single source (e.g.
'metaDescription' => $this->configuration->get('seo.description')) or ensure
$metaDescription is assigned before that line so the null-coalescing expression
is meaningful.
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/ContactController.php`:
- Around line 94-96: Remove the duplicate setReplyTo() call that overwrites the
submitter's address: in the block using $this->mailer (where setReplyTo($email,
$author) and addTo($this->configuration->getAdminEmail()) are called), delete
the subsequent setReplyTo($this->configuration->getNoReplyEmail()) so the
Reply-To header remains the submitter's email; leave addTo(...) and, if needed,
use getNoReplyEmail() only for the From header or similar where appropriate.
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/FaqController.php`:
- Around line 143-145: The answer check currently calls
$this->stopWords->checkBannedWord(strip_tags($answer)) but discards its return
value, so banned words in $answer are ignored; update the logic in FaqController
(same block that checks $questionText) to use the return value of
$this->stopWords->checkBannedWord(strip_tags($answer)) as a condition and handle
a false result the same way you handle a banned question (e.g., abort/return the
error response or throw), ensuring the check mirrors the questionText path and
uses the exact symbols $answer, strip_tags and
$this->stopWords->checkBannedWord.
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/UserController.php`:
- Around line 262-264: The second setReplyTo call overwrites the first in
UserController ($this->mailer->setReplyTo($email, $author); then
setReplyTo($this->configuration->getNoReplyEmail())), so remove the redundant
call to $this->mailer->setReplyTo($this->configuration->getNoReplyEmail()); if
the desired behavior is to let admins reply to the requesting user;
alternatively, if the intent was to send from a no-reply address, replace that
second setReplyTo with
$this->mailer->setFrom($this->configuration->getNoReplyEmail()) so reply-to
remains the user while the message sender is the no-reply address.
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/VotingController.php`:
- Around line 77-101: The compound condition in VotingController that checks
isset($vote) and numeric range alongside $this->rating->check(...) is redundant
because earlier guards already validated $faqId and $vote; simplify by replacing
the big if block with if ($this->rating->check($faqId, $userIp)) { ... } (keep
the body that calls userSession->userTracking('save_voting', ...), constructs
Vote, and uses rating->getNumberOfVotings(), rating->create() or
rating->update(), and returns $this->json(...)); then replace the subsequent
separate if (!$this->rating->check(...)) branch with an else { ... } that logs
error_save_voting and returns the err_VoteTooMuch response, removing the
now-unreachable final return so control flow is clear and non-redundant.
In `@phpmyfaq/src/phpMyFAQ/Instance/Search/Elasticsearch.php`:
- Around line 121-158: buildMappings() currently sets 'search_analyzer' to the
value returned by getTokenizer(), but Elasticsearch expects an analyzer name
(not a tokenizer), so replace that by either a fixed analyzer (e.g., 'standard')
or add a new method getSearchAnalyzer() that returns a valid analyzer name
(default 'standard' but configurable via env/setting) and update buildMappings()
to use getSearchAnalyzer() for all 'search_analyzer' entries instead of
$tokenizer; ensure getTokenizer() remains unchanged and only buildMappings() and
the new getSearchAnalyzer() (or hardcoded 'standard') are modified.
---
Outside diff comments:
In
`@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/AttachmentController.php`:
- Around line 41-42: The Route attribute on the delete action is using methods:
['GET'] while the controller method delete(Request $request): JsonResponse is
intended to handle DELETE requests; update the Route attribute (#[Route(path:
'content/attachments', name: 'admin.api.content.attachments', methods: [...])])
to use methods: ['DELETE'] so the route accepts DELETE requests and matches the
frontend behavior and REST conventions.
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/CategoryController.php`:
- Around line 76-86: The code references undefined local variables
$categoryPermission and $categoryImage inside the language-count conditional,
causing a fatal error; replace those references with the injected properties
used elsewhere (e.g. $this->categoryPermission->delete(Permission::USER, [(int)
$data->categoryId]); $this->categoryPermission->delete(Permission::GROUP, [(int)
$data->categoryId]); and $this->categoryImage->delete();) so the DI-backed
services (categoryPermission and categoryImage) are used consistently with the
existing refactor and avoid undefined variable errors while preserving the
existing delete calls.
In
`@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ElasticsearchController.php`:
- Around line 136-152: The healthcheck() method incorrectly fetches
Elasticsearch from the container using
$this->container->get('phpmyfaq.instance.elasticsearch'); replace that lookup
with the injected property $this->elasticsearch so healthcheck() calls
$this->elasticsearch->isAvailable() (consistent with create(), drop(), and
import()); remove the local $elasticsearch variable and use the injected
instance when building the response and status decision.
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php`:
- Around line 283-290: In getPopular() fix the missing return: when $result is
empty the method currently calls $this->json($result, Response::HTTP_NOT_FOUND)
without returning, so execution falls through and returns HTTP 200; change the
behavior by returning that response (add a return before $this->json) so the 404
is actually sent, mirroring getLatest() and getSticky().
- Around line 378-385: In getTrending() the 404 Response created by
$this->json($result, Response::HTTP_NOT_FOUND) is not returned, so the method
continues and always returns the 200 response; update getTrending() to return
the 404 response when $result is empty (i.e., change the conditional to return
$this->json(...)) so the method returns early with Response::HTTP_NOT_FOUND
instead of falling through to the final return of Response::HTTP_OK.
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/CaptchaController.php`:
- Around line 29-46: The constructor currently type-hints the static factory
Captcha which has no getCaptchaImage(); change the constructor signature in
CaptchaController::__construct to accept the CaptchaInterface (the interface
implemented by BuiltinCaptcha and GoogleRecaptcha) and store it as
$this->captcha so that the call to $this->captcha->getCaptchaImage() in
renderImage() uses the concrete service implementing getCaptchaImage(); update
any DI/config to inject the concrete CaptchaInterface implementation instead of
the Captcha factory.
---
Duplicate comments:
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/GroupController.php`:
- Line 111: Route name changed to follow api.{resource}.{action} convention (use
'api.groups.list' instead of 'api.groups'); update the Route attribute on
GroupController accordingly and replace all usages. Locate the Route attribute
on the GroupController class/method (#[Route(path: 'v3.2/groups', name:
'api.groups.list', methods: ['GET'])]) and ensure the name value is
'api.groups.list'; search the codebase for any references to the old
'api.groups' route name (route generation, tests, docs, and templates) and
update them to 'api.groups.list' so nothing breaks.
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/NewsController.php`:
- Line 119: The Route attribute in NewsController has been updated to follow the
api.{resource}.{action} convention (#[Route('/api/v3.2/news', name:
'api.news.list', methods: ['GET'])]) — confirm this pattern is applied
consistently by scanning other route attributes in the same class
(NewsController) and across API controllers and rename any remaining underscore
or non-conforming route names to the api.{resource}.{action} format using their
respective #[Route(..., name: '...')] attributes.
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/OpenQuestionController.php`:
- Around line 127-130: The Route naming/path issue in the OpenQuestionController
is already fixed: ensure the Route attribute on method list (#[Route(path:
'v3.2/open-questions', name: 'api.open-questions.list', methods: ['GET'])]) uses
the relative path and follows api.{resource}.{action} naming; no further code
changes are required for the list(Request $request) method since it now conforms
to the convention.
---
Nitpick comments:
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/CategoryController.php`:
- Line 107: Replace direct container lookups inside the permissions() and
updateOrder() methods with the already-injected properties: use
$this->categoryPermission instead of
$this->container->get('phpmyfaq.category.permission') in permissions(), and use
$this->categoryOrder instead of $this->container->get('phpmyfaq.category.order')
in updateOrder(); also update any subsequent local variable usages
($categoryOrder) in updateOrder() to reference $this->categoryOrder to keep
dependency access consistent with the constructor injection.
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/FaqController.php`:
- Around line 538-541: The list method currently accepts a nullable Request and
falls back to Request::createFromGlobals(), which bypasses the Kernel lifecycle;
change the signature of list to require a Request (remove the "?"/default null)
so Symfony's HttpKernel injects the proper Request and remove the fallback line
that calls Request::createFromGlobals(); update any callers/tests to pass a
Request to Api/FaqController::list or adjust tests to use the Kernel client to
obtain the routed Request instead of calling the controller directly.
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/NewsController.php`:
- Line 133: The controller currently instantiates News directly with "new
News($this->configuration)"; instead, change NewsController to receive the News
service via dependency injection (either add a __construct(News $news) storing
it on a private property or type-hint the News parameter on the action method),
remove the manual instantiation, and use the injected $this->news (or parameter)
everywhere; also ensure the News service is registered in the Symfony service
container (autowired or explicit service definition) so the controller receives
it automatically.
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/AutoCompleteController.php`:
- Line 50: The route name on the Route attribute in AutoCompleteController is
using api.private.autocomplete which violates the frontend naming convention;
update the Route attribute (the #[Route(path: 'autocomplete', name:
'api.private.autocomplete', methods: ['GET'])] on the AutoCompleteController) to
use the frontend convention public.{resource}.{action} (e.g.,
public.autocomplete.index or public.autocomplete.search as appropriate for this
endpoint) so the name follows public.{resource}.{action}.
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/CommentController.php`:
- Line 66: Route name on the CommentController route attribute uses
api.private.comment which breaks frontend naming rules; update the Route
attribute on CommentController (the #[Route(path: 'comment/create', name:
'api.private.comment', methods: ['POST'])] declaration) to use the frontend
convention, e.g. name: 'public.comment.create', and then search for and update
any usages of the old route name ('api.private.comment') in templates, redirects
or route references to the new name to keep callers working.
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/QuestionController.php`:
- Line 58: The route attribute on the QuestionController create endpoint uses
the wrong name; update the Route attribute on the method in
phpMyFAQ\Controller\Frontend\Api\QuestionController that currently has path
'question/create' and name 'api.private.question.create' to follow the frontend
convention (e.g. change the name to 'public.question.create') so it matches the
public.{resource}.{action} pattern used by Frontend controllers.
In `@phpmyfaq/src/phpMyFAQ/Controller/SitemapController.php`:
- Around line 111-116: Call CustomPage::getAllPages with activeOnly true from
SitemapController instead of fetching all pages and filtering in PHP: replace
the call to $this->customPage->getAllPages() in SitemapController::generate (or
the method containing the shown loop) with $this->customPage->getAllPages(true)
so the DB returns only active pages and remove the manual if ($page['active']
!== 'y') continue; check inside the foreach.
In `@phpmyfaq/src/phpMyFAQ/EventListener/LanguageListener.php`:
- Around line 94-102: In LanguageListener (around the Translation::create()
call) remove the redundant try-catch that catches Exception only to re-throw it;
instead invoke
Translation::create()->setTranslationsDir(PMF_TRANSLATION_DIR)->setDefaultLanguage('en')->setCurrentLanguage($currentLanguage)->setMultiByteLanguage();
directly (i.e., delete the catch block and surrounding try so exceptions bubble
naturally).
In `@phpmyfaq/src/phpMyFAQ/EventListener/WebExceptionListener.php`:
- Around line 116-131: The current handleServerError method uses error_log which
loses structure; instead obtain and use a PSR-3 logger (e.g. inject
Psr\Log\LoggerInterface into the class and store as $this->logger) and replace
the error_log call in handleServerError with a structured
$this->logger->error(...) call that includes the message and a context array
(e.g. ['exception' => $throwable, 'file' => $throwable->getFile(), 'line' =>
$throwable->getLine()]); ensure the class constructor or factory sets the logger
property and keep the existing return via handleErrorResponse unchanged.
In `@phpmyfaq/src/phpMyFAQ/Faq.php`:
- Around line 447-451: The config value records.numberOfRecordsPerPage is cast
to (int) later when passed to Pagination but is read earlier into $numPerPage
without casting, causing inconsistent types when $numPerPage is used in
arithmetic/comparisons; update the assignment that sets $numPerPage to cast the
configuration read (e.g., $numPerPage = (int) $this->configuration->get(item:
'records.numberOfRecordsPerPage')) so all uses of $numPerPage are integer-safe
and consistent with the Pagination call.
In `@phpmyfaq/src/services.php`:
- Around line 710-714: The batch comment above the RootSitemapController service
registration is mislabeled "Batch 6"; update that comment to "Batch 4" (or the
appropriate next sequential batch) to match the surrounding batch numbering;
locate the comment immediately preceding the RootSitemapController::class
registration in the services->set(...) block and change only the comment text so
it reflects the correct batch number.
In
`@tests/phpMyFAQ/Controller/Administration/Api/ElasticsearchControllerTest.php`:
- Around line 50-57: Remove the unused $request = new Request() statement from
all test methods in ElasticsearchControllerTest (e.g.,
testCreateRequiresAuthentication, testDropRequiresAuthentication,
testImportRequiresAuthentication, testStatisticsRequiresAuthentication,
testHealthcheckRequiresAuthentication) since ElasticsearchController::create(),
::drop(), ::import(), ::statistics(), and ::healthcheck() do not accept a
Request and the variable is dead code; simply delete the $request line from each
test to eliminate the unused variable warnings.
In `@tests/phpMyFAQ/Controller/Api/FaqControllerTest.php`:
- Around line 56-61: Add a private createController() helper in the
FaqControllerTest that returns a new FaqController constructed with the four
stubs; locate the repeated inline instantiations of FaqController (using
createStub(Faq::class), createStub(Tags::class),
createStub(FaqStatistics::class), createStub(FaqMetaData::class)) and replace
them with $this->createController(). Ensure the helper method name is
createController and it constructs the FaqController the same way so all test
methods call $this->createController() instead of duplicating the constructor
logic.
In `@tests/phpMyFAQ/Controller/Api/GlossaryControllerTest.php`:
- Around line 127-147: The test name testListReturnsEmptyArrayOn404 is
misleading because it asserts Response::HTTP_OK; rename the test method to
reflect actual behavior (e.g., testListReturnsSuccessWithDataStructure or
testListReturnsEmptyArrayWithOkStatus) and update any references; keep the body
unchanged (using GlossaryController->list($request), assertions on status code
and json structure) so the method name matches the behavior.
- Around line 1-3: Add the missing strict types directive to this test file:
insert declare(strict_types=1); immediately after the opening <?php tag and
before the namespace declaration (namespace phpMyFAQ\Controller\Api;) in
GlossaryControllerTest.php so it matches the other tests in the PR.
In `@tests/phpMyFAQ/Controller/Api/SearchControllerTest.php`:
- Around line 17-153: Extract a private helper method createController() in this
test class that returns a SearchController constructed with
$this->createStub(Search::class), replace all inline constructions like new
SearchController($this->createStub(Search::class)) with calls to
createController(), and update tests to use that helper (keep existing method
names: SearchController, createStub, and Search) to reduce duplication and match
the pattern used in UserControllerTest/ContactControllerTest.
In `@tests/phpMyFAQ/Controller/SitemapControllerTest.php`:
- Line 19: The test class SitemapControllerTest declares a private Environment
$twig property and creates an Environment stub in the test setup but neither the
$twig property nor that stub are passed to SitemapController or used in
assertions, so remove the dead test code: delete the private Environment $twig
declaration and the Environment stub creation in the test (references to $twig
and the Environment::class stub inside SitemapControllerTest and its setUp), and
also remove the unused Environment import so imports and setup only include
objects actually passed to SitemapController or asserted in tests.
In `@tests/phpMyFAQ/EventListener/WebExceptionListenerTest.php`:
- Around line 103-119: The test testHandlesGenericException temporarily changes
the PHP error_log setting with ini_set('error_log', '/dev/null') but restores it
only after listener->onKernelException and can leak if an exception occurs; wrap
the ini_set restore in a try/finally so the original ini_get('error_log') value
is always restored (capture originalErrorLog before the try, call
$this->listener->onKernelException($event) inside the try, and call
ini_set('error_log', $originalErrorLog) in the finally) to ensure error_log is
reset even on failures.
phpmyfaq/src/phpMyFAQ/Controller/Frontend/AbstractFrontController.php
Outdated
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/ContactController.php
Outdated
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/UserController.php
Outdated
Show resolved
Hide resolved
phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/VotingController.php
Outdated
Show resolved
Hide resolved
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 (2)
phpmyfaq/src/phpMyFAQ/Controller/Frontend/AbstractFrontController.php (1)
170-170:⚠️ Potential issue | 🟡 MinorPER CS 3.0 violation: closing
*/and method signature on the same line.The doc-block closing tag and
private functiondeclaration are joined on one line. As per coding guidelines, PER Coding Style 3.0 requires the method signature to start on its own line.- */ private function getUserDropdown(): array + */ + private function getUserDropdown(): array🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/AbstractFrontController.php` at line 170, The docblock closing tag and the method declaration for getUserDropdown() in AbstractFrontController are on the same line; separate them so the closing "*/" ends the docblock on its own line and the "private function getUserDropdown(): array" signature begins on the next line to comply with PER Coding Style 3.0 (update the docblock above getUserDropdown in class AbstractFrontController accordingly).phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/UserController.php (1)
205-212:⚠️ Potential issue | 🟠 MajorMissing authentication guard in
requestUserRemoval.Both
updateData(Line 55) andexportUserData(Line 144) call$this->userIsAuthenticated()before accessing$this->currentUser, butrequestUserRemovaldoes not. It still relies on$this->currentUserat lines 231–234 for identity validation. If an unauthenticated request reaches this endpoint, the identity checks may behave unpredictably.Proposed fix
public function requestUserRemoval(Request $request): JsonResponse { + $this->userIsAuthenticated(); + $data = json_decode($request->getContent());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/UserController.php` around lines 205 - 212, The requestUserRemoval method lacks an authentication guard: call $this->userIsAuthenticated() near the start of requestUserRemoval (as done in updateData and exportUserData) before reading $this->currentUser; if authentication fails return the same unauthorized response/exception the other methods use so the subsequent identity checks around $this->currentUser (lines referencing identity validation) never run for unauthenticated requests.
🧹 Nitpick comments (4)
tests/phpMyFAQ/Controller/Api/NewsControllerTest.php (1)
79-89: Minor observation: theelsebranch duplicates coverage fromtestListReturnsValidStatusCode.The assertion on line 87 (
assertEquals(HTTP_OK, ...)) is effectively already covered by line 51'sassertContains(..., [HTTP_OK, HTTP_NOT_FOUND]). Not harmful, but the else branch doesn't add much signal — if you wanted a stricter contract you could replace the wholeif/elsewith a direct assertion on the decoded content based on status.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Controller/Api/NewsControllerTest.php` around lines 79 - 89, The test testListReturnsEmptyArrayOn404 duplicates coverage in its else branch; simplify it by removing the else branch and only asserting the decoded response when Response::HTTP_NOT_FOUND is returned from NewsController::list($this->createRequest()), or alternatively replace the whole conditional with a single assertion that the status is one of Response::HTTP_OK or Response::HTTP_NOT_FOUND and, if HTTP_NOT_FOUND, assert json_decode($response->getContent(), true) equals []; update the assertions in NewsControllerTest::testListReturnsEmptyArrayOn404 accordingly.phpmyfaq/src/phpMyFAQ/Instance/Search/Elasticsearch.php (2)
172-179:getTokenizer()lacks the empty-string guard thatgetSearchAnalyzer()has.If
PMF_ELASTICSEARCH_TOKENIZERis defined as an empty string,(string) constant(...)will return'', which Elasticsearch will reject.getSearchAnalyzer()already handles this correctly — apply the same pattern here for consistency.Proposed fix
private function getTokenizer(): string { if (defined('PMF_ELASTICSEARCH_TOKENIZER')) { - return (string) constant('PMF_ELASTICSEARCH_TOKENIZER'); + $tokenizer = constant('PMF_ELASTICSEARCH_TOKENIZER'); + if (is_string($tokenizer) && $tokenizer !== '') { + return $tokenizer; + } } return 'standard'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Instance/Search/Elasticsearch.php` around lines 172 - 179, The getTokenizer() method should guard against an empty PMF_ELASTICSEARCH_TOKENIZER value like getSearchAnalyzer() does; change the logic in getTokenizer() to read the constant, cast to string, and if the resulting value is empty return the default 'standard' instead of the raw empty string. Update the function that references PMF_ELASTICSEARCH_TOKENIZER to first assign (string) constant('PMF_ELASTICSEARCH_TOKENIZER') to a variable, check for empty($value) and return 'standard' when empty, otherwise return the token value.
181-197: Consider guarding against non-positive shard/replica values.
getNumberOfShards()/getNumberOfReplicas()cast the constant tointwithout validation. A misconfiguredPMF_ELASTICSEARCH_NUMBER_SHARDS = 0(or negative) would produce an invalid index. Amax(1, ...)for shards andmax(0, ...)for replicas would be a simple safeguard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Instance/Search/Elasticsearch.php` around lines 181 - 197, The getters getNumberOfShards() and getNumberOfReplicas() currently cast constants directly to int; update them to validate and clamp values so shards are at least 1 and replicas are non-negative: when PMF_ELASTICSEARCH_NUMBER_SHARDS is defined, cast to int and return max(1, thatValue) in getNumberOfShards(); when PMF_ELASTICSEARCH_NUMBER_REPLICAS is defined, cast to int and return max(0, thatValue) in getNumberOfReplicas(); keep the existing defaults (2 shards, 0 replicas) when constants are not defined.phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/FaqController.php (1)
65-65: Route nameapi.private.faq.createdoesn't follow thepublic.{resource}.{action}frontend convention.This controller lives under
Controller/Frontend/Api/, so per the coding guidelines, frontend routes should followpublic.{resource}.{action}. The current nameapi.private.faq.createdeviates from that. If this is intentionally a private/authenticated API endpoint, consider moving it outside theFrontendnamespace, or document why it diverges from the convention. As per coding guidelines: "Frontend routes should follow the naming convention: public.{resource}.{action}".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/FaqController.php` at line 65, The route name on the attribute for FaqController (#[Route(path: 'faq/create', name: 'api.private.faq.create', methods: ['POST'])]) violates the frontend naming convention; update the route name to follow public.{resource}.{action} (e.g., public.faq.create) or, if this is meant to be a private/authenticated API, move the action out of Controller/Frontend/Api/FaqController into an appropriate backend/API controller namespace and keep a private naming pattern there; adjust any references to the route name throughout the codebase accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/AbstractFrontController.php`:
- Around line 85-92: The code dereferences nullable properties $this->faqSystem
and $this->seo (used for getSystemUri(...) and getMetaRobots(...)) without
guarantees they are set; update AbstractFrontController to guard or assert their
presence: either make initialization mandatory (remove nullable types and ensure
initializeFromContainer() is invoked before use) or add explicit null checks
where used (e.g., before calling
$this->faqSystem->getSystemUri($this->configuration) and
$this->seo->getMetaRobots($action)) and handle the missing dependency (throw a
clear exception or provide a fallback). Ensure references to
initializeFromContainer(), $this->faqSystem, and $this->seo are updated
consistently.
- Around line 44-51: After calling parent::initializeFromContainer() in
AbstractFrontController::initializeFromContainer, guard against a null container
before accessing it: check that $this->container !== null (or use a null-safe
early return) prior to calling $this->container->get(...) for 'phpmyfaq.system'
and 'phpmyfaq.seo'; this prevents a null-pointer when the parent returned early
and ensures $this->faqSystem and $this->seo are only set when the container is
available.
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/VotingController.php`:
- Line 46: The route name in the Route attribute on VotingController (the
#[Route(path: 'voting', name: 'api.private.voting', methods: ['POST'])]
annotation) violates frontend naming conventions; change the name to follow
public.{resource}.{action} (e.g. 'public.voting.create') by updating the Route
attribute in the VotingController method, and then update any code, templates,
or tests that reference 'api.private.voting' to use the new
'public.voting.create' name.
---
Outside diff comments:
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/AbstractFrontController.php`:
- Line 170: The docblock closing tag and the method declaration for
getUserDropdown() in AbstractFrontController are on the same line; separate them
so the closing "*/" ends the docblock on its own line and the "private function
getUserDropdown(): array" signature begins on the next line to comply with PER
Coding Style 3.0 (update the docblock above getUserDropdown in class
AbstractFrontController accordingly).
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/UserController.php`:
- Around line 205-212: The requestUserRemoval method lacks an authentication
guard: call $this->userIsAuthenticated() near the start of requestUserRemoval
(as done in updateData and exportUserData) before reading $this->currentUser; if
authentication fails return the same unauthorized response/exception the other
methods use so the subsequent identity checks around $this->currentUser (lines
referencing identity validation) never run for unauthenticated requests.
---
Duplicate comments:
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/GlossaryController.php`:
- Around line 126-130: The guard previously checked for !== false and has been
correctly changed to !== ''; ensure the conditional around $currentLanguage
returned from $this->language->setLanguageByAcceptLanguage() remains a strict
empty-string check and that $this->glossary->setLanguage($currentLanguage) is
only called when non-empty; no further code changes needed to the
setLanguageByAcceptLanguage / setLanguage usage in GlossaryController.
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/FaqController.php`:
- Around line 143-148: The banned-word check for $answer currently uses
!$this->stopWords->checkBannedWord(strip_tags($answer)) to trigger a 400
response; verify that checkBannedWord's return semantics match this inversion
(i.e., it returns true when the text is clean and false when banned) and if not,
invert the condition or adjust the call so that FaqController::... (the block
handling $answer) returns Response::HTTP_BAD_REQUEST only when the stripped
$answer contains banned words.
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/UserController.php`:
- Around line 261-266: The duplicate setReplyTo issue has been resolved; ensure
the mailer usage in UserController's try block keeps a single setReplyTo($email,
$author), adds the admin recipient via
addTo($this->configuration->getAdminEmail()), sets subject via
$this->configuration->getTitle() . ': Remove User Request', assigns the message
to $this->mailer->message, and calls $this->mailer->send(); no further changes
required unless a regression reintroduces the duplicate setReplyTo call.
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/VotingController.php`:
- Around line 77-96: The updated voting control flow in
VotingController::saveVoting (the block using $this->rating->check($faqId,
$userIp), $this->rating->create/update and the json responses) is correct and
removes redundant/unreachable branches; no code changes required—leave the
simplified if/else as-is, ensure the methods check, create, update,
getNumberOfVotings and get on $this->rating and userTracking on
$this->userSession remain covered by tests and logging.
---
Nitpick comments:
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/FaqController.php`:
- Line 65: The route name on the attribute for FaqController (#[Route(path:
'faq/create', name: 'api.private.faq.create', methods: ['POST'])]) violates the
frontend naming convention; update the route name to follow
public.{resource}.{action} (e.g., public.faq.create) or, if this is meant to be
a private/authenticated API, move the action out of
Controller/Frontend/Api/FaqController into an appropriate backend/API controller
namespace and keep a private naming pattern there; adjust any references to the
route name throughout the codebase accordingly.
In `@phpmyfaq/src/phpMyFAQ/Instance/Search/Elasticsearch.php`:
- Around line 172-179: The getTokenizer() method should guard against an empty
PMF_ELASTICSEARCH_TOKENIZER value like getSearchAnalyzer() does; change the
logic in getTokenizer() to read the constant, cast to string, and if the
resulting value is empty return the default 'standard' instead of the raw empty
string. Update the function that references PMF_ELASTICSEARCH_TOKENIZER to first
assign (string) constant('PMF_ELASTICSEARCH_TOKENIZER') to a variable, check for
empty($value) and return 'standard' when empty, otherwise return the token
value.
- Around line 181-197: The getters getNumberOfShards() and getNumberOfReplicas()
currently cast constants directly to int; update them to validate and clamp
values so shards are at least 1 and replicas are non-negative: when
PMF_ELASTICSEARCH_NUMBER_SHARDS is defined, cast to int and return max(1,
thatValue) in getNumberOfShards(); when PMF_ELASTICSEARCH_NUMBER_REPLICAS is
defined, cast to int and return max(0, thatValue) in getNumberOfReplicas(); keep
the existing defaults (2 shards, 0 replicas) when constants are not defined.
In `@tests/phpMyFAQ/Controller/Api/NewsControllerTest.php`:
- Around line 79-89: The test testListReturnsEmptyArrayOn404 duplicates coverage
in its else branch; simplify it by removing the else branch and only asserting
the decoded response when Response::HTTP_NOT_FOUND is returned from
NewsController::list($this->createRequest()), or alternatively replace the whole
conditional with a single assertion that the status is one of Response::HTTP_OK
or Response::HTTP_NOT_FOUND and, if HTTP_NOT_FOUND, assert
json_decode($response->getContent(), true) equals []; update the assertions in
NewsControllerTest::testListReturnsEmptyArrayOn404 accordingly.
Comment on lines
+44
to
+51
| #[\Override] | ||
| protected function initializeFromContainer(): void | ||
| { | ||
| parent::initializeFromContainer(); | ||
|
|
||
| $this->faqSystem = $this->container->get(id: 'phpmyfaq.system'); | ||
| $this->seo = $this->container->get(id: 'phpmyfaq.seo'); | ||
| } |
There was a problem hiding this comment.
Null-safety gap: $this->container may be null when child continues after parent::initializeFromContainer().
The parent's initializeFromContainer() returns early if $this->container === null, but that return only exits the parent method. Execution resumes here at line 49, where $this->container->get(...) will throw an NPE.
Proposed fix
#[\Override]
protected function initializeFromContainer(): void
{
parent::initializeFromContainer();
+ if ($this->container === null) {
+ return;
+ }
+
$this->faqSystem = $this->container->get(id: 'phpmyfaq.system');
$this->seo = $this->container->get(id: 'phpmyfaq.seo');
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/AbstractFrontController.php` around
lines 44 - 51, After calling parent::initializeFromContainer() in
AbstractFrontController::initializeFromContainer, guard against a null container
before accessing it: check that $this->container !== null (or use a null-safe
early return) prior to calling $this->container->get(...) for 'phpmyfaq.system'
and 'phpmyfaq.seo'; this prevents a null-pointer when the parent returned early
and ensures $this->faqSystem and $this->seo are only set when the container is
available.
Comment on lines
+85
to
+92
| 'baseHref' => $this->faqSystem->getSystemUri($this->configuration), | ||
| 'customCss' => $this->configuration->getCustomCss(), | ||
| 'version' => $this->configuration->getVersion(), | ||
| 'header' => str_replace('"', '', $this->configuration->getTitle()), | ||
| 'metaDescription' => $metaDescription ?? $this->configuration->get('seo.description'), | ||
| 'metaDescription' => $this->configuration->get('seo.description'), | ||
| 'metaPublisher' => $this->configuration->get('main.metaPublisher'), | ||
| 'metaLanguage' => Translation::get(key: 'metaLanguage'), | ||
| 'metaRobots' => $seo->getMetaRobots($action), | ||
| 'metaRobots' => $this->seo->getMetaRobots($action), |
There was a problem hiding this comment.
Nullable properties $faqSystem and $seo are dereferenced without null checks.
Both $this->faqSystem (line 85) and $this->seo (line 92) are typed ?System / ?Seo and default to null. If initializeFromContainer() hasn't run (or the container lacked those entries), these calls will NPE at runtime. Consider either removing the nullable types (if initialization is guaranteed) or adding guards / assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/AbstractFrontController.php` around
lines 85 - 92, The code dereferences nullable properties $this->faqSystem and
$this->seo (used for getSystemUri(...) and getMetaRobots(...)) without
guarantees they are set; update AbstractFrontController to guard or assert their
presence: either make initialization mandatory (remove nullable types and ensure
initializeFromContainer() is invoked before use) or add explicit null checks
where used (e.g., before calling
$this->faqSystem->getSystemUri($this->configuration) and
$this->seo->getMetaRobots($action)) and handle the missing dependency (throw a
clear exception or provide a fallback). Ensure references to
initializeFromContainer(), $this->faqSystem, and $this->seo are updated
consistently.
| /** | ||
| * @throws Exception | ||
| */ | ||
| #[Route(path: 'voting', name: 'api.private.voting', methods: ['POST'])] |
There was a problem hiding this comment.
Route name violates the frontend naming convention.
This controller lives under Controller/Frontend/Api/ but the route is named api.private.voting. Per the coding guidelines, frontend routes should follow the public.{resource}.{action} pattern — e.g. public.voting.create.
- #[Route(path: 'voting', name: 'api.private.voting', methods: ['POST'])]
+ #[Route(path: 'voting', name: 'public.voting.create', methods: ['POST'])]As per coding guidelines, "Frontend routes should follow the naming convention: public.{resource}.{action}".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/Api/VotingController.php` at line
46, The route name in the Route attribute on VotingController (the #[Route(path:
'voting', name: 'api.private.voting', methods: ['POST'])] annotation) violates
frontend naming conventions; change the name to follow
public.{resource}.{action} (e.g. 'public.voting.create') by updating the Route
attribute in the VotingController method, and then update any code, templates,
or tests that reference 'api.private.voting' to use the new
'public.voting.create' name.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Chores
Bug Fixes
Tests
Fixes