Skip to content

Get existing page data only on edit page #39746

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.

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
wants to merge 8 commits into
base: 2.4-develop
Choose a base branch
from
57 changes: 18 additions & 39 deletions app/code/Magento/Cms/Model/Page/DataProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@
namespace Magento\Cms\Model\Page;

use Magento\Cms\Api\Data\PageInterface;
use Magento\Cms\Api\PageRepositoryInterface;
use Magento\Cms\Model\PageFactory;
use Magento\Cms\Model\Page\Service\PageService;
use Magento\Cms\Model\ResourceModel\Page\CollectionFactory;
use Magento\Framework\App\ObjectManager;
use Magento\Framework\App\Request\DataPersistorInterface;
Expand All @@ -33,11 +32,6 @@ class DataProvider extends ModifierPoolDataProvider
*/
protected $loadedData;

/**
* @var PageRepositoryInterface
*/
private $pageRepository;

/**
* @var AuthorizationInterface
*/
Expand All @@ -49,14 +43,9 @@ class DataProvider extends ModifierPoolDataProvider
private $request;

/**
* @var CustomLayoutManagerInterface
* @var PageService
*/
private $customLayoutManager;

/**
* @var PageFactory
*/
private $pageFactory;
private $pageService;

/**
* @var LoggerInterface
Expand All @@ -74,9 +63,7 @@ class DataProvider extends ModifierPoolDataProvider
* @param PoolInterface|null $pool
* @param AuthorizationInterface|null $auth
* @param RequestInterface|null $request
* @param CustomLayoutManagerInterface|null $customLayoutManager
* @param PageRepositoryInterface|null $pageRepository
* @param PageFactory|null $pageFactory
* @param PageService|null $pageService
* @param LoggerInterface|null $logger
* @SuppressWarnings(PHPMD.ExcessiveParameterList)
*/
Expand All @@ -91,9 +78,7 @@ public function __construct(
?PoolInterface $pool = null,
?AuthorizationInterface $auth = null,
?RequestInterface $request = null,
?CustomLayoutManagerInterface $customLayoutManager = null,
?PageRepositoryInterface $pageRepository = null,
?PageFactory $pageFactory = null,
?PageService $pageService = null,
?LoggerInterface $logger = null
) {
parent::__construct($name, $primaryFieldName, $requestFieldName, $meta, $data, $pool);
Expand All @@ -102,10 +87,7 @@ public function __construct(
$this->auth = $auth ?? ObjectManager::getInstance()->get(AuthorizationInterface::class);
$this->meta = $this->prepareMeta($this->meta);
$this->request = $request ?? ObjectManager::getInstance()->get(RequestInterface::class);
$this->customLayoutManager = $customLayoutManager
?? ObjectManager::getInstance()->get(CustomLayoutManagerInterface::class);
$this->pageRepository = $pageRepository ?? ObjectManager::getInstance()->get(PageRepositoryInterface::class);
$this->pageFactory = $pageFactory ?: ObjectManager::getInstance()->get(PageFactory::class);
$this->pageService = $pageService ?: ObjectManager::getInstance()->get(PageService::class);
$this->logger = $logger ?: ObjectManager::getInstance()->get(LoggerInterface::class);
}

Expand Down Expand Up @@ -150,22 +132,16 @@ private function getCurrentPage(): PageInterface
{
$pageId = $this->getPageId();
if ($pageId) {
try {
$page = $this->pageRepository->getById($pageId);
} catch (LocalizedException $exception) {
$page = $this->pageFactory->create();
}

return $page;
return $this->pageService->getPageById($pageId);
}

$data = $this->dataPersistor->get('cms_page');
if (empty($data)) {
return $this->pageFactory->create();
return $this->pageService->createPageFactory();
}
$this->dataPersistor->clear('cms_page');

return $this->pageFactory->create()
return $this->pageService->createPageFactory()
->setData($data);
}

Expand Down Expand Up @@ -215,12 +191,15 @@ public function getMeta()

$page = null;
try {
$page = $this->pageRepository->getById($this->getPageId());
if ($page->getCustomLayoutUpdateXml() || $page->getLayoutUpdateXml()) {
$options[] = ['label' => 'Use existing layout update XML', 'value' => '_existing_'];
}
foreach ($this->customLayoutManager->fetchAvailableFiles($page) as $layoutFile) {
$options[] = ['label' => $layoutFile, 'value' => $layoutFile];
$pageId = $this->getPageId();
if ($pageId) {
$page = $this->pageService->getPageById($pageId);
if ($page->getCustomLayoutUpdateXml() || $page->getLayoutUpdateXml()) {
$options[] = ['label' => 'Use existing layout update XML', 'value' => '_existing_'];
}
foreach ($this->pageService->fetchAvailableCustomLayouts($page) as $layoutFile) {
$options[] = ['label' => $layoutFile, 'value' => $layoutFile];
}
}
} catch (LocalizedException $e) {
$this->logger->error($e->getMessage());
Expand Down
67 changes: 67 additions & 0 deletions app/code/Magento/Cms/Model/Page/Service/PageService.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
<?php
declare(strict_types=1);
/**
* Copyright 2015 Adobe
* All Rights Reserved.
*/
namespace Magento\Cms\Model\Page\Service;

use Magento\Cms\Api\Data\PageInterface;
use Magento\Cms\Api\PageRepositoryInterface;
use Magento\Cms\Model\Page\CustomLayoutManagerInterface;
use Magento\Cms\Model\PageFactory;
use Magento\Framework\Exception\LocalizedException;

/**
* Cms Page Service Class
*/
class PageService
Copy link
Contributor

Choose a reason for hiding this comment

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

  • PageService can be unit tested for the logic of getPageById, createPageFactory, and fetchAvailableCustomLayouts.
  • Integration tests could ensure data provider works as expected in edit page scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@engcom-Hotel i have added unit test case, please review

{
/**
* @param PageRepositoryInterface $pageRepository
* @param PageFactory $pageFactory
* @param CustomLayoutManagerInterface $customLayoutManager
*/
public function __construct(
private readonly PageRepositoryInterface $pageRepository,
private readonly PageFactory $pageFactory,
private readonly CustomLayoutManagerInterface $customLayoutManager,
) {
}

/**
* To get the page by its ID. If the page not exists, a new page instance is returned
*
* @param int $id
* @return PageInterface
*/
public function getPageById(int $id): PageInterface
{
try {
return $this->pageRepository->getById($id);
} catch (LocalizedException) {
return $this->pageFactory->create();
}
}

/**
* To create pagefactory class
*
* @return PageInterface
*/
public function createPageFactory(): PageInterface
{
return $this->pageFactory->create();
}

/**
* Fetches the available custom layouts for a given page.
*
* @param PageInterface $page
* @return array
*/
public function fetchAvailableCustomLayouts(PageInterface $page): array
{
return $this->customLayoutManager->fetchAvailableFiles($page);
}
}
182 changes: 182 additions & 0 deletions app/code/Magento/Cms/Test/Unit/Model/Page/Service/PageServiceTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
<?php
/**
* Copyright 2025 Adobe
* All Rights Reserved.
*/
declare(strict_types=1);

namespace Magento\Cms\Test\Unit\Model\Page\Service;

use Magento\Cms\Api\Data\PageInterface;
use Magento\Cms\Api\PageRepositoryInterface;
use Magento\Cms\Model\Page\CustomLayoutManagerInterface;
use Magento\Cms\Model\Page\Service\PageService;
use Magento\Cms\Model\PageFactory;
use Magento\Framework\Exception\LocalizedException;
use Magento\Framework\Exception\NoSuchEntityException;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;

/**
* Unit tests for PageService
*/
class PageServiceTest extends TestCase
{
/**
* @var PageRepositoryInterface|MockObject
*/
private $pageRepositoryMock;

/**
* @var PageFactory|MockObject
*/
private $pageFactoryMock;

/**
* @var CustomLayoutManagerInterface|MockObject
*/
private $customLayoutManagerMock;

/**
* @var PageService
*/
private $pageService;

/**
* @var PageInterface|MockObject
*/
private $pageMock;

/**
* Set up test environment
*/
protected function setUp(): void
{
$this->pageRepositoryMock = $this->createMock(PageRepositoryInterface::class);
$this->pageFactoryMock = $this->createMock(PageFactory::class);
$this->customLayoutManagerMock = $this->createMock(CustomLayoutManagerInterface::class);
$this->pageMock = $this->createMock(PageInterface::class);

$this->pageService = new PageService(
$this->pageRepositoryMock,
$this->pageFactoryMock,
$this->customLayoutManagerMock
);
}

/**
* Test getPageById returns existing page when page exists
*/
public function testGetPageByIdReturnsExistingPage(): void
{
$pageId = 1;

$this->pageRepositoryMock->expects($this->once())
->method('getById')
->with($pageId)
->willReturn($this->pageMock);

$this->pageFactoryMock->expects($this->never())
->method('create');

$result = $this->pageService->getPageById($pageId);

$this->assertSame($this->pageMock, $result);
}

/**
* Test getPageById returns new page instance when page doesn't exist
*/
public function testGetPageByIdReturnsNewPageWhenNotExists(): void
{
$pageId = 999;
$newPageMock = $this->createMock(PageInterface::class);

$this->pageRepositoryMock->expects($this->once())
->method('getById')
->with($pageId)
->willThrowException(new NoSuchEntityException(__('Page not found')));

$this->pageFactoryMock->expects($this->once())
->method('create')
->willReturn($newPageMock);

$result = $this->pageService->getPageById($pageId);

$this->assertSame($newPageMock, $result);
}

/**
* Test getPageById handles LocalizedException and returns new page
*/
public function testGetPageByIdHandlesLocalizedException(): void
{
$pageId = 1;
$newPageMock = $this->createMock(PageInterface::class);

$this->pageRepositoryMock->expects($this->once())
->method('getById')
->with($pageId)
->willThrowException(new LocalizedException(__('Some localized error')));

$this->pageFactoryMock->expects($this->once())
->method('create')
->willReturn($newPageMock);

$result = $this->pageService->getPageById($pageId);

$this->assertSame($newPageMock, $result);
}

/**
* Test createPageFactory returns new page instance
*/
public function testCreatePageFactory(): void
{
$newPageMock = $this->createMock(PageInterface::class);

$this->pageFactoryMock->expects($this->once())
->method('create')
->willReturn($newPageMock);

$result = $this->pageService->createPageFactory();

$this->assertSame($newPageMock, $result);
}

/**
* Test fetchAvailableCustomLayouts returns available layouts
*/
public function testFetchAvailableCustomLayouts(): void
{
$expectedLayouts = [
'layout1.xml',
'layout2.xml',
'custom_layout.xml'
];

$this->customLayoutManagerMock->expects($this->once())
->method('fetchAvailableFiles')
->with($this->pageMock)
->willReturn($expectedLayouts);

$result = $this->pageService->fetchAvailableCustomLayouts($this->pageMock);

$this->assertSame($expectedLayouts, $result);
}

/**
* Test fetchAvailableCustomLayouts returns empty array when no layouts available
*/
public function testFetchAvailableCustomLayoutsReturnsEmptyArray(): void
{
$this->customLayoutManagerMock->expects($this->once())
->method('fetchAvailableFiles')
->with($this->pageMock)
->willReturn([]);

$result = $this->pageService->fetchAvailableCustomLayouts($this->pageMock);

$this->assertSame([], $result);
}
}