Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 51 additions & 26 deletions .coderabbit.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,32 +9,57 @@ reviews:
high_level_summary_in_walkthrough: false
poem: false
path_instructions:
- path: "src/Domain/**"
instructions: |
You are reviewing PHP domain-layer code. Enforce strict domain purity:
- ❌ Do not allow infrastructure persistence side effects here.
- Flag ANY usage of Doctrine persistence APIs, especially:
- $entityManager->flush(...), $this->entityManager->flush(...)
- $em->persist(...), $em->remove(...)
- direct transaction control ($em->beginTransaction(), commit(), rollback())
- If found, request moving these calls to application-layer Command handlers or background Jobs.
- Also flag repositories in Domain that invoke flush/transactional logic; Domain repositories should be abstractions without side effects.
- Encourage domain events/outbox or return-values to signal write-intent, leaving orchestration to Commands/Jobs.

- path: "src/**/Command/**"
instructions: |
Application layer (Commands/Handlers) is the right place to coordinate persistence.
- ✅ It is acceptable to call $entityManager->flush() here.
- Check that flush is used atomically (once per unit of work) after all domain operations.
- Ensure no domain entity or domain service is calling flush; only the handler orchestrates it.
- Prefer $em->transactional(...) or explicit try/catch with rollback on failure.

- path: "src/**/MessageHandler/**"
instructions: |
Background jobs/workers may perform persistence.
- ✅ Allow $entityManager->flush() here when the job is the orchestration boundary.
- Verify idempotency and that flush frequency is appropriate (batching where practical).
- Ensure no domain-layer code invoked by the job performs flush/transaction control.
- path: "src/Domain/**"
instructions: |
You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:

- ❌ Do not allow persistence or transaction side effects here for *normal* domain models.
- Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
- `$entityManager->flush(...)`, `$this->entityManager->flush(...)`
- `$em->persist(...)`, `$em->remove(...)`
- `$em->beginTransaction()`, `$em->commit()`, `$em->rollback()`
- ✅ Accessing Doctrine *metadata*, *schema manager*, or *read-only schema info* is acceptable
as long as it does not modify state or perform writes.

- ✅ **Relaxed rule for DynamicListAttr-related code**:
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
- It is acceptable for DynamicListAttr repositories/services to:
- Create/update/drop DynamicListAttr tables/columns.
- Use Doctrine persistence APIs (`persist`, `remove`, `flush`, etc.)
as part of managing DynamicListAttr data and schema.
- Do *not* flag persistence or schema-creation calls that are clearly scoped
to DynamicListAttr tables or their management.
- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
not scattered across unrelated domain objects.

- ⚠️ For non-DynamicListAttr code:
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).
- Repositories in Domain should be abstractions without side effects; they should express *intent*,
not perform flush/transactional logic.

- path: "src/**/Command/**"
instructions: |
Application layer (Commands/Handlers) is the right place to coordinate persistence.

- ✅ It is acceptable to call $entityManager->flush() here.
- Check that flush is used atomically (once per unit of work) after all domain operations.
- Ensure no domain entity or domain service is calling flush; only the handler orchestrates it,
except for the explicitly allowed DynamicListAttr management logic in Domain.
- Prefer $em->transactional(...) or explicit try/catch with rollback on failure when multiple writes are involved.

- path: "src/**/MessageHandler/**"
instructions: |
Background jobs/workers may perform persistence and schema management.

- ✅ Allow `$entityManager->flush()` when the job is the orchestration boundary.
- ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager),
as this is considered infrastructure-level orchestration.
- For DynamicListAttr-related jobs, it is fine to orchestrate both data and schema changes here,
as long as responsibilities remain clear and behavior is predictable.
- Verify idempotency for schema operations where practical — e.g., check if a table exists before creating.
- Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls.
- Batch flush operations where practical.

auto_review:
enabled: true
Expand Down
2 changes: 1 addition & 1 deletion .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
"@coderabbitai summary"
@coderabbitai summary

