Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SubscribePage.data and accessors; repository adds findPageWithData and cursor pagination; new SubscribePageConfigMigrationService migrates config↔page-data; SubscribePageManager gains findPage and syncPageData and drops Translator; ConfigProvider accepts string namespaced keys; services/parameters wiring and expanded unit tests added. ChangesSubscription page model, repository, manager, migration, config, and tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
🧹 Nitpick comments (2)
src/Domain/Subscription/Repository/SubscriberPageRepository.php (1)
41-49: 💤 Low valueHeads-up on
totalsemantics +lastIdparameter usage.Two things worth a glance:
$countQb = clone $queryBuilder;is cloned before theandWhere('p.id > :afterId')is attached, so$totalends up being the count of all pages — not the count after the cursor. That's actually fine for cursor pagination if you want the consumer to know the total set size, but it's easy to misread; a short comment would help.$filter->getLastId()is called repeatedly — minor, but caching it in a local would also avoid drift if the filter object ever mutates between calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Domain/Subscription/Repository/SubscriberPageRepository.php` around lines 41 - 49, In getFilteredAfterId: cache $filter->getLastId() into a local (e.g. $afterId) and use that variable for all subsequent calls to avoid repeated method calls and potential mutation drift; also add a short comment next to the $countQb = clone $queryBuilder (or adjust cloning placement) clarifying whether $total is intended to be the overall count or the post-cursor count—if you want total after the cursor, move the clone to after applying andWhere('p.id > :afterId'), otherwise keep current clone and document the semantics.src/Domain/Subscription/Service/Manager/SubscribePageManager.php (1)
40-66: 💤 Low valueMixing
pageDataRepository->persist(...)withentityManager->remove(...)reads oddly.
syncPageDataschedules creates through the repository ($this->pageDataRepository->persist($pageData)) but schedules deletes directly against$this->entityManager->remove($pageData). Both are fine from a domain-purity standpoint (no flush, no DDL — that's allowed by the guidelines), but it's a little jarring to read. IfSubscriberPageDataRepositoryalready has (or could grow) aremove()method, going through the repo for both keeps the abstraction consistent and theentityManagerdependency a bit less load-bearing.#!/bin/bash ast-grep --pattern $'class SubscriberPageDataRepository { $$$ }'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Domain/Subscription/Service/Manager/SubscribePageManager.php` around lines 40 - 66, Replace direct use of $this->entityManager->remove(...) in syncPageData with a repository-level deletion to keep persistence abstraction consistent: add a remove(SubscribePageData $pageData): void method to SubscriberPageDataRepository (or ensure it exists) and call $this->pageDataRepository->remove($pageData) inside SubscribePageManager::syncPageData instead of $this->entityManager->remove($pageData); leave persist calls as-is and do not change flushing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Domain/Subscription/Repository/SubscriberPageRepository.php`:
- Around line 92-106: The loadPageData method is dropping the first row's data
because array_shift($result)['page'] removes that row; update loadPageData
(method loadPageData, class SubscriberPageRepository, working with SubscribePage
and setData) to obtain the SubscribePage without mutating the $result array
(e.g. read $result[0]['page'] or otherwise extract 'page' from the first
element) and then iterate over the full $result to collect every 'data' entry
before calling $page->setData($data) so the first row's data is preserved.
- Around line 66-82: The grouping in getFilteredAfterId is creating
duplicate/invalid buckets by indexing both $page->getId() and $data->getId();
change the grouping so each bucket is keyed only by the page id (use
$page->getId()) and aggregate associated data entries into that same bucket
(append ['data'=> $data] to the page's array) while skipping data-only entries
when no page is present, so loadPageData always receives a group starting with a
'page' element; update getFilteredAfterId's loop to only create/append to
$grouped[$page->getId()] and stop creating $grouped[$data->getId()], ensuring
compatibility with SubscribePageData/SubscribePageManager::setPageData and
syncPageData expectations.
In `@src/Domain/Subscription/Service/Manager/SubscribePageManager.php`:
- Around line 55-60: The call to SubscribePageData::setId uses $page->getId()
which is ?int and can be null for a new page; update syncPageData to mirror the
existing setPageData behavior by passing (int)$page->getId() into
SubscribePageData::setId (or add an explicit guard/assert that the page is
persisted before calling syncPageData) so setId never receives null and avoids a
TypeError.
In `@tests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.php`:
- Around line 347-350: The test uses the deprecated withConsecutive on the
entity manager mock; replace that expectation by capturing the actual arguments
passed to EntityManagerInterface::remove via a willReturnCallback and then
assert the captured calls equal [$pageData1, $pageData2]. Concretely, in
SubscribePageManagerTest replace the ->withConsecutive([...]) on the
$this->entityManager->expects(...)->method('remove') mock with
->willReturnCallback(function($arg) use (&$calls) { $calls[] = $arg; }) and
after exercising the SUT assert that $calls contains $pageData1 then $pageData2.
---
Nitpick comments:
In `@src/Domain/Subscription/Repository/SubscriberPageRepository.php`:
- Around line 41-49: In getFilteredAfterId: cache $filter->getLastId() into a
local (e.g. $afterId) and use that variable for all subsequent calls to avoid
repeated method calls and potential mutation drift; also add a short comment
next to the $countQb = clone $queryBuilder (or adjust cloning placement)
clarifying whether $total is intended to be the overall count or the post-cursor
count—if you want total after the cursor, move the clone to after applying
andWhere('p.id > :afterId'), otherwise keep current clone and document the
semantics.
In `@src/Domain/Subscription/Service/Manager/SubscribePageManager.php`:
- Around line 40-66: Replace direct use of $this->entityManager->remove(...) in
syncPageData with a repository-level deletion to keep persistence abstraction
consistent: add a remove(SubscribePageData $pageData): void method to
SubscriberPageDataRepository (or ensure it exists) and call
$this->pageDataRepository->remove($pageData) inside
SubscribePageManager::syncPageData instead of
$this->entityManager->remove($pageData); leave persist calls as-is and do not
change flushing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d7ca3a94-7f3e-4081-b51b-c76c40753239
📒 Files selected for processing (4)
src/Domain/Subscription/Model/SubscribePage.phpsrc/Domain/Subscription/Repository/SubscriberPageRepository.phpsrc/Domain/Subscription/Service/Manager/SubscribePageManager.phptests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.php
| $grouped = []; | ||
| foreach ($rows as $row) { | ||
| /** @var SubscribePage $page */ | ||
| $page = $row['page'] ?? null; | ||
| $data = $row['data'] ?? null; | ||
| if ($page !== null) { | ||
| $grouped[$page->getId()][] = $row; | ||
| } | ||
| if ($data !== null) { | ||
| $grouped[$data->getId()][] = ['data' => $data]; | ||
| } | ||
| } | ||
|
|
||
| $pages = []; | ||
| foreach ($grouped as $group) { | ||
| $pages[] = $this->loadPageData($group); | ||
| } |
There was a problem hiding this comment.
Grouping in getFilteredAfterId double-buckets rows and can crash loadPageData.
Every row is appended twice into $grouped: once under $page->getId() with the full row, and once under $data->getId() with a ['data' => ...]-only entry. Because SubscribePageData::setId(...) is called with $page->getId() elsewhere (see SubscribePageManager::setPageData / syncPageData), $data->getId() collides with $page->getId(), so every page's bucket ends up with duplicated data items (each SubscribePageData gets counted twice).
And if data.id ever doesn't equal page.id, the second branch creates a group that has no 'page' key, and loadPageData()'s array_shift($result)['page'] then triggers an undefined-index access on a non-page entry.
🩹 Suggested rewrite — single bucket per page, aggregate data inline
- $grouped = [];
- foreach ($rows as $row) {
- /** `@var` SubscribePage $page */
- $page = $row['page'] ?? null;
- $data = $row['data'] ?? null;
- if ($page !== null) {
- $grouped[$page->getId()][] = $row;
- }
- if ($data !== null) {
- $grouped[$data->getId()][] = ['data' => $data];
- }
- }
-
- $pages = [];
- foreach ($grouped as $group) {
- $pages[] = $this->loadPageData($group);
- }
+ /** `@var` array<int, array{page: SubscribePage, data: SubscribePageData[]}> $grouped */
+ $grouped = [];
+ foreach ($rows as $row) {
+ $page = $row['page'] ?? null;
+ if ($page === null) {
+ continue;
+ }
+ $pageId = $page->getId();
+ if (!isset($grouped[$pageId])) {
+ $grouped[$pageId] = ['page' => $page, 'data' => []];
+ }
+ if (!empty($row['data'])) {
+ $grouped[$pageId]['data'][] = $row['data'];
+ }
+ }
+
+ $pages = [];
+ foreach ($grouped as $entry) {
+ $entry['page']->setData($entry['data']);
+ $pages[] = $entry['page'];
+ }📝 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.
| $grouped = []; | |
| foreach ($rows as $row) { | |
| /** @var SubscribePage $page */ | |
| $page = $row['page'] ?? null; | |
| $data = $row['data'] ?? null; | |
| if ($page !== null) { | |
| $grouped[$page->getId()][] = $row; | |
| } | |
| if ($data !== null) { | |
| $grouped[$data->getId()][] = ['data' => $data]; | |
| } | |
| } | |
| $pages = []; | |
| foreach ($grouped as $group) { | |
| $pages[] = $this->loadPageData($group); | |
| } | |
| /** `@var` array<int, array{page: SubscribePage, data: SubscribePageData[]}> $grouped */ | |
| $grouped = []; | |
| foreach ($rows as $row) { | |
| $page = $row['page'] ?? null; | |
| if ($page === null) { | |
| continue; | |
| } | |
| $pageId = $page->getId(); | |
| if (!isset($grouped[$pageId])) { | |
| $grouped[$pageId] = ['page' => $page, 'data' => []]; | |
| } | |
| if (!empty($row['data'])) { | |
| $grouped[$pageId]['data'][] = $row['data']; | |
| } | |
| } | |
| $pages = []; | |
| foreach ($grouped as $entry) { | |
| $entry['page']->setData($entry['data']); | |
| $pages[] = $entry['page']; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Domain/Subscription/Repository/SubscriberPageRepository.php` around lines
66 - 82, The grouping in getFilteredAfterId is creating duplicate/invalid
buckets by indexing both $page->getId() and $data->getId(); change the grouping
so each bucket is keyed only by the page id (use $page->getId()) and aggregate
associated data entries into that same bucket (append ['data'=> $data] to the
page's array) while skipping data-only entries when no page is present, so
loadPageData always receives a group starting with a 'page' element; update
getFilteredAfterId's loop to only create/append to $grouped[$page->getId()] and
stop creating $grouped[$data->getId()], ensuring compatibility with
SubscribePageData/SubscribePageManager::setPageData and syncPageData
expectations.
| private function loadPageData(array $result): SubscribePage | ||
| { | ||
| /** @var SubscribePage $page */ | ||
| $page = array_shift($result)['page']; | ||
|
|
||
| $data = []; | ||
| foreach ($result as $row) { | ||
| if (isset($row['data'])) { | ||
| $data[] = $row['data']; | ||
| } | ||
| } | ||
| $page->setData($data); | ||
|
|
||
| return $page; | ||
| } |
There was a problem hiding this comment.
loadPageData drops the first row's data.
array_shift($result)['page'] removes the entire first row and only keeps its page; the data value that came on that same row is silently discarded. The loop then continues from the second row, so any page that has ≥1 SubscribePageData will return missing exactly the first one. For a page with a single data row, getData() ends up as [].
🩹 Suggested fix — keep the whole result set when collecting `data`
- private function loadPageData(array $result): SubscribePage
- {
- /** `@var` SubscribePage $page */
- $page = array_shift($result)['page'];
-
- $data = [];
- foreach ($result as $row) {
- if (isset($row['data'])) {
- $data[] = $row['data'];
- }
- }
- $page->setData($data);
-
- return $page;
- }
+ private function loadPageData(array $result): SubscribePage
+ {
+ /** `@var` SubscribePage $page */
+ $page = $result[0]['page'];
+
+ $data = [];
+ foreach ($result as $row) {
+ if (!empty($row['data'])) {
+ $data[] = $row['data'];
+ }
+ }
+ $page->setData($data);
+
+ return $page;
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Domain/Subscription/Repository/SubscriberPageRepository.php` around lines
92 - 106, The loadPageData method is dropping the first row's data because
array_shift($result)['page'] removes that row; update loadPageData (method
loadPageData, class SubscriberPageRepository, working with SubscribePage and
setData) to obtain the SubscribePage without mutating the $result array (e.g.
read $result[0]['page'] or otherwise extract 'page' from the first element) and
then iterate over the full $result to collect every 'data' entry before calling
$page->setData($data) so the first row's data is preserved.
| $pageData = (new SubscribePageData()) | ||
| ->setId($page->getId()) | ||
| ->setName($pageDataKey) | ||
| ->setData($value); | ||
|
|
||
| $this->pageDataRepository->persist($pageData); |
There was a problem hiding this comment.
$page->getId() is ?int; type may bite if the page is fresh.
SubscribePage::getId() returns ?int. The pre-existing setPageData() method already guards against this with (int)$page->getId() (line 110). The new syncPageData() passes it raw, so if setId() is typed int, calling this for a not-yet-persisted page hands it null and throws a TypeError. If the contract is "page must already be persisted", a small assert/early return makes that explicit; otherwise just mirror the existing cast.
🩹 Suggested fix — mirror the existing cast
$pageData = (new SubscribePageData())
- ->setId($page->getId())
+ ->setId((int)$page->getId())
->setName($pageDataKey)
->setData($value);Quick check on SubscribePageData::setId's signature so we know which behavior to lean into:
#!/bin/bash
ast-grep --pattern $'class SubscribePageData {
$$$
public function setId($_) { $$$ }
$$$
}'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Domain/Subscription/Service/Manager/SubscribePageManager.php` around
lines 55 - 60, The call to SubscribePageData::setId uses $page->getId() which is
?int and can be null for a new page; update syncPageData to mirror the existing
setPageData behavior by passing (int)$page->getId() into
SubscribePageData::setId (or add an explicit guard/assert that the page is
persisted before calling syncPageData) so setId never receives null and avoids a
TypeError.
| $this->entityManager | ||
| ->expects($this->exactly(2)) | ||
| ->method('remove') | ||
| ->withConsecutive([$pageData1], [$pageData2]); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PHPUnit withConsecutive deprecation replacement
💡 Result:
The withConsecutive method was deprecated in PHPUnit 9.6 and removed in PHPUnit 10 due to fundamental design limitations regarding rigid sequential matching and difficulties with dynamic scenarios [1]. To replace withConsecutive, you should move away from verifying sequences using a single method call and instead use one of the following approaches depending on your specific testing needs: 1. Use standard PHPUnit constraints: For simple cases, you can often replace withConsecutive with multiple individual expectation setups or by using with combined with more flexible argument constraints. 2. Use willReturnCallback: This is the most robust way to replace withConsecutive for complex scenarios. You can use this to inspect arguments dynamically and return values based on the invocation number or the arguments received [2]. Example: $matcher = $this->exactly(2); $mock->expects($matcher) ->method('yourMethod') ->willReturnCallback(function ($arg) use ($matcher) { match ($matcher->numberOfInvocations) { 1 => $this->assertEquals('expected1', $arg), 2 => $this->assertEquals('expected2', $arg), }; return 'someResponse'; }); 3. Use dedicated libraries: If you rely heavily on this functionality, third-party extensions such as inspirum/phpunit-extension provide wrappers that re-implement the withConsecutive functionality for newer versions of PHPUnit [3]. 4. Leverage newer PHPUnit features: For modern PHPUnit versions, focus on writing tests that are expressive and maintainable. If you find yourself needing to mock complex sequences, consider whether the design of the code under test can be simplified to avoid the need for such brittle mock configurations [1].
Citations:
- 1: https://phpunit.expert/articles/better-than-withconsecutive.html
- 2: https://stackoverflow.com/questions/75389000/replace-phpunit-method-withconsecutive-abandoned-in-phpunit-10
- 3: https://github.com/inspirum/phpunit-extension
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show phpunit version constraints from composer.json/composer.lock if present
ls -la
if [ -f composer.json ]; then
echo "---- composer.json ----"
rg -n '"phpunit/phpunit"' composer.json || true
rg -n 'phpunit' composer.json || true
cat composer.json | sed -n '1,120p'
fi
if [ -f composer.lock ]; then
echo "---- composer.lock (phpunit entries) ----"
rg -n '"name": "phpunit/phpunit"' composer.lock | head -n 20 || true
# Try to extract the version near that entry
python3 - <<'PY'
import json,sys
p='composer.lock'
try:
with open(p,'r',encoding='utf-8') as f:
data=json.load(f)
except Exception as e:
print("Could not parse composer.lock:",e); sys.exit(0)
pkgs=[]
for sec in ('packages','packages-dev'):
for pkg in data.get(sec,[]):
if pkg.get('name')=='phpunit/phpunit':
pkgs.append(pkg)
print("phpunit/phpunit packages found:",len(pkgs))
for pkg in pkgs:
print(" - version:",pkg.get('version'),"requires:",pkg.get('require',{}).get('php','(n/a)'))
PY
fiRepository: phpList/core
Length of output: 5370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if [ -f composer.lock ]; then
echo "---- composer.lock exists ----"
python3 - <<'PY'
import json
p='composer.lock'
data=json.load(open(p,'r',encoding='utf-8'))
for sec in ('packages','packages-dev'):
for pkg in data.get(sec,[]):
if pkg.get('name')=='phpunit/phpunit':
print("phpunit/phpunit:",pkg.get('version'))
PY
else
echo "---- composer.lock not found ----"
fiRepository: phpList/core
Length of output: 90
withConsecutive() is heading for a future PHPUnit 10 break 😅
In tests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.php (lines 347-350), the test relies on ->withConsecutive(...), which PHPUnit deprecated in 9.6 and removed in 10. The repo currently allows PHPUnit via phpunit/phpunit: "^9.5" (no composer.lock), so a major bump to 10 will break this test.
🩹 Sketch — callback-based equivalent
- $this->entityManager
- ->expects($this->exactly(2))
- ->method('remove')
- ->withConsecutive([$pageData1], [$pageData2]);
+ $removed = [];
+ $this->entityManager
+ ->expects($this->exactly(2))
+ ->method('remove')
+ ->willReturnCallback(function ($arg) use (&$removed): void {
+ $removed[] = $arg;
+ });
+ // ... after the call:
+ // $this->assertSame([$pageData1, $pageData2], $removed);PHPUnit withConsecutive deprecation replacement
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.php`
around lines 347 - 350, The test uses the deprecated withConsecutive on the
entity manager mock; replace that expectation by capturing the actual arguments
passed to EntityManagerInterface::remove via a willReturnCallback and then
assert the captured calls equal [$pageData1, $pageData2]. Concretely, in
SubscribePageManagerTest replace the ->withConsecutive([...]) on the
$this->entityManager->expects(...)->method('remove') mock with
->willReturnCallback(function($arg) use (&$calls) { $calls[] = $arg; }) and
after exercising the SUT assert that $calls contains $pageData1 then $pageData2.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Domain/Subscription/Service/Manager/SubscribePageManager.php (1)
55-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
SubscribePage::$datain sync with the diff you just applied.
syncPageData()updates persistence state, but it never rebuilds the new in-memorySubscribePage::getData()container. After a create/delete path, callers can keep using$pageand still see stale entries until they reload it.💡 Minimal fix sketch
public function syncPageData(array $data, SubscribePage $page): void { $existingPageData = []; + $syncedPageData = []; foreach ($this->getPageData($page) as $pageData) { $existingPageData[$pageData->getName()] = $pageData; } foreach ($data as $pageDataKey => $value) { if (isset($existingPageData[$pageDataKey])) { $pageData = $existingPageData[$pageDataKey]; $pageData->setData($value); unset($existingPageData[$pageDataKey]); + $syncedPageData[] = $pageData; continue; } $pageData = (new SubscribePageData()) ->setId($page->getId()) ->setName($pageDataKey) ->setData($value); $this->pageDataRepository->persist($pageData); + $syncedPageData[] = $pageData; } foreach ($existingPageData as $pageData) { $this->entityManager->remove($pageData); } + + $page->setData($syncedPageData); if ($this->subscribePageConfigMigrationEnabled) { $this->configMigrationService->copyToConfig(page: $page, data: $data); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Domain/Subscription/Service/Manager/SubscribePageManager.php` around lines 55 - 84, syncPageData updates DB entries but never updates the in-memory SubscribePage::$data, so callers see stale data; after applying creates/updates/deletes in syncPageData (the method in SubscribePageManager) update the SubscribePage instance's data container to reflect the new state — e.g. rebuild the data array from the final $data input (or from the adjusted $existingPageData plus newly created entries) and call $page->setData(...) so SubscribePage::getData() returns the current values; ensure this happens just before calling configMigrationService->copyToConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Domain/Configuration/Service/Provider/ConfigProvider.php`:
- Around line 102-103: In ConfigProvider::getValue path where you call
getValue($parentKey), guard against boolean-style parent keys by wrapping the
call in a try/catch that specifically catches InvalidArgumentException and
returns null (or delegates to isEnabled handling) instead of letting the
exception bubble; locate the getValue($parentKey) invocation in ConfigProvider
and add the try/catch around it so boolean-like inputs such as
"maintenancemode:1" do not cause a runtime exception.
In `@src/Domain/Subscription/Service/SubscribePageConfigMigrationService.php`:
- Around line 82-84: The Domain service SubscribePageConfigMigrationService
currently calls $this->entityManager->flush() inside its migration method;
remove that flush so the domain service remains side-effect free
(prepare/persist/remove only) and instead return or expose the $hasChanges /
scheduled-operations result (e.g., boolean or array of changed entities) so the
Application/handler layer can call flush(); update the method in
SubscribePageConfigMigrationService to stop finalizing the DB and adjust tests
to assert the returned change state and that persistence was scheduled rather
than expecting flush() to have been invoked.
---
Outside diff comments:
In `@src/Domain/Subscription/Service/Manager/SubscribePageManager.php`:
- Around line 55-84: syncPageData updates DB entries but never updates the
in-memory SubscribePage::$data, so callers see stale data; after applying
creates/updates/deletes in syncPageData (the method in SubscribePageManager)
update the SubscribePage instance's data container to reflect the new state —
e.g. rebuild the data array from the final $data input (or from the adjusted
$existingPageData plus newly created entries) and call $page->setData(...) so
SubscribePage::getData() returns the current values; ensure this happens just
before calling configMigrationService->copyToConfig.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7dde6c6f-30e3-4851-9b36-e4c6f478cac1
📒 Files selected for processing (8)
config/parameters.yml.distconfig/services/services.ymlsrc/Domain/Configuration/Service/Provider/ConfigProvider.phpsrc/Domain/Subscription/Service/Manager/SubscribePageManager.phpsrc/Domain/Subscription/Service/SubscribePageConfigMigrationService.phptests/Unit/Domain/Configuration/Service/Provider/ConfigProviderTest.phptests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.phptests/Unit/Domain/Subscription/Service/SubscribePageConfigMigrationServiceTest.php
| if ($hasChanges) { | ||
| $this->entityManager->flush(); | ||
| } |
There was a problem hiding this comment.
Move transaction finalization out of the Domain service.
$this->entityManager->flush() at Line 83 performs DB finalization in a non-DynamicListAttr domain service. Keep this method side-effect free (prepare/schedule changes only) and flush in the Application/handler layer.
Suggested change
- if ($hasChanges) {
- $this->entityManager->flush();
- }
-
return $hasChanges;Downstream impact: tests currently asserting flush() here should be updated to assert only returned change state and scheduled persistence behavior.
As per coding guidelines, "src/Domain/**: ❌ Do not allow, flag ANY DB write / finalization: $entityManager->flush(...) … For non-DynamicListAttr code … move this to Infrastructure/Application layer."
📝 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.
| if ($hasChanges) { | |
| $this->entityManager->flush(); | |
| } | |
| return $hasChanges; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Domain/Subscription/Service/SubscribePageConfigMigrationService.php`
around lines 82 - 84, The Domain service SubscribePageConfigMigrationService
currently calls $this->entityManager->flush() inside its migration method;
remove that flush so the domain service remains side-effect free
(prepare/persist/remove only) and instead return or expose the $hasChanges /
scheduled-operations result (e.g., boolean or array of changed entities) so the
Application/handler layer can call flush(); update the method in
SubscribePageConfigMigrationService to stop finalizing the DB and adjust tests
to assert the returned change state and that persistence was scheduled rather
than expecting flush() to have been invoked.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
94-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle boolean parent keys in namespace fallback to prevent runtime exceptions.
At Line 102,
getValue($parentKey)can still throwInvalidArgumentExceptionfor boolean parent keys (for example,maintenancemode:1) and currently bubbles out.Proposed fix
if (str_contains($keyValue, ':')) { [$parent] = explode(':', $keyValue, 2); try { $parentKey = ConfigOption::from($parent); } catch (ValueError) { return null; } - return $this->getValue($parentKey); + try { + return $this->getValue($parentKey); + } catch (InvalidArgumentException) { + return null; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Domain/Configuration/Service/Provider/ConfigProvider.php` around lines 94 - 103, The namespace fallback branch in ConfigProvider tries to convert the parent token via ConfigOption::from($parent) and then calls $this->getValue($parentKey), but getValue can throw InvalidArgumentException for boolean-like parent keys (e.g., "maintenancemode:1") and currently bubbles up; wrap the $this->getValue($parentKey) call in a try/catch that catches InvalidArgumentException and returns null on failure so the method gracefully falls back, keeping the existing catch for ValueError around ConfigOption::from intact (refer to ConfigOption::from and ConfigProvider::getValue to locate the code).
🧹 Nitpick comments (1)
tests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.php (1)
71-76: 💤 Low value
withConsecutive()is deprecated in PHPUnit 9.6 and removed in 10.This new test uses the same deprecated pattern flagged elsewhere. Consider using a callback-based approach to future-proof the test.
Suggested refactor
+ $callCount = 0; $this->pageRepository ->expects($this->exactly(2)) ->method('findPageWithData') - ->withConsecutive([123], [123]) - ->willReturnOnConsecutiveCalls($page, $refetchedPage); + ->with(123) + ->willReturnCallback(function () use (&$callCount, $page, $refetchedPage) { + return ++$callCount === 1 ? $page : $refetchedPage; + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.php` around lines 71 - 76, The test currently uses the deprecated withConsecutive() on the mock for pageRepository->findPageWithData; replace this with a callback-based stub: remove withConsecutive() and use willReturnCallback on the mock for findPageWithData, implement a small closure that captures a local counter (e.g., $call = 0) and increments on each invocation, inspects the incoming argument(s) to ensure it's the expected page id (123) and returns $page on the first call and $refetchedPage on the second; keep the ->expects($this->exactly(2)) expectation and ensure the closure signature matches findPageWithData so the test behavior is identical without using withConsecutive().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/Domain/Configuration/Service/Provider/ConfigProvider.php`:
- Around line 94-103: The namespace fallback branch in ConfigProvider tries to
convert the parent token via ConfigOption::from($parent) and then calls
$this->getValue($parentKey), but getValue can throw InvalidArgumentException for
boolean-like parent keys (e.g., "maintenancemode:1") and currently bubbles up;
wrap the $this->getValue($parentKey) call in a try/catch that catches
InvalidArgumentException and returns null on failure so the method gracefully
falls back, keeping the existing catch for ValueError around ConfigOption::from
intact (refer to ConfigOption::from and ConfigProvider::getValue to locate the
code).
---
Nitpick comments:
In `@tests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.php`:
- Around line 71-76: The test currently uses the deprecated withConsecutive() on
the mock for pageRepository->findPageWithData; replace this with a
callback-based stub: remove withConsecutive() and use willReturnCallback on the
mock for findPageWithData, implement a small closure that captures a local
counter (e.g., $call = 0) and increments on each invocation, inspects the
incoming argument(s) to ensure it's the expected page id (123) and returns $page
on the first call and $refetchedPage on the second; keep the
->expects($this->exactly(2)) expectation and ensure the closure signature
matches findPageWithData so the test behavior is identical without using
withConsecutive().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85869e0d-e1b8-4472-8c17-e3523cddc4d3
📒 Files selected for processing (8)
config/parameters.yml.distconfig/services/services.ymlsrc/Domain/Configuration/Service/Provider/ConfigProvider.phpsrc/Domain/Subscription/Service/Manager/SubscribePageManager.phpsrc/Domain/Subscription/Service/SubscribePageConfigMigrationService.phptests/Unit/Domain/Configuration/Service/Provider/ConfigProviderTest.phptests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.phptests/Unit/Domain/Subscription/Service/SubscribePageConfigMigrationServiceTest.php
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests
Thanks for contributing to phpList!