From 8a2af0925dfb1ec5c556607848c8b5cc193413c6 Mon Sep 17 00:00:00 2001 From: Mohamed Azarudeen Date: Tue, 18 Mar 2025 14:51:38 +0530 Subject: [PATCH 1/4] Get existing page data only on edit page --- .../Magento/Cms/Model/Page/DataProvider.php | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/app/code/Magento/Cms/Model/Page/DataProvider.php b/app/code/Magento/Cms/Model/Page/DataProvider.php index ecdf3bc2bdd97..8ce81bedd81e9 100644 --- a/app/code/Magento/Cms/Model/Page/DataProvider.php +++ b/app/code/Magento/Cms/Model/Page/DataProvider.php @@ -1,7 +1,7 @@ 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->pageRepository->getById($pageId); + 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]; + } } } catch (LocalizedException $e) { $this->logger->error($e->getMessage()); From f8474ec8ca63b9f97b3a3b0552a47769c5f1f477 Mon Sep 17 00:00:00 2001 From: Mohamed Azarudeen Date: Fri, 23 May 2025 15:44:37 +0530 Subject: [PATCH 2/4] Static test fix - reduce coupling in page data provider with service --- .../Magento/Cms/Model/Page/DataProvider.php | 46 ++++--------- .../Cms/Model/Page/Service/PageService.php | 66 +++++++++++++++++++ 2 files changed, 77 insertions(+), 35 deletions(-) create mode 100644 app/code/Magento/Cms/Model/Page/Service/PageService.php diff --git a/app/code/Magento/Cms/Model/Page/DataProvider.php b/app/code/Magento/Cms/Model/Page/DataProvider.php index 8ce81bedd81e9..0f3d724e37d99 100644 --- a/app/code/Magento/Cms/Model/Page/DataProvider.php +++ b/app/code/Magento/Cms/Model/Page/DataProvider.php @@ -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; @@ -33,11 +32,6 @@ class DataProvider extends ModifierPoolDataProvider */ protected $loadedData; - /** - * @var PageRepositoryInterface - */ - private $pageRepository; - /** * @var AuthorizationInterface */ @@ -49,14 +43,9 @@ class DataProvider extends ModifierPoolDataProvider private $request; /** - * @var CustomLayoutManagerInterface - */ - private $customLayoutManager; - - /** - * @var PageFactory + * @var PageService */ - private $pageFactory; + private $pageService; /** * @var LoggerInterface @@ -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) */ @@ -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); @@ -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); } @@ -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); } @@ -217,11 +193,11 @@ public function getMeta() try { $pageId = $this->getPageId(); if ($pageId) { - $page = $this->pageRepository->getById($pageId); + $page = $this->pageService->getPageById($pageId); if ($page->getCustomLayoutUpdateXml() || $page->getLayoutUpdateXml()) { $options[] = ['label' => 'Use existing layout update XML', 'value' => '_existing_']; } - foreach ($this->customLayoutManager->fetchAvailableFiles($page) as $layoutFile) { + foreach ($this->pageService->fetchAvailableCustomLayouts($page) as $layoutFile) { $options[] = ['label' => $layoutFile, 'value' => $layoutFile]; } } diff --git a/app/code/Magento/Cms/Model/Page/Service/PageService.php b/app/code/Magento/Cms/Model/Page/Service/PageService.php new file mode 100644 index 0000000000000..8634b1b281bbd --- /dev/null +++ b/app/code/Magento/Cms/Model/Page/Service/PageService.php @@ -0,0 +1,66 @@ +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); + } +} From 2c1b4db68b2a4f240f374e652bce305bf3d4d8c7 Mon Sep 17 00:00:00 2001 From: Mohamed Azarudeen Date: Fri, 23 May 2025 20:44:43 +0530 Subject: [PATCH 3/4] Strict types added --- app/code/Magento/Cms/Model/Page/Service/PageService.php | 1 + 1 file changed, 1 insertion(+) diff --git a/app/code/Magento/Cms/Model/Page/Service/PageService.php b/app/code/Magento/Cms/Model/Page/Service/PageService.php index 8634b1b281bbd..27649c598734a 100644 --- a/app/code/Magento/Cms/Model/Page/Service/PageService.php +++ b/app/code/Magento/Cms/Model/Page/Service/PageService.php @@ -1,4 +1,5 @@ Date: Mon, 2 Jun 2025 19:46:20 +0530 Subject: [PATCH 4/4] Unit test added for page service class and code review changes --- .../Cms/Model/Page/Service/PageService.php | 6 +- .../Model/Page/Service/PageServiceTest.php | 182 ++++++++++++++++++ 2 files changed, 185 insertions(+), 3 deletions(-) create mode 100644 app/code/Magento/Cms/Test/Unit/Model/Page/Service/PageServiceTest.php diff --git a/app/code/Magento/Cms/Model/Page/Service/PageService.php b/app/code/Magento/Cms/Model/Page/Service/PageService.php index 27649c598734a..cb318369d0cb5 100644 --- a/app/code/Magento/Cms/Model/Page/Service/PageService.php +++ b/app/code/Magento/Cms/Model/Page/Service/PageService.php @@ -23,9 +23,9 @@ class PageService * @param CustomLayoutManagerInterface $customLayoutManager */ public function __construct( - private PageRepositoryInterface $pageRepository, - private PageFactory $pageFactory, - private CustomLayoutManagerInterface $customLayoutManager, + private readonly PageRepositoryInterface $pageRepository, + private readonly PageFactory $pageFactory, + private readonly CustomLayoutManagerInterface $customLayoutManager, ) { } diff --git a/app/code/Magento/Cms/Test/Unit/Model/Page/Service/PageServiceTest.php b/app/code/Magento/Cms/Test/Unit/Model/Page/Service/PageServiceTest.php new file mode 100644 index 0000000000000..1c72b2f72d189 --- /dev/null +++ b/app/code/Magento/Cms/Test/Unit/Model/Page/Service/PageServiceTest.php @@ -0,0 +1,182 @@ +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); + } +}