Thanks for contributing to phpList!
6 changes: 4 additions & 2 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@
"symfony/lock": "^6.4",
"webklex/php-imap": "^6.2",
"ext-imap": "*",
"tatevikgr/rss-feed": "dev-main"
"tatevikgr/rss-feed": "dev-main",
"ext-pdo": "*"
},
"require-dev": {
"phpunit/phpunit": "^9.5",
Expand All @@ -94,7 +95,8 @@
"symfony/http-kernel": "^6.4",
"symfony/http-foundation": "^6.4",
"symfony/routing": "^6.4",
"symfony/console": "^6.4"
"symfony/console": "^6.4",
"psr/simple-cache": "^1.0 || ^2.0 || ^3.0"
},
"suggest": {
"phplist/web-frontend": "5.0.x-dev",
Expand Down
16 changes: 0 additions & 16 deletions config/PHPMD/rules.xml
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,6 @@

<!-- rules from the "clean code" rule set -->
<rule ref="rulesets/cleancode.xml/BooleanArgumentFlag"/>
<rule ref="rulesets/cleancode.xml/StaticAccess">
<properties>
<property name="exceptions" value="
\Doctrine\ORM\EntityManager,
\Doctrine\ORM\Tools\Setup,
\FOS\RestBundle\View\View,
\PhpList\Core\Core\Bootstrap,
\PhpList\Core\Core\Environment,
\PHPUnit\DbUnit\Operation\Factory,
\Symfony\Component\Debug\Debug,
\Symfony\Component\Yaml\Yaml,
\League\Csv\Reader,
"/>
</properties>
</rule>

<rule ref="rulesets/codesize.xml/CyclomaticComplexity"/>
<rule ref="rulesets/codesize.xml/NPathComplexity"/>
<rule ref="rulesets/codesize.xml/ExcessiveMethodLength"/>
Expand Down
1 change: 1 addition & 0 deletions config/packages/messenger.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,5 @@ framework:
'PhpList\Core\Domain\Messaging\Message\PasswordResetMessage': async_email
'PhpList\Core\Domain\Messaging\Message\CampaignProcessorMessage': async_email
'PhpList\Core\Domain\Messaging\Message\SyncCampaignProcessorMessage': sync
'PhpList\Core\Domain\Subscription\Message\DynamicTableMessage': sync

2 changes: 2 additions & 0 deletions config/parameters.yml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ parameters:
env(PHPLIST_DATABASE_PASSWORD): 'phplist'
database_prefix: '%%env(DATABASE_PREFIX)%%'
env(DATABASE_PREFIX): 'phplist_'
list_table_prefix: '%%env(LIST_TABLE_PREFIX)%%'
env(LIST_TABLE_PREFIX): 'listattr_'

# Email configuration
app.mailer_from: '%%env(MAILER_FROM)%%'
Expand Down
15 changes: 15 additions & 0 deletions config/services/managers.yml
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,21 @@ services:
autowire: true
autoconfigure: true

PhpList\Core\Domain\Subscription\Service\Manager\DynamicListAttrManager:
autowire: true
autoconfigure: true

Doctrine\DBAL\Schema\AbstractSchemaManager:
factory: ['@doctrine.dbal.default_connection', 'createSchemaManager']

PhpList\Core\Domain\Subscription\Service\Manager\DynamicListAttrTablesManager:
autowire: true
autoconfigure: true
public: true
arguments:
$dbPrefix: '%database_prefix%'
$dynamicListTablePrefix: '%list_table_prefix%'

PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager:
autowire: true
autoconfigure: true
Expand Down
4 changes: 2 additions & 2 deletions config/services/mappers.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ services:
autoconfigure: true
public: false

PhpList\Core\Domain\Subscription\Service\CsvRowToDtoMapper:
PhpList\Core\Domain\Subscription\Service\ArrayToDtoMapper:
autowire: true
autoconfigure: true

PhpList\Core\Domain\Subscription\Service\CsvImporter:
PhpList\Core\Domain\Subscription\Service\CsvToDtoImporter:
autowire: true
autoconfigure: true
4 changes: 4 additions & 0 deletions config/services/messenger.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,7 @@ services:
autoconfigure: true
tags: [ 'messenger.message_handler' ]

PhpList\Core\Domain\Subscription\MessageHandler\DynamicTableMessageHandler:
autowire: true
autoconfigure: true
tags: [ 'messenger.message_handler' ]
5 changes: 4 additions & 1 deletion config/services/repositories.yml
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,17 @@ services:
parent: PhpList\Core\Domain\Common\Repository\AbstractRepository
arguments:
- PhpList\Core\Domain\Subscription\Model\SubscriberAttributeDefinition
calls:
- [ setDynamicListAttrRepository, [ '@PhpList\Core\Domain\Subscription\Repository\DynamicListAttrRepository' ] ]
PhpList\Core\Domain\Subscription\Repository\SubscriptionRepository:
parent: PhpList\Core\Domain\Common\Repository\AbstractRepository
arguments:
- PhpList\Core\Domain\Subscription\Model\Subscription
PhpList\Core\Domain\Subscription\Repository\DynamicListAttrRepository:
autowire: true
arguments:
$prefix: '%database_prefix%'
$dbPrefix: '%database_prefix%'
$dynamicListTablePrefix: '%list_table_prefix%'
PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryRepository:
parent: PhpList\Core\Domain\Common\Repository\AbstractRepository
arguments:
Expand Down
12 changes: 8 additions & 4 deletions resources/translations/messages.en.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -342,10 +342,6 @@
<source>Subscription not found for this subscriber and list.</source>
<target>Subscription not found for this subscriber and list.</target>
</trans-unit>
<trans-unit id="xWcRIPk" resname="Attribute definition already exists">
<source>Attribute definition already exists</source>
<target>Attribute definition already exists</target>
</trans-unit>
<trans-unit id="kQejNsl" resname="Another attribute with this name already exists.">
<source>Another attribute with this name already exists.</source>
<target>Another attribute with this name already exists.</target>
Expand Down Expand Up @@ -734,6 +730,14 @@ Thank you.</target>
<source>Conflict: email and foreign key refer to different subscribers.</source>
<target>__Conflict: email and foreign key refer to different subscribers.</target>
</trans-unit>
<trans-unit id="HHtgwG9" resname="Invalid email: %email%">
<source>Invalid email: %email%</source>
<target>__Invalid email: %email%</target>
</trans-unit>
<trans-unit id="VvdYY4S" resname="Value must be an AttributeTypeEnum or string.">
<source>Value must be an AttributeTypeEnum or string.</source>
<target>__Value must be an AttributeTypeEnum or string.</target>
</trans-unit>
</body>
</file>
</xliff>
2 changes: 0 additions & 2 deletions src/Core/Bootstrap.php
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,6 @@ private function assertConfigureHasBeenCalled(): void
/**
* Dispatches the current HTTP request (if there is any).
*
* @SuppressWarnings("PHPMD.StaticAccess")
*
* @return null
*
* @throws RuntimeException if configure has not been called before
Expand Down
58 changes: 58 additions & 0 deletions src/Domain/Common/Model/AttributeTypeEnum.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
<?php

declare(strict_types=1);

namespace PhpList\Core\Domain\Common\Model;

use PhpList\Core\Domain\Subscription\Exception\AttributeTypeChangeNotAllowed;

enum AttributeTypeEnum: string
{
case Text = 'text';
case Number = 'number';
case Date = 'date';
case Select = 'select';
case Checkbox = 'checkbox';
case CheckboxGroup = 'checkboxgroup';
case Radio = 'radio';
case Hidden = 'hidden';
case TextLine = 'textline';
case CreditCardNo = 'creditcardno';
case TextArea = 'textarea';

public function equals(self $other): bool
{
return $this === $other;
}

public function isMultiValued(): bool
{
return match ($this) {
self::Select,
self::Checkbox,
self::Radio,
self::CheckboxGroup => true,
default => false,
};
}

public function canTransitionTo(self $toType): bool
{
if ($this === $toType) {
return true;
}

if ($this->isMultiValued() !== $toType->isMultiValued()) {
return false;
}

return true;
}

public function assertTransitionAllowed(self $toType): void
{
if (!$this->canTransitionTo($toType)) {
throw new AttributeTypeChangeNotAllowed($this->value, $toType->value);
}
}
}
1 change: 0 additions & 1 deletion src/Domain/Common/SystemInfoCollector.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ public function __construct(RequestStack $requestStack, array $configuredKeys =

/**
* Return key=>value pairs (already sanitized for safe logging/HTML display).
* @SuppressWarnings(PHPMD.StaticAccess)
* @return array<string,string>
*/
public function collect(): array
Expand Down
3 changes: 0 additions & 3 deletions src/Domain/Configuration/Service/Provider/ConfigProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ public function __construct(
}

/**
* @SuppressWarnings(PHPMD.StaticAccess)
* @throws InvalidArgumentException
*/
public function isEnabled(ConfigOption $key): bool
Expand All @@ -43,7 +42,6 @@ public function isEnabled(ConfigOption $key): bool

/**
* Get configuration value by its key, from settings or default configs or default value (if provided)
* @SuppressWarnings(PHPMD.StaticAccess)
* @throws InvalidArgumentException
*/
public function getValue(ConfigOption $key): ?string
Expand All @@ -65,7 +63,6 @@ public function getValue(ConfigOption $key): ?string
return $this->defaultConfigs->has($key->value) ? $this->defaultConfigs->get($key->value)['value'] : null;
}

