Skip to content

Commit 4c07ff3

Browse files
TatevikGrtatevikg1
andauthored
Refactor attribute type system to use enums and manage dynamic list attributes table names (#369)
* Rename classes/functions * DynamicListAttrManager * coderabbitai * Fix createTable * After review 0 * After review 1 * After review 2 * DynamicTableMessageHandler * Code rabbit config * After review 3 * Move enum to common * Refactor * StaticAccess * test fix * After review 4 * Update codeRabbit configs * Add options * Add psr/simple-cache * Use parent for SubscriberAttributeDefinitionRepository --------- Co-authored-by: Tatevik <[email protected]>
1 parent 033aa54 commit 4c07ff3

File tree

59 files changed

+1625
-216
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

59 files changed

+1625
-216
lines changed

.coderabbit.yaml

Lines changed: 51 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -9,32 +9,57 @@ reviews:
99
high_level_summary_in_walkthrough: false
1010
poem: false
1111
path_instructions:
12-
- path: "src/Domain/**"
13-
instructions: |
14-
You are reviewing PHP domain-layer code. Enforce strict domain purity:
15-
- ❌ Do not allow infrastructure persistence side effects here.
16-
- Flag ANY usage of Doctrine persistence APIs, especially:
17-
- $entityManager->flush(...), $this->entityManager->flush(...)
18-
- $em->persist(...), $em->remove(...)
19-
- direct transaction control ($em->beginTransaction(), commit(), rollback())
20-
- If found, request moving these calls to application-layer Command handlers or background Jobs.
21-
- Also flag repositories in Domain that invoke flush/transactional logic; Domain repositories should be abstractions without side effects.
22-
- Encourage domain events/outbox or return-values to signal write-intent, leaving orchestration to Commands/Jobs.
23-
24-
- path: "src/**/Command/**"
25-
instructions: |
26-
Application layer (Commands/Handlers) is the right place to coordinate persistence.
27-
- ✅ It is acceptable to call $entityManager->flush() here.
28-
- Check that flush is used atomically (once per unit of work) after all domain operations.
29-
- Ensure no domain entity or domain service is calling flush; only the handler orchestrates it.
30-
- Prefer $em->transactional(...) or explicit try/catch with rollback on failure.
31-
32-
- path: "src/**/MessageHandler/**"
33-
instructions: |
34-
Background jobs/workers may perform persistence.
35-
- ✅ Allow $entityManager->flush() here when the job is the orchestration boundary.
36-
- Verify idempotency and that flush frequency is appropriate (batching where practical).
37-
- Ensure no domain-layer code invoked by the job performs flush/transaction control.
12+
- path: "src/Domain/**"
13+
instructions: |
14+
You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:
15+
16+
- ❌ Do not allow persistence or transaction side effects here for *normal* domain models.
17+
- Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:
18+
- `$entityManager->flush(...)`, `$this->entityManager->flush(...)`
19+
- `$em->persist(...)`, `$em->remove(...)`
20+
- `$em->beginTransaction()`, `$em->commit()`, `$em->rollback()`
21+
- ✅ Accessing Doctrine *metadata*, *schema manager*, or *read-only schema info* is acceptable
22+
as long as it does not modify state or perform writes.
23+
24+
- ✅ **Relaxed rule for DynamicListAttr-related code**:
25+
- DynamicListAttr is a special case dealing with dynamic tables/attrs.
26+
- It is acceptable for DynamicListAttr repositories/services to:
27+
- Create/update/drop DynamicListAttr tables/columns.
28+
- Use Doctrine persistence APIs (`persist`, `remove`, `flush`, etc.)
29+
as part of managing DynamicListAttr data and schema.
30+
- Do *not* flag persistence or schema-creation calls that are clearly scoped
31+
to DynamicListAttr tables or their management.
32+
- Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
33+
not scattered across unrelated domain objects.
34+
35+
- ⚠️ For non-DynamicListAttr code:
36+
- If code is invoking actual table-creation, DDL execution, or schema synchronization,
37+
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).
38+
- Repositories in Domain should be abstractions without side effects; they should express *intent*,
39+
not perform flush/transactional logic.
40+
41+
- path: "src/**/Command/**"
42+
instructions: |
43+
Application layer (Commands/Handlers) is the right place to coordinate persistence.
44+
45+
- ✅ It is acceptable to call $entityManager->flush() here.
46+
- Check that flush is used atomically (once per unit of work) after all domain operations.
47+
- Ensure no domain entity or domain service is calling flush; only the handler orchestrates it,
48+
except for the explicitly allowed DynamicListAttr management logic in Domain.
49+
- Prefer $em->transactional(...) or explicit try/catch with rollback on failure when multiple writes are involved.
50+
51+
- path: "src/**/MessageHandler/**"
52+
instructions: |
53+
Background jobs/workers may perform persistence and schema management.
54+
55+
- ✅ Allow `$entityManager->flush()` when the job is the orchestration boundary.
56+
- ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager),
57+
as this is considered infrastructure-level orchestration.
58+
- For DynamicListAttr-related jobs, it is fine to orchestrate both data and schema changes here,
59+
as long as responsibilities remain clear and behavior is predictable.
60+
- Verify idempotency for schema operations where practical — e.g., check if a table exists before creating.
61+
- Ensure domain-layer code invoked by the job (outside the DynamicListAttr exception) remains free of persistence calls.
62+
- Batch flush operations where practical.
3863
3964
auto_review:
4065
enabled: true

