Added functional test suite with optimizations for bundle#15
Merged
Conversation
WalkthroughThis PR expands Symfony compatibility to ^7.3 || ^8.0, replaces autowire/autoconfigure defaults with explicit DI wiring across configs, adjusts several service constructor arguments and priorities, adds defensive checks, and introduces a full functional test suite with fixtures, kernel, and OpenAPI spec. Changes
sequenceDiagram
actor Client
participant Kernel
participant Controller as CommandController
participant Validator
participant CommandBus as MessageBus
participant Handler
participant Responder
participant Serializer
Client->>Kernel: HTTP request (e.g. POST /api/books)
Kernel->>Controller: route -> invoke CommandController
Controller->>Serializer: deserialize request -> command object
Controller->>Validator: validate(command)
Validator-->>Controller: validation result
Controller->>CommandBus: dispatch(command)
CommandBus->>Handler: deliver command to handler (__invoke)
Handler-->>CommandBus: result (BookResource)
Controller->>Responder: create Response from result
Responder->>Serializer: serialize payload (if needed)
Responder-->>Controller: Response
Controller-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Controller/ArgumentResolver/CommandValueResolver.php (1)
113-121:⚠️ Potential issue | 🟠 MajorPotential TypeError when array key is an integer.
The filter callback accesses
$key[0]without verifying that$keyis a string. While$request->attributes->all()typically returns string keys, PHP arrays can have integer keys. If a non-string key exists,$key[0]will throw aTypeErrorin PHP 8.x.🛡️ Proposed fix to add string type guard
$routeData = array_filter( $attributes, static function ($value, $key): bool { - return $key !== '' + return is_string($key) + && $key !== '' && $key[0] !== '_' && (is_scalar($value) || $value === null); }, ARRAY_FILTER_USE_BOTH );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Controller/ArgumentResolver/CommandValueResolver.php` around lines 113 - 121, The callback used to build $routeData in CommandValueResolver.php assumes $key is a string and uses $key[0], which can throw a TypeError for integer keys; update the closure (the static function used with ARRAY_FILTER_USE_BOTH that filters $attributes) to first check is_string($key) and that $key !== '' before accessing $key[0] (i.e., require is_string($key) && $key !== '' && $key[0] !== '_' && (is_scalar($value) || $value === null)).
🧹 Nitpick comments (4)
tests/Functional/App/Kernel.php (2)
40-52: PHPDoc is redundant with parameter type.The
@param RoutingConfigurator $routesannotation duplicates the type information that's already available from the type check on line 47. Consider removing the docblock or relying solely on the instanceof check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Functional/App/Kernel.php` around lines 40 - 52, The PHPDoc above configureRoutes duplicates the type information and is redundant; remove the docblock comment containing "@param RoutingConfigurator $routes" and rely on the existing method signature and the instanceof check inside configureRoutes (refer to the configureRoutes method and the RoutingConfigurator type check, and the route imports for CreateBookCommand, UpdateBookCommand, DeleteBookCommand) so the file no longer contains the redundant annotation.
23-23: Consider making theKernelclassfinal.Per coding guidelines, classes not intended for extension (especially tests) should be
final. If this test kernel isn't extended elsewhere, adding thefinalkeyword would align with the project standards.-class Kernel extends TestKernel +final class Kernel extends TestKernelAs per coding guidelines: "Classes not intended for extension (especially tests) must be
final."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Functional/App/Kernel.php` at line 23, The Kernel class should be marked final since it's not intended for extension; update the class declaration for Kernel (which currently extends TestKernel) to add the final keyword (i.e., declare final class Kernel extends TestKernel) so it follows the project's guideline that test-only classes are final.tests/Functional/Resources/specifications/openapi.json (1)
1-6: Consider adding meaningfulinfometadata.The empty
titleand placeholderversion"0.0.0" work for tests but could be more descriptive for documentation purposes.The Checkov warnings about missing global security definitions (CKV_OPENAPI_4, CKV_OPENAPI_5) can be safely ignored for this test fixture since authentication is not part of the functional test scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Functional/Resources/specifications/openapi.json` around lines 1 - 6, The OpenAPI document's info block has an empty "title" and a placeholder "version"; update the "info" object so "title" contains a meaningful name (e.g. describing this test API) and "version" reflects a proper semantic version (not "0.0.0"), keeping the rest of the spec unchanged; reference the "openapi" top-level field and the "info" object's "title" and "version" keys when making the change (you can ignore Checkov warnings CKV_OPENAPI_4/5 for this test fixture).tests/Functional/ScenarioTest.php (1)
109-170: Consolidate duplicated validation-error scenarios with#[DataProvider].These two tests are identical except for the HTTP method and URI; parameterizing them will keep assertions in one place and reduce drift. Per the coding guidelines, use
#[DataProvider]for multiple similar scenarios.Note: The
DataProviderattribute will need to be imported fromPHPUnit\Framework\Attributes.♻️ Proposed refactor
+use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\WithoutErrorHandler; - #[WithoutErrorHandler] - public function testCreateBookValidationError(): void - { - // Arrange - $kernel = $this->createKernelWithConfig(static function (Kernel $kernel): void { - $kernel->addTestConfig(__DIR__.'/Resources/config/scenario.php'); - }); - - $payload = [ - 'title' => '', // NotBlank - ]; - - $request = Request::create( - uri: '/api/books', - method: 'POST', - content: json_encode($payload, JSON_THROW_ON_ERROR) - ); - $request->headers->set('Content-Type', 'application/json'); - - // Act - $response = $kernel->handle($request); - - // Assert - self::assertSame(400, $response->getStatusCode()); - self::assertSame('application/problem+json', $response->headers->get('Content-Type')); - $data = json_decode($response->getContent() ?: 'null', true, 512, JSON_THROW_ON_ERROR); - self::assertIsArray($data); - self::assertSame(400, $data['status'] ?? null); - self::assertSame('Validation failed', $data['detail'] ?? null); - self::assertArrayHasKey('violations', $data); - } - - #[WithoutErrorHandler] - public function testUpdateBookValidationError(): void + #[WithoutErrorHandler] + #[DataProvider('validationErrorProvider')] + public function testValidationError(string $uri, string $method): void { // Arrange $kernel = $this->createKernelWithConfig(static function (Kernel $kernel): void { $kernel->addTestConfig(__DIR__.'/Resources/config/scenario.php'); }); @@ - uri: '/api/books/1', - method: 'PUT', + uri: $uri, + method: $method, content: json_encode($payload, JSON_THROW_ON_ERROR) ); @@ self::assertSame('Validation failed', $data['detail'] ?? null); self::assertArrayHasKey('violations', $data); } + + public static function validationErrorProvider(): array + { + return [ + 'create' => ['/api/books', 'POST'], + 'update' => ['/api/books/1', 'PUT'], + ]; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Functional/ScenarioTest.php` around lines 109 - 170, The two tests testCreateBookValidationError and testUpdateBookValidationError are duplicated; replace them with a single parameterized test (e.g., testValidationError) that accepts method and URI from a DataProvider (e.g., validationErrorProvider) and add the PHPUnit\Framework\Attributes\DataProvider import; keep the #[WithoutErrorHandler] attribute on the test, move the shared Arrange/Act/Assert logic into the single test that builds the Request using the provided method and URI, and implement validationErrorProvider to yield the two cases (['POST','/api/books'] and ['PUT','/api/books/1']) so all assertions remain in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Functional/BundleInitializationTest.php`:
- Around line 89-95: Add an explicit "Arrange" comment above the setup block in
testBundleWithCustomConfiguration: mark the initial kernel/config creation (the
call to $this->createKernelWithConfig(...) and the Kernel->addTestConfig(...)
invocation) with a comment "// Arrange" so the test follows the AAA convention
before the existing Act/Assert comments.
In `@tests/Functional/Resources/specifications/openapi.json`:
- Around line 27-51: The PUT operation with operationId
"put_command_updatebookcommand" (PUT /api/books/{id}) is missing a requestBody
definition; add a required requestBody object describing the update payload and
reference the appropriate schema (e.g., "#/components/schemas/BookResource" or a
dedicated UpdateBook schema) under content "application/json", and ensure
requestBody.required is true so the API expects the payload for the update.
- Around line 8-24: The POST operation for "/api/books" (operationId:
post_command_createbookcommand) is missing a requestBody; add a requestBody
object with "required": true and "content": { "application/json": { "schema": {
"$ref": "#/components/schemas/BookCreateRequest" } } } (or reference an
appropriate existing schema such as BookResource if creation and response share
the same shape) so the OpenAPI spec documents the payload expected by create
book and satisfies validation in tests.
---
Outside diff comments:
In `@src/Controller/ArgumentResolver/CommandValueResolver.php`:
- Around line 113-121: The callback used to build $routeData in
CommandValueResolver.php assumes $key is a string and uses $key[0], which can
throw a TypeError for integer keys; update the closure (the static function used
with ARRAY_FILTER_USE_BOTH that filters $attributes) to first check
is_string($key) and that $key !== '' before accessing $key[0] (i.e., require
is_string($key) && $key !== '' && $key[0] !== '_' && (is_scalar($value) ||
$value === null)).
---
Nitpick comments:
In `@tests/Functional/App/Kernel.php`:
- Around line 40-52: The PHPDoc above configureRoutes duplicates the type
information and is redundant; remove the docblock comment containing "@param
RoutingConfigurator $routes" and rely on the existing method signature and the
instanceof check inside configureRoutes (refer to the configureRoutes method and
the RoutingConfigurator type check, and the route imports for CreateBookCommand,
UpdateBookCommand, DeleteBookCommand) so the file no longer contains the
redundant annotation.
- Line 23: The Kernel class should be marked final since it's not intended for
extension; update the class declaration for Kernel (which currently extends
TestKernel) to add the final keyword (i.e., declare final class Kernel extends
TestKernel) so it follows the project's guideline that test-only classes are
final.
In `@tests/Functional/Resources/specifications/openapi.json`:
- Around line 1-6: The OpenAPI document's info block has an empty "title" and a
placeholder "version"; update the "info" object so "title" contains a meaningful
name (e.g. describing this test API) and "version" reflects a proper semantic
version (not "0.0.0"), keeping the rest of the spec unchanged; reference the
"openapi" top-level field and the "info" object's "title" and "version" keys
when making the change (you can ignore Checkov warnings CKV_OPENAPI_4/5 for this
test fixture).
In `@tests/Functional/ScenarioTest.php`:
- Around line 109-170: The two tests testCreateBookValidationError and
testUpdateBookValidationError are duplicated; replace them with a single
parameterized test (e.g., testValidationError) that accepts method and URI from
a DataProvider (e.g., validationErrorProvider) and add the
PHPUnit\Framework\Attributes\DataProvider import; keep the
#[WithoutErrorHandler] attribute on the test, move the shared Arrange/Act/Assert
logic into the single test that builds the Request using the provided method and
URI, and implement validationErrorProvider to yield the two cases
(['POST','/api/books'] and ['PUT','/api/books/1']) so all assertions remain in
one place.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
composer.jsonconfig/controller.phpconfig/openapi.phpconfig/response.phpconfig/routing.phpconfig/serializer.phpconfig/subscribers.phpconfig/validators.phpphpunit.xmlsrc/Controller/ArgumentResolver/CommandValueResolver.phpsrc/DependencyInjection/StixxOpenApiCommandExtension.phpsrc/EventSubscriber/ApiExceptionSubscriber.phpsrc/Response/ResponseStatusResolver.phptests/Functional/.gitignoretests/Functional/AbstractKernelTestCase.phptests/Functional/App/Command/CreateBookCommand.phptests/Functional/App/Command/DeleteBookCommand.phptests/Functional/App/Command/UpdateBookCommand.phptests/Functional/App/Handler/CreateBookHandler.phptests/Functional/App/Handler/DeleteBookHandler.phptests/Functional/App/Handler/UpdateBookHandler.phptests/Functional/App/Kernel.phptests/Functional/App/Model/BookResource.phptests/Functional/BundleInitializationTest.phptests/Functional/OpenApiSchemaTest.phptests/Functional/Resources/config/custom.phptests/Functional/Resources/config/default.phptests/Functional/Resources/config/scenario.phptests/Functional/Resources/specifications/openapi.jsontests/Functional/ScenarioTest.php
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/Functional/BundleInitializationTest.php (1)
34-86: Consider adding explicit// Actcomment for AAA consistency.The test follows the Arrange-Assert pattern (which is valid for state verification tests), but the coding guidelines require explicit AAA comments. The "act" here is implicitly the kernel boot. For consistency with
testBundleWithCustomConfiguration, consider adding a brief// Actsection.📝 Suggested adjustment
public function testInitBundle(): void { // Arrange $this->bootKernel(); + + // Act $container = self::getContainer(); // AssertAs per coding guidelines, "Tests must follow the AAA (Arrange, Act, Assert) pattern with explicit comments."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Functional/BundleInitializationTest.php` around lines 34 - 86, The testInitBundle method currently mixes Arrange and the implicit action (kernel boot); add an explicit "// Act" comment and move or place the call to bootKernel() (the action) under that comment so the test follows Arrange (setup), Act (bootKernel()), Assert (the container assertions) explicitly—mirror the pattern used in testBundleWithCustomConfiguration and keep the method name testInitBundle and calls like $this->bootKernel() and self::getContainer() unchanged.tests/Functional/Resources/specifications/openapi.json (2)
1-144: Note: Missing global security definitions (static analysis).Checkov flags missing global
securityfield (CKV_OPENAPI_4, CKV_OPENAPI_5). For a functional test fixture, omitting security is likely intentional to simplify testing. If this spec is also used to validate security-aware behavior, consider adding a placeholder security definition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Functional/Resources/specifications/openapi.json` around lines 1 - 144, The OpenAPI spec lacks a global security declaration which triggers CKV_OPENAPI_4/5; add a top-level "security" field (e.g., an empty array if you want no enforced schemes for tests, or a placeholder like [{"ApiKeyAuth": []}]) and, if using a placeholder, add a matching entry under "components.securitySchemes" (e.g., "ApiKeyAuth" with type and name) so tools recognize global security; update the root object to include "security" and, when adding a scheme, modify "components" -> "securitySchemes" to include the referenced scheme name.
3-6: Consider populating API info metadata.The
titleis empty andversionis "0.0.0". While acceptable for a test fixture, adding meaningful values would improve test documentation and could help catch regressions if schema generation is being validated.📝 Suggested improvement
"info": { - "title": "", - "version": "0.0.0" + "title": "Book API", + "version": "1.0.0" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Functional/Resources/specifications/openapi.json` around lines 3 - 6, The OpenAPI fixture has empty/placeholder metadata: the "info" object's "title" is empty and "version" is "0.0.0"; update the "info.title" to a meaningful string (e.g., a short API name) and set "info.version" to a realistic semantic version (e.g., "1.0.0") to improve test documentation and schema validation; ensure you update the "info" object where "title" and "version" are defined so tests using the openapi.json pick up the new metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/Functional/BundleInitializationTest.php`:
- Around line 34-86: The testInitBundle method currently mixes Arrange and the
implicit action (kernel boot); add an explicit "// Act" comment and move or
place the call to bootKernel() (the action) under that comment so the test
follows Arrange (setup), Act (bootKernel()), Assert (the container assertions)
explicitly—mirror the pattern used in testBundleWithCustomConfiguration and keep
the method name testInitBundle and calls like $this->bootKernel() and
self::getContainer() unchanged.
In `@tests/Functional/Resources/specifications/openapi.json`:
- Around line 1-144: The OpenAPI spec lacks a global security declaration which
triggers CKV_OPENAPI_4/5; add a top-level "security" field (e.g., an empty array
if you want no enforced schemes for tests, or a placeholder like [{"ApiKeyAuth":
[]}]) and, if using a placeholder, add a matching entry under
"components.securitySchemes" (e.g., "ApiKeyAuth" with type and name) so tools
recognize global security; update the root object to include "security" and,
when adding a scheme, modify "components" -> "securitySchemes" to include the
referenced scheme name.
- Around line 3-6: The OpenAPI fixture has empty/placeholder metadata: the
"info" object's "title" is empty and "version" is "0.0.0"; update the
"info.title" to a meaningful string (e.g., a short API name) and set
"info.version" to a realistic semantic version (e.g., "1.0.0") to improve test
documentation and schema validation; ensure you update the "info" object where
"title" and "version" are defined so tests using the openapi.json pick up the
new metadata.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/Functional/App/Command/CreateBookCommand.phptests/Functional/App/Command/UpdateBookCommand.phptests/Functional/App/Model/BookRequest.phptests/Functional/BundleInitializationTest.phptests/Functional/Resources/specifications/openapi.json
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Functional/App/Command/UpdateBookCommand.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests