Conversation
|
Note
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 22.73% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'Add forward email endpoint' clearly and concisely describes the main feature added in this changeset: a new REST endpoint for forwarding emails/campaigns. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
feat/email-forwarding
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 help to get the list of available commands and usage tips.
TatevikGr
force-pushed
the
feat/email-forwarding
branch
from
38332db to
8fbe97a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Identity/Controller/AdminAttributeValueController.php (1)
350-362:⚠️ Potential issue | 🔴 CriticalIncorrect entity type for
$adminparameter.The
$adminparameter is typed asAdminAttributeDefinitionbut should beAdministrator. The mapping['adminId' => 'id']is intended to resolve an Administrator entity, not an attribute definition. This will cause Doctrine to look up the wrong entity type.🐛 Proposed fix
public function getAttributeDefinition( Request $request, - #[MapEntity(mapping: ['adminId' => 'id'])] ?AdminAttributeDefinition $admin, + #[MapEntity(mapping: ['adminId' => 'id'])] ?Administrator $admin, #[MapEntity(mapping: ['definitionId' => 'id'])] ?AdminAttributeDefinition $definition, ): JsonResponse {
🤖 Fix all issues with AI agents
In `@composer.json`:
- Around line 45-52: The dependency "phplist/core" is pinned to a mutable branch
ref (dev-ref/campaign-processing); update composer.json to reference a specific
commit by changing the version to the branch with its commit SHA
(dev-ref/campaign-processing#<commit-sha>) to ensure reproducible installs, and
consider doing the same for "tatevikgr/rss-feed" (currently dev-main) so both
mutable refs are consistently pinned; locate and update the "phplist/core" and
optionally "tatevikgr/rss-feed" entries in composer.json and run composer update
to regenerate composer.lock locally.
In `@src/Messaging/Controller/EmailForwardController.php`:
- Around line 70-118: The OpenAPI schema name collision is caused by
ForwardingResultNormalizer registering a schema as 'ForwardResult' — change the
schema name in the ForwardingResultNormalizer class from 'ForwardResult' to
'ForwardingResult' (update the OA\Schema/@OA\Components reference inside that
normalizer), then update the EmailForwardController::forwardMessage OpenAPI
response reference from '#/components/schemas/ForwardResult' to
'#/components/schemas/ForwardingResult' so the controller's OA\JsonContent
refers to the new schema name (also search for any other references to
'ForwardResult' and update to 'ForwardingResult' if they point to the forwarding
normalizer).
In `@src/Messaging/Request/ForwardMessageRequest.php`:
- Around line 56-66: The per-recipient validation currently in
ForwardMessageRequest uses Assert\All with Assert\Email and Assert\Length, but
Assert\Email allows empty strings so an array like [''] can pass; update the
constraints inside the Assert\All for the recipients field to include
Assert\NotBlank (in addition to Assert\Email and Assert\Length) so each array
element is required to be non-empty; locate the attribute block on the
recipients property (the group including #[Assert\NotNull], #[Assert\NotBlank],
#[Assert\Type('array')], #[Assert\Count(min: 1)], #[MaxForwardCount],
#[Assert\All([...])]) and add the NotBlank constraint to the inner constraints
list.
In `@src/Messaging/Serializer/ForwardingResultNormalizer.php`:
- Around line 21-35: The OpenAPI schema name collides — change the OA\Schema
annotation in ForwardingResultNormalizer to use schema: 'ForwardingResult'
(instead of 'ForwardResult') so it matches the model and intent, and then update
any controller references (e.g., the schema ref used in EmailForwardController)
to point to 'ForwardingResult'; also verify the other normalizer
(ForwardResultNormalizer) keeps its own 'ForwardResult' schema name to avoid
further collisions.
In `@src/Messaging/Serializer/ForwardResultNormalizer.php`:
- Around line 10-27: The OpenAPI schema name collision occurs because
ForwardResultNormalizer.php and ForwardingResultNormalizer.php both declare
schema "ForwardResult"; rename the schema in ForwardResultNormalizer (e.g.
change the OA\Schema schema name to "ForwardQueueResult") and then update any
usages—specifically the reference in EmailForwardController.php that points to
'#/components/schemas/ForwardResult'—to the new name so the two distinct schemas
no longer conflict.
- Around line 39-46: The current normalizer returns 'recipients_count'
defaulting to 0 which can be inconsistent when 'recipients' is present; update
ForwardResultNormalizer (the method that builds the returned array using
$object['recipients'] and $object['recipients_count']) to compute
recipients_count as count($object['recipients']) when
$object['recipients_count'] is not set (i.e., use $object['recipients_count'] ??
count($object['recipients'] ?? [])), ensuring the count fallback uses an empty
array if recipients is missing and casting or leaving as int.
In `@src/Messaging/Validator/Constraint/MaxForwardCountValidator.php`:
- Around line 13-45: The injected $maxForward is typed as string but should be
an int (nullable) like in MaxPersonalNoteSizeValidator; change the
constructor/property type to ?int $maxForward, add a guard in validate to return
early if $this->maxForward is null or less than 0, and when building the
violation use setParameter('{{ limit }}', (string) $this->maxForward) so the
parameter is a string; update the constructor signature and references to
$maxForward in MaxForwardCountValidator and ensure validate() still compares
$uniqueCount against $this->maxForward as an int.
In `@src/Messaging/Validator/Constraint/MaxPersonalNoteSizeValidator.php`:
- Around line 32-38: The validator in MaxPersonalNoteSizeValidator currently
sets $sizeLimit = $this->maxSize * 2 which makes the length check twice as
permissive than the reported limit; change the check to use the configured limit
directly (remove the "* 2" so $sizeLimit === $this->maxSize or eliminate
$sizeLimit and compare $length to $this->maxSize), keeping the existing length
computation (mb_strlen/strlen) and leaving the violation creation via
$this->context->buildViolation(...)->setParameter('{{ limit }}', (string)
$this->maxSize)->addViolation() intact so the enforced limit matches the
reported {{ limit }}.
🧹 Nitpick comments (1)
src/Messaging/Controller/EmailForwardController.php (1)
61-67: Use integer schema formessageId.The route requires digits, so the OpenAPI parameter type should be
integerto match validation and client expectations.✏️ Suggested tweak
new OA\Parameter( name: 'messageId', description: 'message ID', in: 'path', required: true, - schema: new OA\Schema(type: 'string') + schema: new OA\Schema(type: 'integer') )
| new OA\Response( | ||
| response: 202, | ||
| description: 'Accepted', | ||
| content: new OA\JsonContent(ref: '#/components/schemas/ForwardResult') | ||
| ), | ||
| new OA\Response( | ||
| response: 403, | ||
| description: 'Failure', | ||
| content: new OA\JsonContent(ref: '#/components/schemas/UnauthorizedResponse') | ||
| ), | ||
| new OA\Response( | ||
| response: 404, | ||
| description: 'Failure', | ||
| content: new OA\JsonContent(ref: '#/components/schemas/NotFoundErrorResponse') | ||
| ), | ||
| new OA\Response( | ||
| response: 422, | ||
| description: 'Failure', | ||
| content: new OA\JsonContent(ref: '#/components/schemas/ValidationErrorResponse') | ||
| ) | ||
| ] | ||
| )] | ||
| public function forwardMessage( | ||
| Request $request, | ||
| #[MapEntity(mapping: ['messageId' => 'id'])] ?Message $message = null | ||
| ): JsonResponse { | ||
| if ($message === null) { | ||
| throw $this->createNotFoundException('Campaign not found.'); | ||
| } | ||
|
|
||
| /** @var ForwardMessageRequest $forwardRequest */ | ||
| $forwardRequest = $this->validator->validate($request, ForwardMessageRequest::class); | ||
|
|
||
| $result = $this->messageForwardService->forward( | ||
| messageForwardDto: new MessageForwardDto( | ||
| emails: $forwardRequest->recipients, | ||
| uid: $forwardRequest->uid, | ||
| fromName: $forwardRequest->fromName, | ||
| fromEmail: $forwardRequest->fromEmail, | ||
| note: $forwardRequest->note, | ||
| ), | ||
| campaign: $message, | ||
| ); | ||
|
|
||
| $this->entityManager->flush(); | ||
|
|
||
| return $this->json( | ||
| $this->forwardingResultNormalizer->normalize($result), | ||
| Response::HTTP_ACCEPTED |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full controller file context
cat -n src/Messaging/Controller/EmailForwardController.php | head -130Repository: phpList/rest-api
Length of output: 5504
🏁 Script executed:
# Find the ForwardingResultNormalizer file
find . -name "*ForwardingResultNormalizer*" -type fRepository: phpList/rest-api
Length of output: 117
🏁 Script executed:
# Search for schema definitions or OpenAPI schemas related to ForwardResult
find . -name "*.php" -type f | head -50 | xargs grep -l "ForwardResult\|ForwardingResult" 2>/dev/null | head -20Repository: phpList/rest-api
Length of output: 113
🏁 Script executed:
cat -n src/Messaging/Serializer/ForwardingResultNormalizer.phpRepository: phpList/rest-api
Length of output: 2993
🏁 Script executed:
# Search for OpenAPI schema definitions (might be in attributes or configuration)
find . -type f -name "*.php" | xargs grep -l "#/components/schemas/ForwardResult" 2>/dev/nullRepository: phpList/rest-api
Length of output: 113
🏁 Script executed:
# Look for schema attribute definitions or any other ForwardResult/ForwardingResult references
rg "ForwardResult|ForwardingResult" --type php -C 3Repository: phpList/rest-api
Length of output: 6174
🏁 Script executed:
cat -n src/Messaging/Serializer/ForwardResultNormalizer.phpRepository: phpList/rest-api
Length of output: 2500
Resolve the schema naming collision: ForwardingResultNormalizer should define 'ForwardingResult', not 'ForwardResult'.
Both ForwardingResultNormalizer and ForwardResultNormalizer define a schema named 'ForwardResult', but with completely different structures. This will break OpenAPI spec generation.
Rename the schema in ForwardingResultNormalizer (src/Messaging/Serializer/ForwardingResultNormalizer.php, line 22) from 'ForwardResult' to 'ForwardingResult' to match the class name and avoid the collision. Then update the controller's schema reference accordingly:
Fix
- new OA\Response(
- response: 202,
- description: 'Accepted',
- content: new OA\JsonContent(ref: '#/components/schemas/ForwardResult')
- ),
+ new OA\Response(
+ response: 202,
+ description: 'Accepted',
+ content: new OA\JsonContent(ref: '#/components/schemas/ForwardingResult')
+ ),🤖 Prompt for AI Agents
In `@src/Messaging/Controller/EmailForwardController.php` around lines 70 - 118,
The OpenAPI schema name collision is caused by ForwardingResultNormalizer
registering a schema as 'ForwardResult' — change the schema name in the
ForwardingResultNormalizer class from 'ForwardResult' to 'ForwardingResult'
(update the OA\Schema/@OA\Components reference inside that normalizer), then
update the EmailForwardController::forwardMessage OpenAPI response reference
from '#/components/schemas/ForwardResult' to
'#/components/schemas/ForwardingResult' so the controller's OA\JsonContent
refers to the new schema name (also search for any other references to
'ForwardResult' and update to 'ForwardingResult' if they point to the forwarding
normalizer).
| #[Assert\NotNull] | ||
| #[Assert\NotBlank] | ||
| #[Assert\Type('array')] | ||
| #[Assert\Count(min: 1)] | ||
| #[MaxForwardCount] | ||
| #[Assert\All([ | ||
| 'constraints' => [ | ||
| new Assert\Email([]), | ||
| new Assert\Length(max: 255), | ||
| ], | ||
| ])] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Symfony Assert Email constraint empty string validation behavior
💡 Result:
Symfony’s #[Assert\Email] does not fail on an empty string ('') (or null). Empty values are treated as valid so the field can be optional. If you want the email to be required, combine it with #[Assert\NotBlank] (or another “required” constraint). [1]
Example:
use Symfony\Component\Validator\Constraints as Assert;
class UserInput
{
#[Assert\NotBlank]
#[Assert\Email]
public ?string $email = null;
}NotBlank is the constraint that enforces “not an empty string” (and other blank-ish values). [2]
Sources: [1] Symfony Docs – Email constraint, note about null/empty strings being valid; [2] Symfony Docs – NotBlank constraint.
Add NotBlank to reject empty recipient entries.
Assert\Email allows empty strings by default (they're considered valid to keep the field optional). Since you're using Assert\All with Count(min: 1), an array of blank strings like [''] would pass validation. Add NotBlank inside the constraints array to enforce actual content per item.
Proposed fix
#[Assert\All([
'constraints' => [
+ new Assert\NotBlank(),
new Assert\Email([]),
new Assert\Length(max: 255),
],
])]📝 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.
| #[Assert\NotNull] | |
| #[Assert\NotBlank] | |
| #[Assert\Type('array')] | |
| #[Assert\Count(min: 1)] | |
| #[MaxForwardCount] | |
| #[Assert\All([ | |
| 'constraints' => [ | |
| new Assert\Email([]), | |
| new Assert\Length(max: 255), | |
| ], | |
| ])] | |
| #[Assert\NotNull] | |
| #[Assert\NotBlank] | |
| #[Assert\Type('array')] | |
| #[Assert\Count(min: 1)] | |
| #[MaxForwardCount] | |
| #[Assert\All([ | |
| 'constraints' => [ | |
| new Assert\NotBlank(), | |
| new Assert\Email([]), | |
| new Assert\Length(max: 255), | |
| ], | |
| ])] |
🤖 Prompt for AI Agents
In `@src/Messaging/Request/ForwardMessageRequest.php` around lines 56 - 66, The
per-recipient validation currently in ForwardMessageRequest uses Assert\All with
Assert\Email and Assert\Length, but Assert\Email allows empty strings so an
array like [''] can pass; update the constraints inside the Assert\All for the
recipients field to include Assert\NotBlank (in addition to Assert\Email and
Assert\Length) so each array element is required to be non-empty; locate the
attribute block on the recipients property (the group including
#[Assert\NotNull], #[Assert\NotBlank], #[Assert\Type('array')],
#[Assert\Count(min: 1)], #[MaxForwardCount], #[Assert\All([...])]) and add the
NotBlank constraint to the inner constraints list.
| #[OA\Schema( | ||
| schema: 'ForwardResult', | ||
| properties: [ | ||
| new OA\Property(property: 'total_requested', type: 'integer', example: 3), | ||
| new OA\Property(property: 'total_sent', type: 'integer', example: 2), | ||
| new OA\Property(property: 'total_failed', type: 'integer', example: 1), | ||
| new OA\Property(property: 'total_already_sent', type: 'integer', example: 0), | ||
| new OA\Property( | ||
| property: 'recipients', | ||
| type: 'array', | ||
| items: new OA\Items(ref: '#/components/schemas/ForwardRecipientResult') | ||
| ), | ||
| ], | ||
| type: 'object' | ||
| )] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the ForwardResultNormalizer.php file and check its content
find . -name "ForwardResultNormalizer.php" -type f | head -20Repository: phpList/rest-api
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Search for all schema declarations named 'ForwardResult'
rg -n "schema:\s*['\"]ForwardResult['\"]" -g '*.php' srcRepository: phpList/rest-api
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Search for references to ForwardResult in schema references
rg -n "ForwardResult" -g '*.php' -g '*.yml' -g '*.yaml' src configRepository: phpList/rest-api
Length of output: 491
🏁 Script executed:
#!/bin/bash
# Check for all references to both possible schema names
rg -n "#/components/schemas/ForwardResult|#/components/schemas/ForwardingResult" -g '*.php' -g '*.yml' -g '*.yaml'Repository: phpList/rest-api
Length of output: 201
🏁 Script executed:
#!/bin/bash
# Examine both normalizer classes to understand their purpose
cat -n src/Messaging/Serializer/ForwardResultNormalizer.php | head -40
cat -n src/Messaging/Serializer/ForwardingResultNormalizer.php | head -40Repository: phpList/rest-api
Length of output: 3427
Schema name collision with ForwardResult—rename to ForwardingResult.
Both ForwardResultNormalizer and ForwardingResultNormalizer declare schema: 'ForwardResult', but they define different structures (one for message forwarding details, one for operation results with counts). Since your class uses the ForwardingResult model and represents a forwarding operation, rename the schema to ForwardingResult and update the reference in EmailForwardController.php line 73.
Changes needed
- schema: 'ForwardResult',
+ schema: 'ForwardingResult',Also update the controller reference:
- content: new OA\JsonContent(ref: '#/components/schemas/ForwardResult')
+ content: new OA\JsonContent(ref: '#/components/schemas/ForwardingResult')🤖 Prompt for AI Agents
In `@src/Messaging/Serializer/ForwardingResultNormalizer.php` around lines 21 -
35, The OpenAPI schema name collides — change the OA\Schema annotation in
ForwardingResultNormalizer to use schema: 'ForwardingResult' (instead of
'ForwardResult') so it matches the model and intent, and then update any
controller references (e.g., the schema ref used in EmailForwardController) to
point to 'ForwardingResult'; also verify the other normalizer
(ForwardResultNormalizer) keeps its own 'ForwardResult' schema name to avoid
further collisions.
| public function __construct( | ||
| #[Autowire('%phplist.forward_email_count%')] private readonly string $maxForward | ||
| ) { | ||
| } | ||
|
|
||
| public function validate($value, Constraint $constraint): void | ||
| { | ||
| if (!$constraint instanceof MaxForwardCount) { | ||
| return; | ||
| } | ||
|
|
||
| if (!is_array($value)) { | ||
| return; | ||
| } | ||
|
|
||
| $normalized = []; | ||
| foreach ($value as $item) { | ||
| if (!is_string($item)) { | ||
| continue; | ||
| } | ||
| $email = strtolower(trim($item)); | ||
| if ($email === '') { | ||
| continue; | ||
| } | ||
| $normalized[$email] = true; | ||
| } | ||
|
|
||
| $uniqueCount = count($normalized); | ||
|
|
||
| if ($uniqueCount > $this->maxForward) { | ||
| $this->context->buildViolation($constraint->message) | ||
| ->setParameter('{{ limit }}', $this->maxForward) | ||
| ->addViolation(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for forward_email_count definition
rg -n "forward_email_count" -g '*.yml' -g '*.yaml' -g '*.php' --max-count 20Repository: phpList/rest-api
Length of output: 215
🏁 Script executed:
#!/bin/bash
# Check the PHP file for strict_types declaration
head -20 src/Messaging/Validator/Constraint/MaxForwardCountValidator.phpRepository: phpList/rest-api
Length of output: 640
🏁 Script executed:
#!/bin/bash
# Look for any parameter definition files or config structure
find . -name "*.yml" -o -name "*.yaml" | head -20Repository: phpList/rest-api
Length of output: 531
🏁 Script executed:
#!/bin/bash
# Search all YAML config files for forward_email_count definition
rg -n "forward_email_count" config/Repository: phpList/rest-api
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check services.yml for parameter definitions
grep -A 5 -B 5 "forward_email_count" config/services.ymlRepository: phpList/rest-api
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look at the parameters section in services.yml
sed -n '/^parameters:/,/^[^ ]/p' config/services.yml | head -50Repository: phpList/rest-api
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if services.yml exists and examine its contents
cat config/services.ymlRepository: phpList/rest-api
Length of output: 616
🏁 Script executed:
#!/bin/bash
# Search across all config files more broadly
find config -type f \( -name "*.yml" -o -name "*.yaml" \) -exec grep -l "phplist" {} \;Repository: phpList/rest-api
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check for environment files or .env that might define this
find . -name ".env*" -o -name "parameters.yml" -o -name "parameters.yaml" 2>/dev/nullRepository: phpList/rest-api
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search entire codebase for forward_email_count
rg "forward_email_count" .Repository: phpList/rest-api
Length of output: 214
🏁 Script executed:
#!/bin/bash
# Check validators.yml specifically since that's where this validator would be configured
cat config/services/validators.yml 2>/dev/null || echo "File not found"Repository: phpList/rest-api
Length of output: 1491
🏁 Script executed:
#!/bin/bash
# Look for any reference to MaxForwardCount constraint
rg -n "MaxForwardCount" --max-count 20Repository: phpList/rest-api
Length of output: 601
🏁 Script executed:
#!/bin/bash
# Look for parameter definitions - check all config files
rg "parameters:" config/ -A 20Repository: phpList/rest-api
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if there's a services.yaml or any other config format
find config -type f | xargs cat | grep -i "forward"Repository: phpList/rest-api
Length of output: 249
🏁 Script executed:
#!/bin/bash
# Check the MaxForwardCount constraint itself for hints
cat src/Messaging/Validator/Constraint/MaxForwardCount.phpRepository: phpList/rest-api
Length of output: 426
🏁 Script executed:
#!/bin/bash
# Check if phplist.core or parent packages define this parameter
find . -path ./vendor -prune -o -name "*.yml" -type f -print | xargs grep -l "phplist.forward_email_count" 2>/dev/nullRepository: phpList/rest-api
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check composer.json to understand dependencies
cat composer.json | head -50Repository: phpList/rest-api
Length of output: 1473
🏁 Script executed:
#!/bin/bash
# Search for any file mentioning this parameter at all
rg "phplist\.forward_email_count" . --max-count 10Repository: phpList/rest-api
Length of output: 214
🏁 Script executed:
#!/bin/bash
# Check test files for MaxForwardCountValidator usage
find . -name "*MaxForwardCount*Test*" -o -name "*test*" -type f | xargs grep -l "MaxForwardCount" 2>/dev/null | head -5Repository: phpList/rest-api
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search tests for how maxForward or forward_email_count is tested
rg "maxForward|forward_email_count" --max-count 15 -g "*.php"Repository: phpList/rest-api
Length of output: 453
🏁 Script executed:
#!/bin/bash
# Check if there's any integration or unit test that exercises this validator
find . -path ./vendor -prune -o -name "*Test.php" -type f -print | xargs grep -l "MaxForwardCount" 2>/dev/nullRepository: phpList/rest-api
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check if there are integration tests at project level
find tests -name "*.php" 2>/dev/null | head -10Repository: phpList/rest-api
Length of output: 569
🏁 Script executed:
#!/bin/bash
# Check setParameter usage in Symfony to understand if it needs string
rg "setParameter" -B 2 -A 2 --max-count 10 -g "*.php"Repository: phpList/rest-api
Length of output: 2390
🏁 Script executed:
#!/bin/bash
# Check MaxPersonalNoteSizeValidator to see how it handles similar situation
cat src/Messaging/Validator/Constraint/MaxPersonalNoteSizeValidator.phpRepository: phpList/rest-api
Length of output: 1225
🏁 Script executed:
#!/bin/bash
# Look for any integration tests that might instantiate MaxForwardCountValidator
rg "MaxForwardCount" tests/ -A 5 -B 5Repository: phpList/rest-api
Length of output: 42
Type mismatch: $maxForward should be int, not string.
The similar validator MaxPersonalNoteSizeValidator in the same codebase already establishes the correct pattern: it uses ?int for the injected config parameter %phplist.forward_personal_note_size%, includes a null/negative guard, and casts to string for setParameter(). The MaxForwardCountValidator should follow the same pattern. With strict_types=1, a string type hint will reject an integer injected by the DI container.
Update to match the established pattern:
🛠️ Suggested fix
- public function __construct(
- #[Autowire('%phplist.forward_email_count%')] private readonly string $maxForward
- ) {
- }
+ public function __construct(
+ #[Autowire('%phplist.forward_email_count%')] private readonly ?int $maxForward
+ ) {
+ }
@@
+ if ($this->maxForward === null || $this->maxForward < 0) {
+ return;
+ }
+
$uniqueCount = count($normalized);
@@
- ->setParameter('{{ limit }}', $this->maxForward)
+ ->setParameter('{{ limit }}', (string) $this->maxForward)🤖 Prompt for AI Agents
In `@src/Messaging/Validator/Constraint/MaxForwardCountValidator.php` around lines
13 - 45, The injected $maxForward is typed as string but should be an int
(nullable) like in MaxPersonalNoteSizeValidator; change the constructor/property
type to ?int $maxForward, add a guard in validate to return early if
$this->maxForward is null or less than 0, and when building the violation use
setParameter('{{ limit }}', (string) $this->maxForward) so the parameter is a
string; update the constructor signature and references to $maxForward in
MaxForwardCountValidator and ensure validate() still compares $uniqueCount
against $this->maxForward as an int.
| $sizeLimit = $this->maxSize * 2; | ||
| $length = function_exists('mb_strlen') ? mb_strlen($value) : strlen($value); | ||
|
|
||
| if ($length > $sizeLimit) { | ||
| $this->context->buildViolation($constraint->message) | ||
| ->setParameter('{{ limit }}', (string) $this->maxSize) | ||
| ->addViolation(); |
There was a problem hiding this comment.
Configured limit is effectively doubled.
$sizeLimit uses $maxSize * 2, but the violation message advertises {{ limit }} as the configured value. That makes the check twice as lenient as the message implies and can let oversized notes slip through. If the intent is a character cap, align the check with $maxSize.
🛠️ Proposed fix
- $sizeLimit = $this->maxSize * 2;
+ $sizeLimit = $this->maxSize;📝 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.
| $sizeLimit = $this->maxSize * 2; | |
| $length = function_exists('mb_strlen') ? mb_strlen($value) : strlen($value); | |
| if ($length > $sizeLimit) { | |
| $this->context->buildViolation($constraint->message) | |
| ->setParameter('{{ limit }}', (string) $this->maxSize) | |
| ->addViolation(); | |
| $sizeLimit = $this->maxSize; | |
| $length = function_exists('mb_strlen') ? mb_strlen($value) : strlen($value); | |
| if ($length > $sizeLimit) { | |
| $this->context->buildViolation($constraint->message) | |
| ->setParameter('{{ limit }}', (string) $this->maxSize) | |
| ->addViolation(); |
🤖 Prompt for AI Agents
In `@src/Messaging/Validator/Constraint/MaxPersonalNoteSizeValidator.php` around
lines 32 - 38, The validator in MaxPersonalNoteSizeValidator currently sets
$sizeLimit = $this->maxSize * 2 which makes the length check twice as permissive
than the reported limit; change the check to use the configured limit directly
(remove the "* 2" so $sizeLimit === $this->maxSize or eliminate $sizeLimit and
compare $length to $this->maxSize), keeping the existing length computation
(mb_strlen/strlen) and leaving the violation creation via
$this->context->buildViolation(...)->setParameter('{{ limit }}', (string)
$this->maxSize)->addViolation() intact so the enforced limit matches the
reported {{ limit }}.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests
Thanks for contributing to phpList!