Conversation
📝 WalkthroughWalkthroughThis PR replaces the custom Application bootstrap with a Symfony Kernel-based architecture across all entry points, adds Symfony testing dev-dependencies, introduces a Kernel, multiple event listeners, controller/container integration, and updates controllers and tests to the new kernel lifecycle. The old Application class and its tests were removed. Changes
Sequence DiagramssequenceDiagram
participant Client
participant EntryPoint
participant Kernel
participant RouterListener
participant LanguageListener
participant ControllerContainerListener
participant Controller
participant ApiExceptionListener
participant WebExceptionListener
participant Response
Client->>EntryPoint: HTTP request
EntryPoint->>Kernel: new Kernel(routingContext, debug)
EntryPoint->>Kernel: handle(Request::createFromGlobals())
activate Kernel
Kernel->>RouterListener: dispatch KernelEvents::REQUEST
RouterListener->>Kernel: set _controller/_route (if matched)
Kernel->>LanguageListener: dispatch KernelEvents::REQUEST
LanguageListener->>Kernel: initialize language/translation
Kernel->>ControllerContainerListener: dispatch KernelEvents::CONTROLLER
ControllerContainerListener->>Controller: setContainer(...) (if controller is AbstractController)
Kernel->>Controller: invoke controller
alt controller succeeds
Controller-->>Kernel: Response
else controller throws
Controller-->>Kernel: Exception
Kernel->>ApiExceptionListener: dispatch KernelEvents::EXCEPTION (for API)
ApiExceptionListener-->>Kernel: ProblemDetails JSON Response
Kernel->>WebExceptionListener: dispatch KernelEvents::EXCEPTION (for web)
WebExceptionListener-->>Kernel: HTML/redirect Response
end
Kernel-->>EntryPoint: Response
EntryPoint->>Response: send()
deactivate Kernel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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.
| @@ -108,16 +109,22 @@ final class GroupController extends AbstractApiController | |||
| ))] | |||
| #[OA\Response(response: 401, description: 'If the user is not authenticated.')] | |||
| #[Route(path: 'v3.2/groups', name: 'api.groups', methods: ['GET'])] | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check all API route names to see the pattern
rg -n "name: 'api\." --type=php -g '**/Controller/Api/**'Repository: thorsten/phpMyFAQ
Length of output: 4218
Route name api.groups does not follow the api.{resource}.{action} convention.
Per the coding guidelines, API routes should be named api.{resource}.{action}. This should be api.groups.list to be consistent with similar list endpoints like api.categories.list, api.faqs.list, and api.glossary.list.
Proposed fix
- #[Route(path: 'v3.2/groups', name: 'api.groups', methods: ['GET'])]
+ #[Route(path: 'v3.2/groups', name: 'api.groups.list', methods: ['GET'])]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[Route(path: 'v3.2/groups', name: 'api.groups', methods: ['GET'])] | |
| #[Route(path: 'v3.2/groups', name: 'api.groups.list', methods: ['GET'])] |
🤖 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` at 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.
| @@ -116,11 +117,14 @@ enum: ['id', 'datum', 'header', 'author_name'], | |||
| }'), | |||
| )] | |||
| #[Route('/api/v3.2/news', name: 'api_news_list', methods: ['GET'])] | |||
There was a problem hiding this comment.
Route name api_news_list does not follow the api.{resource}.{action} naming convention.
Should be api.news.list (dot-separated) to match the convention used elsewhere (e.g., api.tags in TagController).
- #[Route('/api/v3.2/news', name: 'api_news_list', methods: ['GET'])]
+ #[Route('/api/v3.2/news', name: 'api.news.list', methods: ['GET'])]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/NewsController.php` at 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.
| @@ -118,23 +131,22 @@ enum: ['id', 'username', 'created', 'categoryId'], | |||
| }', | |||
| ))] | |||
| #[Route('/api/v3.2/open-questions', name: 'api_open_questions', methods: ['GET'])] | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Route definitions in OpenQuestionController.php ==="
cat -n phpmyfaq/src/phpMyFAQ/Controller/Api/OpenQuestionController.php | head -150
echo ""
echo "=== All route names in API controllers ==="
rg '#\[Route\(' --type=php -A 1 phpmyfaq/src/phpMyFAQ/Controller/Api/ | grep -E "(Route|name:)" | head -50
echo ""
echo "=== Check CategoryController routes ==="
rg '#\[Route\(' --type=php -A 1 phpmyfaq/src/phpMyFAQ/Controller/Api/CategoryController.php
echo ""
echo "=== Check QuestionController routes ==="
rg '#\[Route\(' --type=php -A 1 phpmyfaq/src/phpMyFAQ/Controller/Api/QuestionController.phpRepository: thorsten/phpMyFAQ
Length of output: 11230
Route name violates the api.{resource}.{action} naming convention.
The name api_open_questions uses underscores instead of dots. The codebase consistently uses dot-separated names (e.g., api.categories.list, api.question.create, api.faqs.popular). Additionally, the route path here is absolute (/api/v3.2/open-questions) while most other API controllers use relative paths (e.g., v3.2/categories in CategoryController).
Proposed fix for the route name and path
- #[Route('/api/v3.2/open-questions', name: 'api_open_questions', methods: ['GET'])]
+ #[Route(path: 'v3.2/open-questions', name: 'api.open-questions.list', methods: ['GET'])]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[Route('/api/v3.2/open-questions', name: 'api_open_questions', methods: ['GET'])] | |
| #[Route(path: 'v3.2/open-questions', name: 'api.open-questions.list', methods: ['GET'])] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/OpenQuestionController.php` at 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.
| @@ -106,23 +120,25 @@ final class TagController extends AbstractApiController | |||
| }', | |||
| ))] | |||
| #[Route('/api/v3.2/tags', name: 'api.tags', methods: ['GET'])] | |||
There was a problem hiding this comment.
Route name api.tags is missing the action segment.
Per the naming convention api.{resource}.{action}, this should be api.tags.list.
- #[Route('/api/v3.2/tags', name: 'api.tags', methods: ['GET'])]
+ #[Route('/api/v3.2/tags', name: 'api.tags.list', methods: ['GET'])]As per coding guidelines: "API routes should follow the naming convention: api.{resource}.{action}"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[Route('/api/v3.2/tags', name: 'api.tags', methods: ['GET'])] | |
| #[Route('/api/v3.2/tags', name: 'api.tags.list', methods: ['GET'])] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@phpmyfaq/src/phpMyFAQ/Controller/Api/TagController.php` at 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.
| require PMF_TRANSLATION_DIR . '/language_en.php'; | ||
| if (Language::isASupportedLanguage($currentLanguage)) { | ||
| require PMF_TRANSLATION_DIR . '/language_' . strtolower($currentLanguage) . '.php'; | ||
| } |
There was a problem hiding this comment.
require instead of require_once — risk of redeclaration errors if language files are loaded elsewhere.
If any other code path (legacy bootstrap, another listener, or a test) also loads these language files, require will cause a fatal error due to redeclared constants/functions. Using require_once would be safer during the transition to the Kernel-based architecture.
Proposed fix
- require PMF_TRANSLATION_DIR . '/language_en.php';
+ require_once PMF_TRANSLATION_DIR . '/language_en.php';
if (Language::isASupportedLanguage($currentLanguage)) {
- require PMF_TRANSLATION_DIR . '/language_' . strtolower($currentLanguage) . '.php';
+ require_once PMF_TRANSLATION_DIR . '/language_' . strtolower($currentLanguage) . '.php';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| require PMF_TRANSLATION_DIR . '/language_en.php'; | |
| if (Language::isASupportedLanguage($currentLanguage)) { | |
| require PMF_TRANSLATION_DIR . '/language_' . strtolower($currentLanguage) . '.php'; | |
| } | |
| require_once PMF_TRANSLATION_DIR . '/language_en.php'; | |
| if (Language::isASupportedLanguage($currentLanguage)) { | |
| require_once PMF_TRANSLATION_DIR . '/language_' . strtolower($currentLanguage) . '.php'; | |
| } |
🤖 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 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.
| try { | ||
| $request->attributes->set('_route', 'public.404'); | ||
| $request->attributes->set('_controller', PageNotFoundController::class . '::index'); | ||
| $controllerResolver = new ControllerResolver(); | ||
| $argumentResolver = new ArgumentResolver(); | ||
| $controller = $controllerResolver->getController($request); | ||
| $arguments = $argumentResolver->getArguments($request, $controller); | ||
| return call_user_func_array($controller, $arguments); | ||
| } catch (Throwable) { | ||
| return $this->handleErrorResponse( | ||
| 'Not Found: :message at line :line at :file', | ||
| 'Not Found', | ||
| Response::HTTP_NOT_FOUND, | ||
| $throwable, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
PageNotFoundController likely requires container injection to render the 404 page.
PageNotFoundController::index accesses $this->container->get('phpmyfaq.user.session') and calls $this->render(), both of which depend on the container being set. When invoked directly via ControllerResolver + ArgumentResolver here, the container won't be injected, so this will almost certainly throw and fall through to the catch block every time.
The fallback in the catch block works, but it returns a plain-text response instead of the styled 404 page. If the intent is to always use the fallback, the PageNotFoundController invocation is dead code. If the intent is to render the styled page, consider passing the container to the controller or delegating via a sub-request through the kernel.
🤖 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 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.
| public function testSetContainerReEvaluatesIsSecuredWhenContainerChanges(): void | ||
| { | ||
| $controller = new class() extends AbstractController {}; | ||
|
|
||
| $session = $this->createMock(SessionInterface::class); | ||
|
|
||
| $firstConfiguration = $this->createMock(Configuration::class); | ||
| $firstConfiguration->expects($this->once())->method('getTemplateSet')->willReturn('default'); | ||
|
|
||
| $firstCurrentUser = $this->createMock(CurrentUser::class); | ||
| $firstCurrentUser->expects($this->once())->method('isLoggedIn')->willReturn(true); | ||
|
|
||
| $firstContainer = $this->createMock(ContainerInterface::class); | ||
| $firstContainer | ||
| ->method('get') | ||
| ->willReturnCallback(static function (string $id) use ($firstConfiguration, $firstCurrentUser, $session) { | ||
| return match ($id) { | ||
| 'phpmyfaq.configuration' => $firstConfiguration, | ||
| 'phpmyfaq.user.current_user' => $firstCurrentUser, | ||
| 'session' => $session, | ||
| default => throw new \InvalidArgumentException(sprintf('Unexpected service id "%s".', $id)), | ||
| }; | ||
| }); | ||
|
|
||
| $controller->setContainer($firstContainer); | ||
|
|
||
| $secondConfiguration = $this->createMock(Configuration::class); | ||
| $secondConfiguration->expects($this->once())->method('getTemplateSet')->willReturn('default'); | ||
| $secondConfiguration->expects($this->once())->method('get')->with('security.enableLoginOnly')->willReturn(true); | ||
|
|
||
| $secondCurrentUser = $this->createMock(CurrentUser::class); | ||
| $secondCurrentUser->expects($this->once())->method('isLoggedIn')->willReturn(false); | ||
|
|
||
| $secondContainer = $this->createMock(ContainerInterface::class); | ||
| $secondContainer | ||
| ->method('get') | ||
| ->willReturnCallback(static function (string $id) use ($secondConfiguration, $secondCurrentUser, $session) { | ||
| return match ($id) { | ||
| 'phpmyfaq.configuration' => $secondConfiguration, | ||
| 'phpmyfaq.user.current_user' => $secondCurrentUser, | ||
| 'session' => $session, | ||
| default => throw new \InvalidArgumentException(sprintf('Unexpected service id "%s".', $id)), | ||
| }; | ||
| }); | ||
|
|
||
| $this->expectException(UnauthorizedHttpException::class); | ||
| $controller->setContainer($secondContainer); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the setUp method and the surrounding context
sed -n '40,60p' tests/phpMyFAQ/Controller/AbstractControllerTest.phpRepository: thorsten/phpMyFAQ
Length of output: 969
🏁 Script executed:
# Also check the actual test method to see its full context
sed -n '450,510p' tests/phpMyFAQ/Controller/AbstractControllerTest.phpRepository: thorsten/phpMyFAQ
Length of output: 2878
🏁 Script executed:
# Search for references to "Skip parent constructor" to verify the claim
rg -n 'Skip parent constructor' tests/phpMyFAQ/Controller/AbstractControllerTest.php -B3 -A3Repository: thorsten/phpMyFAQ
Length of output: 402
🏁 Script executed:
# Find the AbstractController class file
fd -type f -name "AbstractController.php" | grep -v TestRepository: thorsten/phpMyFAQ
Length of output: 231
🏁 Script executed:
# Once found, examine the constructor
rg -n '__construct' src/Controller/AbstractController.php -A10Repository: thorsten/phpMyFAQ
Length of output: 138
🏁 Script executed:
# Search for AbstractController.php in the entire repository
find . -name "AbstractController.php" -type f | head -20Repository: thorsten/phpMyFAQ
Length of output: 118
🏁 Script executed:
# Also check the namespace and use statements in the test file to understand where it comes from
head -30 tests/phpMyFAQ/Controller/AbstractControllerTest.phpRepository: thorsten/phpMyFAQ
Length of output: 1160
🏁 Script executed:
# Examine the AbstractController class constructor
sed -n '1,100p' phpmyfaq/src/phpMyFAQ/Controller/AbstractController.phpRepository: thorsten/phpMyFAQ
Length of output: 3434
🏁 Script executed:
# Look for the __construct method specifically
rg -n '__construct' phpmyfaq/src/phpMyFAQ/Controller/AbstractController.php -A20Repository: thorsten/phpMyFAQ
Length of output: 720
🏁 Script executed:
# Check the createContainer method
rg -n 'protected function createContainer' phpmyfaq/src/phpMyFAQ/Controller/AbstractController.php -A15Repository: thorsten/phpMyFAQ
Length of output: 643
🏁 Script executed:
# Also check initializeFromContainer to see what dependencies it tries to get
rg -n 'protected function initializeFromContainer' phpmyfaq/src/phpMyFAQ/Controller/AbstractController.php -A20Repository: thorsten/phpMyFAQ
Length of output: 872
🏁 Script executed:
# Check if services.php exists in the expected location
find . -name "services.php" -type fRepository: thorsten/phpMyFAQ
Length of output: 88
🏁 Script executed:
# Let's also check if there are other tests in the same file that use the same pattern
rg -n 'new class.*extends AbstractController' tests/phpMyFAQ/Controller/AbstractControllerTest.phpRepository: thorsten/phpMyFAQ
Length of output: 129
🏁 Script executed:
# Check the PER Coding Style 3.0 requirements for indentation and formatting
# Look at a few lines to verify style compliance
sed -n '457,465p' tests/phpMyFAQ/Controller/AbstractControllerTest.php | cat -ARepository: thorsten/phpMyFAQ
Length of output: 464
Override the constructor to avoid fallback container initialization issues.
Line 459 calls new class() extends AbstractController {} which invokes the parent constructor. This will call createContainer() → load services.php → then initializeFromContainer() attempts to resolve services like phpmyfaq.configuration. However, the setUp() method (lines 46-53) in this same test file explicitly documents this pattern as problematic with the comment "Skip parent constructor completely to avoid container issues."
This test should follow the same pattern: override __construct() to skip parent initialization and let setContainer() handle the container setup.
Proposed fix
public function testSetContainerReEvaluatesIsSecuredWhenContainerChanges(): void
{
- $controller = new class() extends AbstractController {};
+ $controller = new class() extends AbstractController {
+ public function __construct()
+ {
+ // Skip parent constructor completely to avoid container issues
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testSetContainerReEvaluatesIsSecuredWhenContainerChanges(): void | |
| { | |
| $controller = new class() extends AbstractController {}; | |
| $session = $this->createMock(SessionInterface::class); | |
| $firstConfiguration = $this->createMock(Configuration::class); | |
| $firstConfiguration->expects($this->once())->method('getTemplateSet')->willReturn('default'); | |
| $firstCurrentUser = $this->createMock(CurrentUser::class); | |
| $firstCurrentUser->expects($this->once())->method('isLoggedIn')->willReturn(true); | |
| $firstContainer = $this->createMock(ContainerInterface::class); | |
| $firstContainer | |
| ->method('get') | |
| ->willReturnCallback(static function (string $id) use ($firstConfiguration, $firstCurrentUser, $session) { | |
| return match ($id) { | |
| 'phpmyfaq.configuration' => $firstConfiguration, | |
| 'phpmyfaq.user.current_user' => $firstCurrentUser, | |
| 'session' => $session, | |
| default => throw new \InvalidArgumentException(sprintf('Unexpected service id "%s".', $id)), | |
| }; | |
| }); | |
| $controller->setContainer($firstContainer); | |
| $secondConfiguration = $this->createMock(Configuration::class); | |
| $secondConfiguration->expects($this->once())->method('getTemplateSet')->willReturn('default'); | |
| $secondConfiguration->expects($this->once())->method('get')->with('security.enableLoginOnly')->willReturn(true); | |
| $secondCurrentUser = $this->createMock(CurrentUser::class); | |
| $secondCurrentUser->expects($this->once())->method('isLoggedIn')->willReturn(false); | |
| $secondContainer = $this->createMock(ContainerInterface::class); | |
| $secondContainer | |
| ->method('get') | |
| ->willReturnCallback(static function (string $id) use ($secondConfiguration, $secondCurrentUser, $session) { | |
| return match ($id) { | |
| 'phpmyfaq.configuration' => $secondConfiguration, | |
| 'phpmyfaq.user.current_user' => $secondCurrentUser, | |
| 'session' => $session, | |
| default => throw new \InvalidArgumentException(sprintf('Unexpected service id "%s".', $id)), | |
| }; | |
| }); | |
| $this->expectException(UnauthorizedHttpException::class); | |
| $controller->setContainer($secondContainer); | |
| } | |
| public function testSetContainerReEvaluatesIsSecuredWhenContainerChanges(): void | |
| { | |
| $controller = new class() extends AbstractController { | |
| public function __construct() | |
| { | |
| // Skip parent constructor completely to avoid container issues | |
| } | |
| }; | |
| $session = $this->createMock(SessionInterface::class); | |
| $firstConfiguration = $this->createMock(Configuration::class); | |
| $firstConfiguration->expects($this->once())->method('getTemplateSet')->willReturn('default'); | |
| $firstCurrentUser = $this->createMock(CurrentUser::class); | |
| $firstCurrentUser->expects($this->once())->method('isLoggedIn')->willReturn(true); | |
| $firstContainer = $this->createMock(ContainerInterface::class); | |
| $firstContainer | |
| ->method('get') | |
| ->willReturnCallback(static function (string $id) use ($firstConfiguration, $firstCurrentUser, $session) { | |
| return match ($id) { | |
| 'phpmyfaq.configuration' => $firstConfiguration, | |
| 'phpmyfaq.user.current_user' => $firstCurrentUser, | |
| 'session' => $session, | |
| default => throw new \InvalidArgumentException(sprintf('Unexpected service id "%s".', $id)), | |
| }; | |
| }); | |
| $controller->setContainer($firstContainer); | |
| $secondConfiguration = $this->createMock(Configuration::class); | |
| $secondConfiguration->expects($this->once())->method('getTemplateSet')->willReturn('default'); | |
| $secondConfiguration->expects($this->once())->method('get')->with('security.enableLoginOnly')->willReturn(true); | |
| $secondCurrentUser = $this->createMock(CurrentUser::class); | |
| $secondCurrentUser->expects($this->once())->method('isLoggedIn')->willReturn(false); | |
| $secondContainer = $this->createMock(ContainerInterface::class); | |
| $secondContainer | |
| ->method('get') | |
| ->willReturnCallback(static function (string $id) use ($secondConfiguration, $secondCurrentUser, $session) { | |
| return match ($id) { | |
| 'phpmyfaq.configuration' => $secondConfiguration, | |
| 'phpmyfaq.user.current_user' => $secondCurrentUser, | |
| 'session' => $session, | |
| default => throw new \InvalidArgumentException(sprintf('Unexpected service id "%s".', $id)), | |
| }; | |
| }); | |
| $this->expectException(UnauthorizedHttpException::class); | |
| $controller->setContainer($secondContainer); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/phpMyFAQ/Controller/AbstractControllerTest.php` around lines 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.
1 participant
Summary by CodeRabbit
Chores
Bug Fixes
Tests
Fixes