Skip to content

Dev #384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
TatevikGr wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Dev #384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
16d3809
remove extra code
tatevikg1 May 20, 2026
e876320
syncPageData
tatevikg1 May 21, 2026
4a1084b
load data into page
tatevikg1 May 21, 2026
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
16 changes: 16 additions & 0 deletions src/Domain/Subscription/Model/SubscribePage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ class SubscribePage implements DomainModel, Identity, OwnableInterface
#[ORM\JoinColumn(name: 'owner', referencedColumnName: 'id', nullable: true)]
private ?Administrator $owner = null;

/** @var SubscribePageData[] */
private array $data = [];

public function getId(): ?int
{
return $this->id;
Expand All @@ -50,6 +53,12 @@ public function getOwner(): ?Administrator
return $this->owner;
}

/** @return SubscribePageData[] */
public function getData(): array
{
return $this->data;
}

public function setTitle(string $title): self
{
$this->title = $title;
Expand All @@ -67,4 +76,11 @@ public function setOwner(?Administrator $owner): self
$this->owner = $owner;
return $this;
}

/** @param SubscribePageData[] $data */
public function setData(array $data): self
{
$this->data = $data;
return $this;
}
}
91 changes: 84 additions & 7 deletions src/Domain/Subscription/Repository/SubscriberPageRepository.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

namespace PhpList\Core\Domain\Subscription\Repository;

use PhpList\Core\Domain\Common\Model\Filter\FilterRequestInterface;
use PhpList\Core\Domain\Common\Model\PaginatedResult;
use PhpList\Core\Domain\Common\Repository\AbstractRepository;
use PhpList\Core\Domain\Common\Repository\CursorPaginationTrait;
use PhpList\Core\Domain\Common\Repository\Interfaces\PaginatableRepositoryInterface;
Expand All @@ -14,17 +16,92 @@ class SubscriberPageRepository extends AbstractRepository implements Paginatable
{
use CursorPaginationTrait;

/** @return array{page: SubscribePage, data: SubscribePageData}[] */
public function findPagesWithData(int $pageId): array
public function findPageWithData(int $pageId): ?SubscribePage
{
return $this->createQueryBuilder('p')
->select('p AS page, d AS data')
->from(SubscribePage::class, 'p')
->from(SubscribePageData::class, 'd')
$result = $this->createQueryBuilder('p')
->select('p AS page', 'd AS data')
->leftJoin(
SubscribePageData::class,
'd',
'ON',
'd.id = p.id'
)
->where('p.id = :id')
->andWhere('d.id = p.id')
->setParameter('id', $pageId)
->getQuery()
->getResult();

if ($result === []) {
return null;
}

return $this->loadPageData($result);
}

public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedResult
{
$queryBuilder = $this->createQueryBuilder('p');

$countQb = clone $queryBuilder;
$total = (int) $countQb
->select('COUNT(DISTINCT p.id)')
->getQuery()
->getSingleScalarResult();

$rows = $queryBuilder
->select('p AS page', 'd AS data')
->leftJoin(
SubscribePageData::class,
'd',
'ON',
'd.id = p.id'
)
->andWhere('p.id > :afterId')
->setParameter('afterId', $filter->getLastId())
->orderBy('p.id', 'ASC')
->setMaxResults($filter->getLimit())
->getQuery()
->getResult();

$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);
}
Comment on lines +66 to +82
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

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.

Suggested change
$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.


return new PaginatedResult(
items: array_values($pages),
total: $total,
limit: $filter->getLimit(),
lastId: $filter->getLastId(),
);
}

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;
}
Comment on lines +92 to 106
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

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.

}
39 changes: 29 additions & 10 deletions src/Domain/Subscription/Service/Manager/SubscribePageManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,21 @@
use PhpList\Core\Domain\Subscription\Model\SubscribePageData;
use PhpList\Core\Domain\Subscription\Repository\SubscriberPageDataRepository;
use PhpList\Core\Domain\Subscription\Repository\SubscriberPageRepository;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
use Symfony\Contracts\Translation\TranslatorInterface;

class SubscribePageManager
{
public function __construct(
private readonly SubscriberPageRepository $pageRepository,
private readonly SubscriberPageDataRepository $pageDataRepository,
private readonly EntityManagerInterface $entityManager,
private readonly TranslatorInterface $translator,
) {
}

public function findPage(int $id): ?SubscribePage
{
return $this->pageRepository->findPageWithData($id);
}

public function createPage(string $title, bool $active = false, ?Administrator $owner = null): SubscribePage
{
$page = new SubscribePage();
Expand All @@ -35,15 +37,32 @@ public function createPage(string $title, bool $active = false, ?Administrator $
return $page;
}

public function getPage(int $id): SubscribePage
public function syncPageData(array $data, SubscribePage $page): void
{
/** @var SubscribePage|null $page */
$page = $this->pageRepository->find($id);
if (!$page) {
throw new NotFoundHttpException($this->translator->trans('Subscribe page not found'));
$existingPageData = [];
foreach ($this->getPageData($page) as $pageData) {
$existingPageData[$pageData->getName()] = $pageData;
}

return $page;
foreach ($data as $pageDataKey => $value) {
if (isset($existingPageData[$pageDataKey])) {
$pageData = $existingPageData[$pageDataKey];
$pageData->setData($value);
unset($existingPageData[$pageDataKey]);
continue;
}

$pageData = (new SubscribePageData())
->setId($page->getId())
->setName($pageDataKey)
->setData($value);

$this->pageDataRepository->persist($pageData);
Comment on lines +55 to +60
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

$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.

}

foreach ($existingPageData as $pageData) {
$this->entityManager->remove($pageData);
}
}

public function updatePage(
Expand Down Expand Up @@ -78,7 +97,7 @@ public function deletePage(SubscribePage $page): void
/** @return SubscribePageData[] */
public function getPageData(SubscribePage $page): array
{
return $this->pageDataRepository->getByPage($page,);
return $this->pageDataRepository->getByPage($page);
}

public function setPageData(SubscribePage $page, string $name, ?string $value): SubscribePageData
Expand Down
Loading
Loading