/** @SuppressWarnings(PHPMD.StaticAccess) */
public function getValueWithNamespace(ConfigOption $key): ?string
{
$full = $this->getValue($key);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
use Symfony\Contracts\Translation\TranslatorInterface;

// phpcs:disable Generic.Files.LineLength
/** @SuppressWarnings(PHPMD.StaticAccess) */
class DefaultConfigProvider
{
/**
Expand Down
1 change: 0 additions & 1 deletion src/Domain/Identity/Command/ImportDefaultsCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ protected function execute(InputInterface $input, OutputInterface $output): int

/**
* @return array<string,bool>
* @SuppressWarnings(PHPMD.StaticAccess)
*/
private function allPrivilegesGranted(): array
{
Expand Down
7 changes: 0 additions & 7 deletions src/Domain/Identity/Model/AdminAttributeDefinition.php
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,4 @@ public function setRequired(?bool $required): self

return $this;
}

public function setTableName(?string $tableName): self
{
$this->tableName = $tableName;

return $this;
}
}
3 changes: 0 additions & 3 deletions src/Domain/Identity/Model/Administrator.php
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,6 @@ public function setPrivileges(Privileges $privileges): self

/**
* @throws InvalidArgumentException
*
* @SuppressWarnings(PHPMD.StaticAccess)
*/
public function setPrivilegesFromArray(array $privilegesData): void
{
Expand All @@ -183,7 +181,6 @@ public function setPrivilegesFromArray(array $privilegesData): void
$this->setPrivileges($privileges);
}

/** @SuppressWarnings(PHPMD.StaticAccess) */
public function getPrivileges(): Privileges
{
return Privileges::fromSerialized($this->privileges);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ public function __construct(
public readonly ?int $listOrder = null,
public readonly ?string $defaultValue = null,
public readonly ?bool $required = false,
public readonly ?string $tableName = null,
) {
}
}
3 changes: 0 additions & 3 deletions src/Domain/Identity/Model/Privileges.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@ class Privileges
*/
private array $flags = [];

/**
* @SuppressWarnings(PHPMD.StaticAccess)
*/
public function __construct(?array $flags = [])
{
foreach (PrivilegeFlag::cases() as $flag) {
Expand Down
Loading
Loading