Refactor attribute definition request handling #161
Conversation
📝 WalkthroughWalkthroughThis pull request consolidates attribute definition request handlers across the Identity and Subscription domains. It unifies separate create/update request classes into single unified request classes ( Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Validator
participant RequestDTO
participant Repository
participant Response
Client->>Controller: POST /api/v2/attributes (JSON payload)
Controller->>Validator: validate(request, SubscriberAttributeDefinitionRequest)
Validator->>RequestDTO: validateType() callback
RequestDTO->>Validator: AttributeTypeValidator.validate(type)
alt Type is valid
Validator-->>Controller: validation passes
Controller->>RequestDTO: getDto() returns AttributeDefinitionDto
Controller->>Repository: create/update attributeDefinition
Repository-->>Controller: AttributeDefinition persisted
Controller-->>Response: 201/204 response
Response-->>Client: Success
else Type is invalid
Validator-->>Controller: validation error
Controller-->>Response: 422 Unprocessable Entity
Response-->>Client: Validation error with message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Subscription/OpenApi/SwaggerSchemasResponse.php (1)
88-105: AttributeDefinition schema is out of sync with the normalizerRight now the
AttributeDefinitionschema still documentstable_name, and now also anoptionsarray, but theAttributeDefinitionNormalizerno longer emitstable_nameand doesn’t includeoptionsat all.That gives clients a misleading contract:
table_nameappears in the OpenAPI spec but is never present in responses.optionsis documented but won’t show up until serialization is updated.I’d recommend:
- Dropping the
table_nameproperty from this schema, and- Either adding
optionstoAttributeDefinitionNormalizeror removing it from the schema until it’s actually returned.
🧹 Nitpick comments (3)
src/Subscription/Request/UpdateSubscriberRequest.php (1)
7-26: Align OpenAPIrequiredvs PHP property nullabilityThe schema only requires
confirmed,blacklisted,html_email,disabled, andadditional_dataare mapped to non-nullable typed properties (bool/string) without defaults inUpdateSubscriberRequest. If clients follow the docs and omit these fields on update, you risk uninitialized typed properties at runtime.Either:
- mark these fields as required in the OA schema, or
- make the PHP properties nullable / give safe defaults so partial updates are actually supported.
src/Identity/OpenApi/SwaggerSchemasRequest.php (1)
100-117: AdminAttributeDefinitionRequest schema looks good, with a small type nitNice reuse of
AttributeTypeEnumfor thetypeenum; that keeps docs and domain in sync.One small consistency tweak:
orderis documented astype: 'number'here, while similar fields (order/list_order) are integers elsewhere. You might want to switch this totype: 'integer'for a crisper contract and to match the underlying PHP/int type.src/Subscription/Request/SubscriberAttributeDefinitionRequest.php (1)
18-61: Solid request DTO overall; just double-check options hydration and enum conversionThis request class is nicely structured: OA schema, validation, and DTO mapping all line up with the AttributeTypeEnum-based design and the new options support.
A couple of things worth sanity-checking:
optionselement type vs hydration
optionsis declared as?arrayand validated withAssert\Type(['type' => DynamicListAttrDto::class])for each item. That assumes the deserialization layer is already instantiatingDynamicListAttrDtoobjects from the incoming JSON. If it actually leaves them as plain arrays, this constraint will always fail and$optionswill never validate. If there isn’t already a custom denormalizer in place, consider either:
- mapping raw arrays to
DynamicListAttrDtoinstances ingetDto(), or- relaxing the constraint to validate array shape instead of object type.
AttributeTypeEnum::from()safety
getDto()usesAttributeTypeEnum::from($this->type)which will throw on an unknown string. As long as every code path validates the request (and thus runsvalidateType()) before callinggetDto(), that’s fine. If there’s any chancegetDto()might be called on an unvalidated instance (e.g., in tests or internal helpers), you might preferAttributeTypeEnum::tryFrom()plus a validation error, or reuseAttributeTypeValidatorhere to avoid hard exceptions.Nothing blocking, but tightening these up would make this DTO a bit more robust.
Also applies to: 74-111
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
README.md(1 hunks)composer.json(1 hunks)config/services/validators.yml(1 hunks)src/Identity/Controller/AdminAttributeDefinitionController.php(5 hunks)src/Identity/OpenApi/SwaggerSchemasRequest.php(2 hunks)src/Identity/OpenApi/SwaggerSchemasResponse.php(1 hunks)src/Identity/Request/AdminAttributeDefinitionRequest.php(1 hunks)src/Identity/Request/UpdateAttributeDefinitionRequest.php(0 hunks)src/Subscription/Controller/SubscriberAttributeDefinitionController.php(5 hunks)src/Subscription/OpenApi/SwaggerSchemasRequest.php(0 hunks)src/Subscription/OpenApi/SwaggerSchemasResponse.php(1 hunks)src/Subscription/Request/CreateAttributeDefinitionRequest.php(0 hunks)src/Subscription/Request/CreateSubscriberListRequest.php(1 hunks)src/Subscription/Request/CreateSubscriberRequest.php(1 hunks)src/Subscription/Request/SubscribePageRequest.php(1 hunks)src/Subscription/Request/SubscriberAttributeDefinitionRequest.php(1 hunks)src/Subscription/Request/SubscribersExportRequest.php(2 hunks)src/Subscription/Request/UpdateAttributeDefinitionRequest.php(0 hunks)src/Subscription/Request/UpdateSubscriberRequest.php(1 hunks)src/Subscription/Serializer/AttributeDefinitionNormalizer.php(1 hunks)tests/Integration/Identity/Controller/AdminAttributeDefinitionControllerTest.php(2 hunks)tests/Integration/Identity/Fixtures/AdminAttributeDefinitionFixture.php(0 hunks)tests/Integration/Subscription/Controller/SubscriberAttributeDefinitionControllerTest.php(0 hunks)tests/Integration/Subscription/Controller/SubscriberImportControllerTest.php(1 hunks)tests/Integration/Subscription/Fixtures/SubscriberAttributeDefinitionFixture.php(2 hunks)tests/Integration/Subscription/Fixtures/SubscriberAttributeValueFixture.php(2 hunks)tests/Unit/Identity/Request/AdminAttributeDefinitionRequestTest.php(2 hunks)tests/Unit/Identity/Request/UpdateAttributeDefinitionRequestTest.php(0 hunks)tests/Unit/Subscription/Request/CreateAttributeDefinitionRequestTest.php(0 hunks)tests/Unit/Subscription/Request/SubscriberAttributeDefinitionRequestTest.php(1 hunks)tests/Unit/Subscription/Request/UpdateAttributeDefinitionRequestTest.php(0 hunks)tests/Unit/Subscription/Serializer/AttributeDefinitionNormalizerTest.php(2 hunks)
💤 Files with no reviewable changes (9)
- tests/Integration/Identity/Fixtures/AdminAttributeDefinitionFixture.php
- src/Identity/Request/UpdateAttributeDefinitionRequest.php
- tests/Integration/Subscription/Controller/SubscriberAttributeDefinitionControllerTest.php
- src/Subscription/Request/UpdateAttributeDefinitionRequest.php
- src/Subscription/OpenApi/SwaggerSchemasRequest.php
- tests/Unit/Identity/Request/UpdateAttributeDefinitionRequestTest.php
- src/Subscription/Request/CreateAttributeDefinitionRequest.php
- tests/Unit/Subscription/Request/UpdateAttributeDefinitionRequestTest.php
- tests/Unit/Subscription/Request/CreateAttributeDefinitionRequestTest.php
🧰 Additional context used
🧬 Code graph analysis (11)
tests/Unit/Subscription/Request/SubscriberAttributeDefinitionRequestTest.php (1)
src/Subscription/Request/SubscriberAttributeDefinitionRequest.php (1)
getDto(83-94)
src/Subscription/OpenApi/SwaggerSchemasResponse.php (1)
src/Subscription/Request/SubscriberAttributeDefinitionRequest.php (1)
OA(18-112)
src/Subscription/Request/SubscriberAttributeDefinitionRequest.php (3)
src/Subscription/OpenApi/SwaggerSchemasResponse.php (1)
OA(9-171)src/Messaging/Request/CreateBounceRegexRequest.php (1)
Assert(44-62)src/Identity/Request/AdminAttributeDefinitionRequest.php (1)
getDto(21-30)
src/Subscription/Request/UpdateSubscriberRequest.php (2)
src/Subscription/OpenApi/SwaggerSchemasResponse.php (1)
OA(9-171)src/Subscription/Request/CreateSubscriberRequest.php (1)
OA(14-45)
src/Subscription/Request/CreateSubscriberListRequest.php (2)
src/Subscription/OpenApi/SwaggerSchemasResponse.php (1)
OA(9-171)src/Subscription/Request/CreateSubscriberRequest.php (1)
OA(14-45)
tests/Unit/Identity/Request/AdminAttributeDefinitionRequestTest.php (1)
src/Identity/Request/AdminAttributeDefinitionRequest.php (1)
AdminAttributeDefinitionRequest(11-31)
src/Identity/Controller/AdminAttributeDefinitionController.php (2)
src/Identity/Request/AdminAttributeDefinitionRequest.php (1)
AdminAttributeDefinitionRequest(11-31)src/Identity/OpenApi/SwaggerSchemasRequest.php (1)
OA(10-122)
src/Identity/OpenApi/SwaggerSchemasResponse.php (2)
src/Identity/OpenApi/SwaggerSchemasRequest.php (1)
OA(10-122)src/Subscription/OpenApi/SwaggerSchemasResponse.php (1)
OA(9-171)
src/Subscription/Request/CreateSubscriberRequest.php (3)
src/Identity/OpenApi/SwaggerSchemasRequest.php (1)
OA(10-122)src/Subscription/Request/CreateSubscriberListRequest.php (1)
OA(12-44)src/Subscription/Request/UpdateSubscriberRequest.php (1)
OA(14-63)
src/Subscription/Controller/SubscriberAttributeDefinitionController.php (1)
src/Subscription/Request/SubscriberAttributeDefinitionRequest.php (1)
OA(18-112)
src/Identity/OpenApi/SwaggerSchemasRequest.php (2)
src/Identity/OpenApi/SwaggerSchemasResponse.php (1)
OA(9-48)src/Subscription/Request/SubscriberAttributeDefinitionRequest.php (1)
OA(18-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
- GitHub Check: Checkout phpList rest-api and generate docs specification (OpenAPI latest-restapi.json)
- GitHub Check: generate-openapi
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
🔇 Additional comments (23)
tests/Integration/Subscription/Controller/SubscriberImportControllerTest.php (2)
209-241: LGTM! Better user feedback on skipped emails.The updated assertion correctly expects the error message for the skipped invalid email, which provides helpful feedback to users about what was skipped and why.
243-262: Verify that invalid emails should be importable when skip_invalid_emails is false.The test expects that when
skip_invalid_emailsis false, the syntactically invalid email "invalid-email" is successfully imported with no errors. This could lead to data quality issues if invalid emails are stored in the system.Please confirm whether this is the intended behavior or if email validation should still occur regardless of the
skip_invalid_emailsflag.tests/Integration/Subscription/Fixtures/SubscriberAttributeDefinitionFixture.php (1)
10-19: LGTM! Consistent enum migration.The changes mirror the previous fixture file and correctly use
AttributeTypeEnum::Checkboxinstead of the string literal. This maintains consistency across the test fixtures and leverages the type safety benefits of the enum.tests/Integration/Subscription/Fixtures/SubscriberAttributeValueFixture.php (1)
10-21: Code changes are correct and enum migration is complete for this fixture.The fixture properly uses
AttributeTypeEnum::Checkboxinstead of string literals. Verification across the codebase confirms no remaining string literalsetTypecalls forSubscriberAttributeDefinitionfixtures.src/Subscription/Request/SubscribePageRequest.php (1)
16-17: Verified: No breaking changes detected in current codebase.The change from
?bool $activetobool $active = falsewas investigated thoroughly:
By default, Symfony Serializer adds null to nullable properties when parameters aren't provided, so the deserialization behavior does change—omitted fields now default to
falseinstead ofnull.No null checks found: The codebase search found zero occurrences of null-checking on the
$activeproperty, indicating no code relies on distinguishing "not provided" from "explicitly false."Usage pattern safe: The property is passed directly to
createPage()andupdatePage()methods without conditional logic, suggesting the consuming code handlesboolvalues correctly.Validation design confirms intent: The absence of
NotBlankorNotNullconstraints (onlyType(bool)) suggests the field was intentionally made optional with a default value rather than required.The change is intentional and appears safe for the current codebase, though the semantic shift (null → false for omitted fields) should be verified in integration/acceptance tests to confirm the business logic aligns with this default.
composer.json (1)
45-45: Verify the phplist/core branch reference.The dependency shifted from
dev-maintodev-dev. In Composer, this means switching from themainbranch to a branch nameddev. While unconventional (most projects usedevelopordevelopment), confirm that this branch exists in the phpList/core repository and that the change aligns with your refactoring goals.README.md (1)
41-41: LGTM! Documentation link updated to hosted docs.The change to an externally hosted documentation URL is clear and improves accessibility for users.
src/Identity/OpenApi/SwaggerSchemasResponse.php (1)
30-30: LGTM! Example value now matches allowed enum types.The type example is now consistent with the enum values defined in AdminAttributeDefinitionRequest (TextLine, Hidden).
src/Subscription/Request/CreateSubscriberRequest.php (1)
7-23: LGTM! OpenAPI schema annotation added.The schema accurately documents the request structure without modifying runtime behavior.
src/Subscription/Request/CreateSubscriberListRequest.php (1)
7-22: LGTM! OpenAPI schema annotation added.The schema accurately documents the request structure without modifying runtime behavior.
src/Subscription/Request/SubscribersExportRequest.php (2)
8-61: LGTM! Comprehensive OpenAPI schema annotation added.The schema thoroughly documents the export request structure including all filtering options and column specifications.
120-129: LGTM! Improved DTO construction with named arguments.Using named arguments instead of positional parameters improves code maintainability and reduces the risk of argument ordering errors.
tests/Unit/Subscription/Serializer/AttributeDefinitionNormalizerTest.php (2)
7-7: LGTM! Added enum import.Necessary import for the AttributeTypeEnum usage in the test.
30-46: LGTM! Test updated for enum-based types and table_name removal.The test correctly reflects:
- Domain model now returns AttributeTypeEnum::Text
- Normalizer converts enum to string 'text' for API responses
- Removal of table_name field from serialized output
src/Identity/Request/AdminAttributeDefinitionRequest.php (1)
11-30: Refactoring verified and complete.The consolidation of Create/Update requests into AdminAttributeDefinitionRequest has been successfully implemented throughout the codebase. The old class names (CreateAttributeDefinitionRequest and UpdateAttributeDefinitionRequest) are no longer referenced anywhere. The new unified class is properly integrated across:
- Both create and update controller actions (lines 89-90 and 155-156 in AdminAttributeDefinitionController)
- Test suite (AdminAttributeDefinitionRequestTest)
- OpenAPI/Swagger documentation (SwaggerSchemasRequest)
- DTO conversion logic (getDto() method returns AdminAttributeDefinitionDto correctly)
All references have been updated and there are no lingering dependencies on the old class names.
config/services/validators.yml (1)
40-42: Add missing validator tag and fix namespace mismatch.The review comment is correct:
AttributeTypeValidatorat line 40 is missing thetags: ['validator.constraint_validator']entry that all other validators in this file have (lines 7-38). The pattern is consistent—every constraint validator here includes this tag for Symfony's automatic registration.Additionally, there's a namespace inconsistency: the config declares the service at
PhpList\Core\Domain\Identity\Validator\AttributeTypeValidator, but the code imports and uses it fromPhpList\Core\Domain\Subscription\Validator\AttributeTypeValidator(insrc/Subscription/Request/SubscriberAttributeDefinitionRequest.php). Verify which namespace is correct and align them.PhpList\Core\Domain\Identity\Validator\AttributeTypeValidator: autowire: true autoconfigure: true tags: [ 'validator.constraint_validator' ]src/Subscription/Serializer/AttributeDefinitionNormalizer.php (1)
21-28: Enum-backed type normalization is solidUsing
$object->getType() ? $object->getType()->value : nullcleanly bridges the enum-backedgetType()to the string API field while staying null-safe. No issues from my side here.tests/Integration/Identity/Controller/AdminAttributeDefinitionControllerTest.php (1)
30-47: Tests now correctly reflect enum-backedtypevaluesThe create/update tests using
'textline'and'hidden'fortypematch the new Admin attribute definition contract and keep request/response expectations in sync. Looks good.Also applies to: 49-65
src/Subscription/OpenApi/SwaggerSchemasResponse.php (1)
106-114: DynamicListAttrOption schema looks fineThe new
DynamicListAttrOptionschema (id, name, listorder) is straightforward and matches the intent for response-side option representations. No issues here from my end.tests/Unit/Identity/Request/AdminAttributeDefinitionRequestTest.php (1)
7-45: Unit tests correctly exercise the new AdminAttributeDefinitionRequestBoth tests line up with
AdminAttributeDefinitionRequest::getDto()—covering explicit values and defaulted/null fields—so they give good protection over the mapping logic after the refactor.src/Identity/Controller/AdminAttributeDefinitionController.php (1)
15-15: Nice consolidation of request classes.The refactoring to use a single
AdminAttributeDefinitionRequestfor both create and update operations reduces duplication and aligns with the pattern used in the Subscriber domain. The OpenAPI schema references and validator calls are consistently updated.Also applies to: 53-53, 89-90, 109-109, 155-156
tests/Unit/Subscription/Request/SubscriberAttributeDefinitionRequestTest.php (1)
1-102: Solid test coverage for the new request class.The test suite comprehensively covers DTO creation, default values, options handling, and validation scenarios. The tests verify both successful mappings (including enum conversion and options as DynamicListAttrDto) and validation failures for invalid inputs.
src/Subscription/Controller/SubscriberAttributeDefinitionController.php (1)
15-15: Clean consolidation matching the Admin domain pattern.The refactoring to use a single
SubscriberAttributeDefinitionRequestfor both create and update operations is consistent with the approach in the Identity module. All OpenAPI schema references, type hints, and validator calls are properly updated.Also applies to: 53-53, 87-88, 106-106, 152-153
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Identity/Request/AdminAttributeDefinitionRequest.php (1)
47-47: Consider dependency injection for services (optional refactor).Instantiating
AttributeTypeValidatorandIdentityTranslatordirectly makes testing harder and couples the validation logic to concrete implementations. Ideally, these would be injected dependencies. However, Symfony's#[Assert\Callback]doesn't support constructor injection easily, and the same pattern appears inSubscriberAttributeDefinitionRequest, so this may be an accepted pattern in the codebase.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Identity/Request/AdminAttributeDefinitionRequest.php(1 hunks)src/Subscription/OpenApi/SwaggerSchemasResponse.php(1 hunks)src/Subscription/Request/SubscriberAttributeDefinitionRequest.php(1 hunks)src/Subscription/Serializer/AttributeDefinitionNormalizer.php(2 hunks)tests/Unit/Subscription/Serializer/AttributeDefinitionNormalizerTest.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Subscription/OpenApi/SwaggerSchemasResponse.php
🧰 Additional context used
🧬 Code graph analysis (3)
tests/Unit/Subscription/Serializer/AttributeDefinitionNormalizerTest.php (1)
src/Subscription/Serializer/AttributeDefinitionNormalizer.php (2)
AttributeDefinitionNormalizer(11-54)normalize(16-45)
src/Subscription/Request/SubscriberAttributeDefinitionRequest.php (2)
src/Subscription/OpenApi/SwaggerSchemasResponse.php (1)
OA(9-171)src/Identity/Request/AdminAttributeDefinitionRequest.php (3)
Assert(15-57)getDto(30-39)validateType(41-56)
src/Identity/Request/AdminAttributeDefinitionRequest.php (1)
src/Subscription/Request/SubscriberAttributeDefinitionRequest.php (2)
getDto(83-94)validateType(96-111)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
🔇 Additional comments (4)
src/Identity/Request/AdminAttributeDefinitionRequest.php (2)
30-39: Couldn't locate DTO classes in the codebase—manual verification needed.Searches for both
AdminAttributeDefinitionDtoandSubscriberAttributeDefinitionDtoreturned no results. I can't confirm the type parameter expectations without seeing the constructor signature.Please verify:
- Whether
AdminAttributeDefinitionDto::__construct()expectstypeas a string orAttributeTypeEnum- If enum conversion is needed, align this with how
SubscriberAttributeDefinitionRequesthandles it
21-22: Verify type handling consistency and Choice constraint completeness.The
#[Assert\Choice]constraint at line 21 restricts input to only['hidden', 'textline'], butSubscriberAttributeDefinitionRequest(a similar class) supports 9 attribute types viaAttributeTypeEnum. Additionally,SubscriberAttributeDefinitionRequestconverts the type string to an enum ingetDto()(line 85:AttributeTypeEnum::from($this->type)), whereasAdminAttributeDefinitionRequestpasses the raw string directly toAdminAttributeDefinitionDto. Confirm whether:
- The Choice constraint should include all enum values supported by
AttributeTypeValidator- The type should be converted to
AttributeTypeEnum::from()before passing to the DTO (matching the pattern inSubscriberAttributeDefinitionRequest)tests/Unit/Subscription/Serializer/AttributeDefinitionNormalizerTest.php (1)
7-9: Nice coverage for enum type and options normalizationThe updated tests now cover both the simple case (no options) and the options path with
DynamicListAttrDtoinstances, and they verify the enum type is normalized viaAttributeTypeEnumas expected. Once the normalizer coalesces missing options to[], these tests give good confidence in the serializer behavior.Also applies to: 26-49, 59-108
src/Subscription/Request/SubscriberAttributeDefinitionRequest.php (1)
18-49: No changes needed — swagger-php 4.11.0 handles enum cases directlyswagger-php v4.x accepts PHP 8.1 enum cases directly in
OA\Property(enum: [...]), and for backed enums it automatically uses the case backing value (→value). The current code pattern is already correct and idiomatic; passingAttributeTypeEnum::Checkboxdirectly achieves the same result asAttributeTypeEnum::Checkbox->valuewith less verbosity.Likely an incorrect or invalid review comment.
| $options = $object->getOptions(); | ||
| if (!empty($options)) { | ||
| $options = array_map(function ($option) { | ||
| if ($option instanceof DynamicListAttrDto) { | ||
| return [ | ||
| 'id' => $option->id, | ||
| 'name' => $option->name, | ||
| 'list_order' => $option->listOrder, | ||
| ]; | ||
| } | ||
| return $option; | ||
| }, $options); | ||
| } | ||
|
|
||
| return [ | ||
| 'id' => $object->getId(), | ||
| 'name' => $object->getName(), | ||
| 'type' => $object->getType(), | ||
| 'type' => $object->getType() ? $object->getType()->value : null, | ||
| 'list_order' => $object->getListOrder(), | ||
| 'default_value' => $object->getDefaultValue(), | ||
| 'required' => $object->isRequired(), | ||
| 'table_name' => $object->getTableName(), | ||
| 'options' => $options, | ||
| ]; |
There was a problem hiding this comment.
Default missing options to an empty array in the normalizer
As written, if SubscriberAttributeDefinition::getOptions() returns null, the normalized payload will contain 'options' => null, while the tests (and typical JSON APIs) expect [] when there are no options.
You can normalize this by coalescing to an empty array before mapping:
- $options = $object->getOptions();
- if (!empty($options)) {
+ $options = $object->getOptions() ?? [];
+ if (!empty($options)) {
$options = array_map(function ($option) {
if ($option instanceof DynamicListAttrDto) {
return [
'id' => $option->id,
'name' => $option->name,
'list_order' => $option->listOrder,
];
}
return $option;
}, $options);
}This keeps the wire format stable (options always an array) and matches the expectations in AttributeDefinitionNormalizerTest.
📝 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.
| $options = $object->getOptions(); | |
| if (!empty($options)) { | |
| $options = array_map(function ($option) { | |
| if ($option instanceof DynamicListAttrDto) { | |
| return [ | |
| 'id' => $option->id, | |
| 'name' => $option->name, | |
| 'list_order' => $option->listOrder, | |
| ]; | |
| } | |
| return $option; | |
| }, $options); | |
| } | |
| return [ | |
| 'id' => $object->getId(), | |
| 'name' => $object->getName(), | |
| 'type' => $object->getType(), | |
| 'type' => $object->getType() ? $object->getType()->value : null, | |
| 'list_order' => $object->getListOrder(), | |
| 'default_value' => $object->getDefaultValue(), | |
| 'required' => $object->isRequired(), | |
| 'table_name' => $object->getTableName(), | |
| 'options' => $options, | |
| ]; | |
| $options = $object->getOptions() ?? []; | |
| if (!empty($options)) { | |
| $options = array_map(function ($option) { | |
| if ($option instanceof DynamicListAttrDto) { | |
| return [ | |
| 'id' => $option->id, | |
| 'name' => $option->name, | |
| 'list_order' => $option->listOrder, | |
| ]; | |
| } | |
| return $option; | |
| }, $options); | |
| } | |
| return [ | |
| 'id' => $object->getId(), | |
| 'name' => $object->getName(), | |
| 'type' => $object->getType() ? $object->getType()->value : null, | |
| 'list_order' => $object->getListOrder(), | |
| 'default_value' => $object->getDefaultValue(), | |
| 'required' => $object->isRequired(), | |
| 'options' => $options, | |
| ]; |
🤖 Prompt for AI Agents
In src/Subscription/Serializer/AttributeDefinitionNormalizer.php around lines 22
to 44, getOptions() can return null which results in 'options' => null in the
normalized payload; coalesce the result to an empty array before mapping (e.g.
$options = $object->getOptions() ?? [];), then run the existing array_map logic
and always return 'options' as an array so empty option sets serialize as []
instead of null.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Subscription/Serializer/AttributeDefinitionNormalizer.php (1)
22-31: Previous review comment still applies: normalize null options to an empty array.If
getOptions()returnsnull, the current code leaves$optionsasnull, which then gets output as'options' => nullon line 40. Typical JSON APIs expect[]for empty collections. The previous reviewer suggested:- $options = $object->getOptions(); + $options = $object->getOptions() ?? []; if (!empty($options)) {Additionally, the
array_mapcallback accesses properties directly ($option->id,$option->name,$option->listOrder) without type checking. If you want more defensive code, consider adding aninstanceof DynamicListAttrDtocheck inside the callback.
🧹 Nitpick comments (1)
src/Subscription/Serializer/AttributeDefinitionNormalizer.php (1)
7-7: Unused import can be removed.
DynamicListAttrDtois imported but never referenced. If you're not planning to add type checks in thearray_mapcallback, consider removing this import.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Subscription/Serializer/AttributeDefinitionNormalizer.php(2 hunks)tests/Unit/Subscription/Serializer/AttributeDefinitionNormalizerTest.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Unit/Subscription/Serializer/AttributeDefinitionNormalizerTest.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
🔇 Additional comments (1)
src/Subscription/Serializer/AttributeDefinitionNormalizer.php (1)
36-36: Type extraction looks good.The null-safe approach correctly extracts the enum's scalar value or returns
nullwhen the type is not set.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Common/EventListener/ExceptionListener.php (1)
36-50: Consider consolidating duplicate exception handling logic.There's a repeating pattern across multiple exception handlers where you create a JsonResponse with
getMessage()andgetStatusCode(). You could extract this into a helper method or create a common interface for these custom exceptions.Something like:
private function createErrorResponse(\Throwable $exception, int $defaultStatusCode = 500): JsonResponse { $statusCode = method_exists($exception, 'getStatusCode') ? $exception->getStatusCode() : $defaultStatusCode; return new JsonResponse([ 'message' => $exception->getMessage(), ], $statusCode); }Then you could simplify the handlers:
- } elseif ($exception instanceof SubscriptionCreationException) { - $response = new JsonResponse([ - 'message' => $exception->getMessage(), - ], $exception->getStatusCode()); - $event->setResponse($response); - } elseif ($exception instanceof AdminAttributeCreationException) { - $response = new JsonResponse([ - 'message' => $exception->getMessage(), - ], $exception->getStatusCode()); - $event->setResponse($response); - } elseif ($exception instanceof AttributeDefinitionCreationException) { - $response = new JsonResponse([ - 'message' => $exception->getMessage(), - ], $exception->getStatusCode()); - $event->setResponse($response); + } elseif ($exception instanceof SubscriptionCreationException + || $exception instanceof AdminAttributeCreationException + || $exception instanceof AttributeDefinitionCreationException) { + $event->setResponse($this->createErrorResponse($exception));Not critical though—the current approach works fine.
src/Subscription/Controller/SubscriberAttributeDefinitionController.php (1)
352-357: Redundant database query—entity already hydrated via MapEntityThe
MapEntityattribute on line 345 already loads and hydrates theSubscriberAttributeDefinitionentity. Re-fetching it by name (lines 353-357) adds an unnecessary query and could introduce subtle bugs if the entity's name was modified but not yet persisted.If the goal is to ensure relationships are loaded, use explicit joins in the repository or fetch-mode configuration instead:
- /** @var SubscriberAttributeDefinitionRepository $repo */ - $repo = $this->entityManager->getRepository(SubscriberAttributeDefinition::class); - $hydrated = $repo->findOneByName($attributeDefinition->getName()); - if ($hydrated instanceof SubscriberAttributeDefinition) { - $attributeDefinition = $hydrated; - } - return $this->json( $this->normalizer->normalize($attributeDefinition), Response::HTTP_OKIf lazy-loading is the issue, verify what relationships need hydration and address them at the repository level.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Common/EventListener/ExceptionListener.php(2 hunks)src/Subscription/Controller/SubscriberAttributeDefinitionController.php(10 hunks)src/Subscription/Request/SubscriberAttributeDefinitionRequest.php(1 hunks)tests/Integration/Subscription/Controller/SubscriberAttributeDefinitionControllerTest.php(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Subscription/Request/SubscriberAttributeDefinitionRequest.php (2)
src/Subscription/OpenApi/SwaggerSchemasResponse.php (1)
OA(9-171)src/Identity/Request/AdminAttributeDefinitionRequest.php (3)
Assert(15-57)getDto(30-39)validateType(41-56)
tests/Integration/Subscription/Controller/SubscriberAttributeDefinitionControllerTest.php (2)
tests/Integration/Identity/Fixtures/AdministratorFixture.php (1)
AdministratorFixture(14-67)tests/Integration/Identity/Fixtures/AdministratorTokenFixture.php (1)
AdministratorTokenFixture(15-62)
src/Subscription/Controller/SubscriberAttributeDefinitionController.php (3)
src/Common/Controller/BaseController.php (1)
BaseController(15-41)src/Subscription/Serializer/AttributeDefinitionNormalizer.php (1)
AttributeDefinitionNormalizer(11-51)src/Subscription/Request/SubscriberAttributeDefinitionRequest.php (1)
OA(18-115)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
🔇 Additional comments (4)
src/Common/EventListener/ExceptionListener.php (2)
9-9: LGTM!The import is correctly placed and necessary for the new exception handler.
46-50: LGTM!The new exception handler follows the existing pattern correctly and will properly handle
AttributeDefinitionCreationExceptionby returning a JSON response with the appropriate status code.tests/Integration/Subscription/Controller/SubscriberAttributeDefinitionControllerTest.php (1)
26-111: Test endpoint updates look goodThe path updates from
/api/v2/subscribers/attributesto/api/v2/attributescorrectly align with the controller route refactoring. Test logic and assertions remain consistent.src/Subscription/Controller/SubscriberAttributeDefinitionController.php (1)
77-81: Documented 409 response but conflict detection unclear — verify external dependencyThe OpenAPI spec documents a 409 (conflict) response with
AlreadyExistsResponse, but thecreate()method (lines 89–101) has no visible conflict detection. The actual conflict logic would exist in the injectedAttributeDefinitionManagerfrom the externalphplist/coredependency (not in this codebase).Verify that
phplist/core'sAttributeDefinitionManager::create()actually throws aConflictHttpExceptionwhen duplicate attribute definitions are detected. If it doesn't, either add validation here or remove the 409 response documentation.
| // Optional multi-value list for types like select/radio | ||
| #[Assert\Type('array')] | ||
| #[Assert\All([ | ||
| 'constraints' => [ | ||
| new Assert\Type(['type' => DynamicListAttrDto::class]), | ||
| ], | ||
| ])] | ||
| public ?array $options = null; |
There was a problem hiding this comment.
Validation constraint mismatch: array elements won't be DynamicListAttrDto instances
The #[Assert\Type(['type' => DynamicListAttrDto::class])] constraint expects array elements to be instances of DynamicListAttrDto, but JSON request deserialization produces plain arrays, not class instances. This validation will fail when clients send valid JSON arrays for options.
Consider removing the class type constraint and validating the array structure instead:
- #[Assert\Type('array')]
- #[Assert\All([
- 'constraints' => [
- new Assert\Type(['type' => DynamicListAttrDto::class]),
- ],
- ])]
+ #[Assert\Type('array')]
+ #[Assert\All([
+ new Assert\Collection(
+ fields: [
+ 'id' => new Assert\Optional([new Assert\Type('integer')]),
+ 'name' => [new Assert\NotBlank(), new Assert\Type('string')],
+ 'list_order' => new Assert\Optional([new Assert\Type('integer')]),
+ ],
+ allowExtraFields: false,
+ ),
+ ])]
public ?array $options = null;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Subscription/Request/SubscriberAttributeDefinitionRequest.php around
lines 74-81, the Assert\Type on array elements currently expects
DynamicListAttrDto instances but incoming JSON yields plain arrays, causing
valid requests to fail; remove the element-level Assert\Type that references
DynamicListAttrDto and replace it with validation that treats each element as an
array (e.g., use Assert\All with Assert\Type('array') and/or Assert\All with
Assert\Collection defining the expected keys and per-key constraints) so the
deserialized arrays are validated by structure rather than class instance.
Summary by CodeRabbit
Release Notes
Documentation
New Features
Changes
Thanks for contributing to phpList!