Conversation
📝 WalkthroughWalkthroughAdds SubscribePage.data with accessors; repository gains findPageWithData and cursor pagination; SubscribePageManager removes Translator, exposes findPage(?), adds syncPageData to create/update/remove page data, and updates getPageData usage. Tests remove getPage() checks and add syncPageData unit tests. ChangesSubscription page model, repository, manager, 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.
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests
Thanks for contributing to phpList!