.github/PULL_REQUEST_TEMPLATE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
1-
"@coderabbitai summary"
1+
@coderabbitai summary
22

33
Thanks for contributing to phpList!

composer.json

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,8 @@
7777
"symfony/lock": "^6.4",
7878
"webklex/php-imap": "^6.2",
7979
"ext-imap": "*",
80-
"tatevikgr/rss-feed": "dev-main"
80+
"tatevikgr/rss-feed": "dev-main",
81+
"ext-pdo": "*"
8182
},
8283
"require-dev": {
8384
"phpunit/phpunit": "^9.5",
@@ -94,7 +95,8 @@
9495
"symfony/http-kernel": "^6.4",
9596
"symfony/http-foundation": "^6.4",
9697
"symfony/routing": "^6.4",
97-
"symfony/console": "^6.4"
98+
"symfony/console": "^6.4",
99+
"psr/simple-cache": "^1.0 || ^2.0 || ^3.0"
98100
},
99101
"suggest": {
100102
"phplist/web-frontend": "5.0.x-dev",

config/PHPMD/rules.xml

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -8,22 +8,6 @@
88

99
<!-- rules from the "clean code" rule set -->
1010
<rule ref="rulesets/cleancode.xml/BooleanArgumentFlag"/>
11-
<rule ref="rulesets/cleancode.xml/StaticAccess">
12-
<properties>
13-
<property name="exceptions" value="
14-
\Doctrine\ORM\EntityManager,
15-
\Doctrine\ORM\Tools\Setup,
16-
\FOS\RestBundle\View\View,
17-
\PhpList\Core\Core\Bootstrap,
18-
\PhpList\Core\Core\Environment,
19-
\PHPUnit\DbUnit\Operation\Factory,
20-
\Symfony\Component\Debug\Debug,
21-
\Symfony\Component\Yaml\Yaml,
22-
\League\Csv\Reader,
23-
"/>
24-
</properties>
25-
</rule>
26-
2711
<rule ref="rulesets/codesize.xml/CyclomaticComplexity"/>
2812
<rule ref="rulesets/codesize.xml/NPathComplexity"/>
2913
<rule ref="rulesets/codesize.xml/ExcessiveMethodLength"/>

config/packages/messenger.yaml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,4 +30,5 @@ framework:
3030
'PhpList\Core\Domain\Messaging\Message\PasswordResetMessage': async_email
3131
'PhpList\Core\Domain\Messaging\Message\CampaignProcessorMessage': async_email
3232
'PhpList\Core\Domain\Messaging\Message\SyncCampaignProcessorMessage': sync
33+
'PhpList\Core\Domain\Subscription\Message\DynamicTableMessage': sync
3334

config/parameters.yml.dist

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ parameters:
2323
env(PHPLIST_DATABASE_PASSWORD): 'phplist'
2424
database_prefix: '%%env(DATABASE_PREFIX)%%'
2525
env(DATABASE_PREFIX): 'phplist_'
26+
list_table_prefix: '%%env(LIST_TABLE_PREFIX)%%'
27+
env(LIST_TABLE_PREFIX): 'listattr_'
2628

2729
# Email configuration
2830
app.mailer_from: '%%env(MAILER_FROM)%%'

config/services/managers.yml

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,21 @@ services:
4848
autowire: true
4949
autoconfigure: true
5050

51+
PhpList\Core\Domain\Subscription\Service\Manager\DynamicListAttrManager:
52+
autowire: true
53+
autoconfigure: true
54+
55+
Doctrine\DBAL\Schema\AbstractSchemaManager:
56+
factory: ['@doctrine.dbal.default_connection', 'createSchemaManager']
57+
58+
PhpList\Core\Domain\Subscription\Service\Manager\DynamicListAttrTablesManager:
59+
autowire: true
60+
autoconfigure: true
61+
public: true
62+
arguments:
63+
$dbPrefix: '%database_prefix%'
64+
$dynamicListTablePrefix: '%list_table_prefix%'
65+
5166
PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager:
5267
autowire: true
5368
autoconfigure: true

config/services/mappers.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,10 @@ services:
44
autoconfigure: true
55
public: false
66

7-
PhpList\Core\Domain\Subscription\Service\CsvRowToDtoMapper:
7+
PhpList\Core\Domain\Subscription\Service\ArrayToDtoMapper:
88
autowire: true
99
autoconfigure: true
1010

11-
PhpList\Core\Domain\Subscription\Service\CsvImporter:
11+
PhpList\Core\Domain\Subscription\Service\CsvToDtoImporter:
1212
autowire: true
1313
autoconfigure: true

config/services/messenger.yml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,3 +29,7 @@ services:
2929
autoconfigure: true
3030
tags: [ 'messenger.message_handler' ]
3131

32+
PhpList\Core\Domain\Subscription\MessageHandler\DynamicTableMessageHandler:
33+
autowire: true
34+
autoconfigure: true
35+
tags: [ 'messenger.message_handler' ]

config/services/repositories.yml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,14 +63,17 @@ services:
6363
parent: PhpList\Core\Domain\Common\Repository\AbstractRepository
6464
arguments:
6565
- PhpList\Core\Domain\Subscription\Model\SubscriberAttributeDefinition
66+
calls:
67+
- [ setDynamicListAttrRepository, [ '@PhpList\Core\Domain\Subscription\Repository\DynamicListAttrRepository' ] ]
6668
PhpList\Core\Domain\Subscription\Repository\SubscriptionRepository:
6769
parent: PhpList\Core\Domain\Common\Repository\AbstractRepository
6870
arguments:
6971
- PhpList\Core\Domain\Subscription\Model\Subscription
7072
PhpList\Core\Domain\Subscription\Repository\DynamicListAttrRepository:
7173
autowire: true
7274
arguments:
73-
$prefix: '%database_prefix%'
75+
$dbPrefix: '%database_prefix%'
76+
$dynamicListTablePrefix: '%list_table_prefix%'
7477
PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryRepository:
7578
parent: PhpList\Core\Domain\Common\Repository\AbstractRepository
7679
arguments:

0 commit comments

Comments
 (0)