From 58d6ed4c18f384b84cfc47bbde53a00c8d049725 Mon Sep 17 00:00:00 2001 From: Ruslan Hanzhuiev Date: Fri, 27 Sep 2019 17:45:44 +0300 Subject: [PATCH 1/6] Fixed bug when static content deploys including static data from disabled modules. (cherry picked from commit e0a13e965bb22ae15ece66c39089addf5853b16c) --- .../Css/PreProcessor/Instruction/MagentoImport.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/internal/Magento/Framework/Css/PreProcessor/Instruction/MagentoImport.php b/lib/internal/Magento/Framework/Css/PreProcessor/Instruction/MagentoImport.php index db30eb4844edb..7be6b5c41b487 100644 --- a/lib/internal/Magento/Framework/Css/PreProcessor/Instruction/MagentoImport.php +++ b/lib/internal/Magento/Framework/Css/PreProcessor/Instruction/MagentoImport.php @@ -13,6 +13,7 @@ use Magento\Framework\View\Design\Theme\ThemeProviderInterface; use Magento\Framework\View\DesignInterface; use Magento\Framework\View\File\CollectorInterface; +use Magento\Framework\Module\ModuleManagerInterface; /** * @magento_import instruction preprocessor @@ -57,25 +58,33 @@ class MagentoImport implements PreProcessorInterface */ private $themeProvider; + /** + * @var ModuleManagerInterface + */ + private $moduleManager; + /** * @param DesignInterface $design * @param CollectorInterface $fileSource * @param ErrorHandlerInterface $errorHandler * @param \Magento\Framework\View\Asset\Repository $assetRepo * @param \Magento\Framework\View\Design\Theme\ListInterface $themeList + * @param ModuleManagerInterface $moduleManager */ public function __construct( DesignInterface $design, CollectorInterface $fileSource, ErrorHandlerInterface $errorHandler, \Magento\Framework\View\Asset\Repository $assetRepo, - \Magento\Framework\View\Design\Theme\ListInterface $themeList + \Magento\Framework\View\Design\Theme\ListInterface $themeList, + ModuleManagerInterface $moduleManager ) { $this->design = $design; $this->fileSource = $fileSource; $this->errorHandler = $errorHandler; $this->assetRepo = $assetRepo; $this->themeList = $themeList; + $this->moduleManager = $moduleManager; } /** @@ -108,6 +117,9 @@ protected function replace(array $matchedContent, LocalInterface $asset) $importFiles = $this->fileSource->getFiles($this->getTheme($relatedAsset), $resolvedPath); /** @var $importFile \Magento\Framework\View\File */ foreach ($importFiles as $importFile) { + if ($importFile->getModule() && !$this->moduleManager->isEnabled($importFile->getModule())) { + continue; + } $referenceString = $isReference ? '(reference) ' : ''; $importsContent .= $importFile->getModule() ? "@import $referenceString'{$importFile->getModule()}::{$resolvedPath}';\n" From ec26f471bde2a44268fe294bb64494cf3061318d Mon Sep 17 00:00:00 2001 From: Ruslan Hanzhuiev Date: Mon, 30 Sep 2019 11:53:54 +0300 Subject: [PATCH 2/6] fixed dependency injection according to Backward Compatible Development Guide (cherry picked from commit 6181f8c907c10b353b4139b3bcdc7fb433cc2d8b) --- .../Framework/Css/PreProcessor/Instruction/MagentoImport.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/Magento/Framework/Css/PreProcessor/Instruction/MagentoImport.php b/lib/internal/Magento/Framework/Css/PreProcessor/Instruction/MagentoImport.php index 7be6b5c41b487..d3501136f8cc5 100644 --- a/lib/internal/Magento/Framework/Css/PreProcessor/Instruction/MagentoImport.php +++ b/lib/internal/Magento/Framework/Css/PreProcessor/Instruction/MagentoImport.php @@ -77,14 +77,14 @@ public function __construct( ErrorHandlerInterface $errorHandler, \Magento\Framework\View\Asset\Repository $assetRepo, \Magento\Framework\View\Design\Theme\ListInterface $themeList, - ModuleManagerInterface $moduleManager + ModuleManagerInterface $moduleManager = null ) { $this->design = $design; $this->fileSource = $fileSource; $this->errorHandler = $errorHandler; $this->assetRepo = $assetRepo; $this->themeList = $themeList; - $this->moduleManager = $moduleManager; + $this->moduleManager = $moduleManager ?: ObjectManager::getInstance()->get(ModuleManagerInterface::class); } /** From 04a8963b5bdf787cc78857447bcc0d7f93f2be67 Mon Sep 17 00:00:00 2001 From: Echron Date: Mon, 21 Oct 2019 11:45:57 +0200 Subject: [PATCH 3/6] Improve unittests + phpdoc for "Fixed bug when static content deploys including static data from disabled modules." --- .../Instruction/MagentoImport.php | 6 +- .../Instruction/MagentoImportTest.php | 80 ++++++++++++++++--- 2 files changed, 75 insertions(+), 11 deletions(-) diff --git a/lib/internal/Magento/Framework/Css/PreProcessor/Instruction/MagentoImport.php b/lib/internal/Magento/Framework/Css/PreProcessor/Instruction/MagentoImport.php index d3501136f8cc5..8321e137c71b1 100644 --- a/lib/internal/Magento/Framework/Css/PreProcessor/Instruction/MagentoImport.php +++ b/lib/internal/Magento/Framework/Css/PreProcessor/Instruction/MagentoImport.php @@ -16,6 +16,8 @@ use Magento\Framework\Module\ModuleManagerInterface; /** + * Processes import statements for css files + * * @magento_import instruction preprocessor * @SuppressWarnings(PHPMD.CouplingBetweenObjects) Must be deleted after moving themeProvider to construct */ @@ -88,7 +90,7 @@ public function __construct( } /** - * {@inheritdoc} + * @inheritdoc */ public function process(\Magento\Framework\View\Asset\PreProcessor\Chain $chain) { @@ -149,6 +151,8 @@ protected function getTheme(LocalInterface $asset) } /** + * Get theme provider + * * @return ThemeProviderInterface * @deprecated 100.1.1 */ diff --git a/lib/internal/Magento/Framework/Css/Test/Unit/PreProcessor/Instruction/MagentoImportTest.php b/lib/internal/Magento/Framework/Css/Test/Unit/PreProcessor/Instruction/MagentoImportTest.php index e5820bc92cd98..8c6fe4083a5b9 100644 --- a/lib/internal/Magento/Framework/Css/Test/Unit/PreProcessor/Instruction/MagentoImportTest.php +++ b/lib/internal/Magento/Framework/Css/Test/Unit/PreProcessor/Instruction/MagentoImportTest.php @@ -7,6 +7,7 @@ namespace Magento\Framework\Css\Test\Unit\PreProcessor\Instruction; use Magento\Framework\Css\PreProcessor\Instruction\MagentoImport; +use Magento\Framework\Module\ModuleManagerInterface; use Magento\Framework\TestFramework\Unit\Helper\ObjectManager; use Magento\Framework\View\Design\Theme\ThemeProviderInterface; @@ -45,6 +46,10 @@ class MagentoImportTest extends \PHPUnit\Framework\TestCase */ private $themeProvider; + /** + * @var ModuleManagerInterface||\PHPUnit_Framework_MockObject_MockObject + */ + private $moduleManager; /** * @var \Magento\Framework\Css\PreProcessor\Instruction\Import */ @@ -61,12 +66,15 @@ protected function setUp() $this->asset->expects($this->any())->method('getContentType')->will($this->returnValue('css')); $this->assetRepo = $this->createMock(\Magento\Framework\View\Asset\Repository::class); $this->themeProvider = $this->createMock(ThemeProviderInterface::class); + $this->moduleManager = $this->createMock(ModuleManagerInterface::class); $this->object = (new ObjectManager($this))->getObject(MagentoImport::class, [ 'design' => $this->design, 'fileSource' => $this->fileSource, 'errorHandler' => $this->errorHandler, 'assetRepo' => $this->assetRepo, - 'themeProvider' => $this->themeProvider + 'themeProvider' => $this->themeProvider, + 'moduleManager' => $this->moduleManager + ]); } @@ -94,16 +102,23 @@ public function testProcess($originalContent, $foundPath, $resolvedPath, $foundF $relatedAsset->expects($this->once())->method('getContext')->will($this->returnValue($context)); $theme = $this->getMockForAbstractClass(\Magento\Framework\View\Design\ThemeInterface::class); $this->themeProvider->expects($this->once())->method('getThemeByFullPath')->will($this->returnValue($theme)); + + $this->moduleManager->expects($this->any())->method('isEnabled')->will($this->returnValue(true)); + $files = []; foreach ($foundFiles as $file) { $fileObject = $this->createMock(\Magento\Framework\View\File::class); $fileObject->expects($this->any()) - ->method('getModule') - ->will($this->returnValue($file['module'])); + ->method('getModule') + ->will($this->returnValue($file['module'])); $fileObject->expects($this->any()) - ->method('getFilename') - ->will($this->returnValue($file['filename'])); - $files[] = $fileObject; + ->method('getFilename') + ->will($this->returnValue($file['filename'])); + + if(\is_null($file['module']) || (isset($file['isEnabled']) && $file['isEnabled'] === true)) + { + $files[] = $fileObject; + } } $this->fileSource->expects($this->once()) ->method('getFiles') @@ -135,11 +150,33 @@ public function processDataProvider() 'Magento_Module::some/file.css', 'some/file.css', [ - ['module' => 'Magento_Module', 'filename' => 'some/file.css'], - ['module' => 'Magento_Two', 'filename' => 'some/file.css'], + ['module' => 'Magento_Module','isEnabled' => true, 'filename' => 'some/file.css'], + ['module' => 'Magento_Two','isEnabled' => true, 'filename' => 'some/file.css'], ], "@import 'Magento_Module::some/file.css';\n@import 'Magento_Two::some/file.css';\n", ], + 'modular one module disabled' => [ + '//@magento_import "Magento_Module::some/file.css";', + 'Magento_Module::some/file.css', + 'some/file.css', + [ + ['module' => 'Magento_Module','isEnabled' => true, 'filename' => 'some/file.css'], + ['module' => 'Magento_Two','isEnabled' => true, 'filename' => 'some/file.css'], + ['module' => 'Magento_Disabled','isEnabled' => false, 'filename' => 'some/file.css'], + ], + "@import 'Magento_Module::some/file.css';\n@import 'Magento_Two::some/file.css';\n", + ], + 'modular all modules disabled' => [ + '//@magento_import "Magento_Module::some/file.css";', + 'Magento_Module::some/file.css', + 'some/file.css', + [ + ['module' => 'Magento_Module','isEnabled' => false, 'filename' => 'some/file.css'], + ['module' => 'Magento_Two','isEnabled' => false, 'filename' => 'some/file.css'], + ['module' => 'Magento_Disabled','isEnabled' => false, 'filename' => 'some/file.css'], + ], + "", + ], 'non-modular reference notation' => [ '//@magento_import (reference) "some/file.css";', 'some/file.css', @@ -155,12 +192,35 @@ public function processDataProvider() 'Magento_Module::some/file.css', 'some/file.css', [ - ['module' => 'Magento_Module', 'filename' => 'some/file.css'], - ['module' => 'Magento_Two', 'filename' => 'some/file.css'], + ['module' => 'Magento_Module','isEnabled' => true, 'filename' => 'some/file.css'], + ['module' => 'Magento_Two','isEnabled' => true, 'filename' => 'some/file.css'], + ], + "@import (reference) 'Magento_Module::some/file.css';\n" . + "@import (reference) 'Magento_Two::some/file.css';\n", + ], + 'modular reference one disabled module' => [ + '//@magento_import (reference) "Magento_Module::some/file.css";', + 'Magento_Module::some/file.css', + 'some/file.css', + [ + ['module' => 'Magento_Module','isEnabled' => true, 'filename' => 'some/file.css'], + ['module' => 'Magento_Two','isEnabled' => true, 'filename' => 'some/file.css'], + ['module' => 'Magento_Disabled','isEnabled' => false, 'filename' => 'some/file.css'], ], "@import (reference) 'Magento_Module::some/file.css';\n" . "@import (reference) 'Magento_Two::some/file.css';\n", ], + 'modular reference all modules disabled' => [ + '//@magento_import (reference) "Magento_Module::some/file.css";', + 'Magento_Module::some/file.css', + 'some/file.css', + [ + ['module' => 'Magento_Module','isEnabled' => false, 'filename' => 'some/file.css'], + ['module' => 'Magento_Two','isEnabled' => false, 'filename' => 'some/file.css'], + ['module' => 'Magento_Disabled','isEnabled' => false, 'filename' => 'some/file.css'], + ], + "", + ], ]; } From ee0e8cc8e8685149cdb4a63b2a548cb20a8d19aa Mon Sep 17 00:00:00 2001 From: Echron Date: Mon, 21 Oct 2019 11:53:41 +0200 Subject: [PATCH 4/6] Fix newsletter subscriber unittest failing because of new lines in newsletter --- .../testsuite/Magento/Newsletter/Model/SubscriberTest.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dev/tests/integration/testsuite/Magento/Newsletter/Model/SubscriberTest.php b/dev/tests/integration/testsuite/Magento/Newsletter/Model/SubscriberTest.php index e971ca88c4e57..fa914cdc72737 100644 --- a/dev/tests/integration/testsuite/Magento/Newsletter/Model/SubscriberTest.php +++ b/dev/tests/integration/testsuite/Magento/Newsletter/Model/SubscriberTest.php @@ -94,13 +94,17 @@ public function testConfirm() $this->model->loadByEmail($customerEmail); $this->model->confirm($this->model->getSubscriberConfirmCode()); + /** @var \Magento\TestFramework\Mail\Template\TransportBuilderMock $transportBuilder */ $transportBuilder = \Magento\TestFramework\Helper\Bootstrap::getObjectManager()->get( \Magento\TestFramework\Mail\Template\TransportBuilderMock::class ); + $output = $transportBuilder->getSentMessage()->getRawMessage(); + $output = \str_replace('\n\r','',$output); + $this->assertContains( 'You have been successfully subscribed to our newsletter.', - $transportBuilder->getSentMessage()->getRawMessage() + $output ); } } From 0bd04294447efce1967f0aba9aa5717526b3ec80 Mon Sep 17 00:00:00 2001 From: Echron Date: Mon, 21 Oct 2019 13:10:23 +0200 Subject: [PATCH 5/6] Improve code according to Magento2 coding standards (Fix newsletter subscriber unittest failing because of new lines in newsletter) --- .../testsuite/Magento/Newsletter/Model/SubscriberTest.php | 2 +- .../Test/Unit/PreProcessor/Instruction/MagentoImportTest.php | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/dev/tests/integration/testsuite/Magento/Newsletter/Model/SubscriberTest.php b/dev/tests/integration/testsuite/Magento/Newsletter/Model/SubscriberTest.php index fa914cdc72737..7b4d0e2e99358 100644 --- a/dev/tests/integration/testsuite/Magento/Newsletter/Model/SubscriberTest.php +++ b/dev/tests/integration/testsuite/Magento/Newsletter/Model/SubscriberTest.php @@ -100,7 +100,7 @@ public function testConfirm() ); $output = $transportBuilder->getSentMessage()->getRawMessage(); - $output = \str_replace('\n\r','',$output); + $output = \str_replace('=\r\n', '', $output); $this->assertContains( 'You have been successfully subscribed to our newsletter.', diff --git a/lib/internal/Magento/Framework/Css/Test/Unit/PreProcessor/Instruction/MagentoImportTest.php b/lib/internal/Magento/Framework/Css/Test/Unit/PreProcessor/Instruction/MagentoImportTest.php index 8c6fe4083a5b9..506a8cb694e6b 100644 --- a/lib/internal/Magento/Framework/Css/Test/Unit/PreProcessor/Instruction/MagentoImportTest.php +++ b/lib/internal/Magento/Framework/Css/Test/Unit/PreProcessor/Instruction/MagentoImportTest.php @@ -115,8 +115,7 @@ public function testProcess($originalContent, $foundPath, $resolvedPath, $foundF ->method('getFilename') ->will($this->returnValue($file['filename'])); - if(\is_null($file['module']) || (isset($file['isEnabled']) && $file['isEnabled'] === true)) - { + if (\is_null($file['module']) || (isset($file['isEnabled']) && $file['isEnabled'] === true)) { $files[] = $fileObject; } } From 52ca3d4a552ab822fba79bd77f275787c7aafbf9 Mon Sep 17 00:00:00 2001 From: Echron Date: Mon, 21 Oct 2019 15:39:44 +0200 Subject: [PATCH 6/6] Improve filtering out css files of disabled (or not available) modules + unittests --- .../File/Collector/Aggregated.php | 34 +++++++++++++++++-- .../Instruction/MagentoImport.php | 7 ++-- .../File/Collector/AggregatedTest.php | 18 ++++++++-- 3 files changed, 52 insertions(+), 7 deletions(-) diff --git a/lib/internal/Magento/Framework/Css/PreProcessor/File/Collector/Aggregated.php b/lib/internal/Magento/Framework/Css/PreProcessor/File/Collector/Aggregated.php index 2163ab9a1c5d4..d38c6e135d0a3 100644 --- a/lib/internal/Magento/Framework/Css/PreProcessor/File/Collector/Aggregated.php +++ b/lib/internal/Magento/Framework/Css/PreProcessor/File/Collector/Aggregated.php @@ -6,6 +6,7 @@ namespace Magento\Framework\Css\PreProcessor\File\Collector; +use Magento\Framework\Module\ModuleManagerInterface; use Magento\Framework\View\Design\ThemeInterface; use Magento\Framework\View\File\CollectorInterface; use Magento\Framework\View\File\FileList\Factory; @@ -36,6 +37,11 @@ class Aggregated implements CollectorInterface */ protected $overriddenBaseFiles; + /** + * @var \Magento\Framework\Module\ModuleManagerInterface + */ + protected $moduleManager; + /** * @var LoggerInterface */ @@ -46,6 +52,7 @@ class Aggregated implements CollectorInterface * @param CollectorInterface $libraryFiles * @param CollectorInterface $baseFiles * @param CollectorInterface $overriddenBaseFiles + * @param ModuleManagerInterface $moduleManager * @param LoggerInterface $logger */ public function __construct( @@ -53,12 +60,14 @@ public function __construct( CollectorInterface $libraryFiles, CollectorInterface $baseFiles, CollectorInterface $overriddenBaseFiles, + ModuleManagerInterface $moduleManager, LoggerInterface $logger ) { $this->fileListFactory = $fileListFactory; $this->libraryFiles = $libraryFiles; $this->baseFiles = $baseFiles; $this->overriddenBaseFiles = $overriddenBaseFiles; + $this->moduleManager = $moduleManager; $this->logger = $logger; } @@ -83,11 +92,32 @@ public function getFiles(ThemeInterface $theme, $filePath) $list->replace($files); } $result = $list->getAll(); - if (empty($result)) { + + $finalResult = []; + + if(!empty($result)) + { + foreach ($result as $file) + { + if ($this->hasEnabledModule($file)) { + $finalResult[] = $file; + } + } + } + + if (empty($finalResult)) { $this->logger->notice( 'magento_import returns empty result by path ' . $filePath . ' for theme ' . $theme->getCode() ); } - return $result; + + + return $finalResult; + } + + private function hasEnabledModule(\Magento\Framework\View\File $file) + { + return $file instanceof \Magento\Framework\View\File && (!$file->getModule() || + $this->moduleManager->isEnabled($file->getModule())); } } diff --git a/lib/internal/Magento/Framework/Css/PreProcessor/Instruction/MagentoImport.php b/lib/internal/Magento/Framework/Css/PreProcessor/Instruction/MagentoImport.php index 8321e137c71b1..8592d297cbafd 100644 --- a/lib/internal/Magento/Framework/Css/PreProcessor/Instruction/MagentoImport.php +++ b/lib/internal/Magento/Framework/Css/PreProcessor/Instruction/MagentoImport.php @@ -116,12 +116,13 @@ protected function replace(array $matchedContent, LocalInterface $asset) $isReference = !empty($matchedContent['reference']); $relatedAsset = $this->assetRepo->createRelated($matchedFileId, $asset); $resolvedPath = $relatedAsset->getFilePath(); + $importFiles = $this->fileSource->getFiles($this->getTheme($relatedAsset), $resolvedPath); /** @var $importFile \Magento\Framework\View\File */ foreach ($importFiles as $importFile) { - if ($importFile->getModule() && !$this->moduleManager->isEnabled($importFile->getModule())) { - continue; - } +// if ($importFile->getModule() && !$this->moduleManager->isEnabled($importFile->getModule())) { +// continue; +// } $referenceString = $isReference ? '(reference) ' : ''; $importsContent .= $importFile->getModule() ? "@import $referenceString'{$importFile->getModule()}::{$resolvedPath}';\n" diff --git a/lib/internal/Magento/Framework/Css/Test/Unit/PreProcessor/File/Collector/AggregatedTest.php b/lib/internal/Magento/Framework/Css/Test/Unit/PreProcessor/File/Collector/AggregatedTest.php index cdf7a1ec3734d..70d32a4221428 100644 --- a/lib/internal/Magento/Framework/Css/Test/Unit/PreProcessor/File/Collector/AggregatedTest.php +++ b/lib/internal/Magento/Framework/Css/Test/Unit/PreProcessor/File/Collector/AggregatedTest.php @@ -42,6 +42,11 @@ class AggregatedTest extends \PHPUnit\Framework\TestCase */ protected $themeMock; + /** + * @var \Magento\Framework\Module\ModuleManagerInterface + */ + protected $moduleManagerMock; + /** * @var \Psr\Log\LoggerInterface|\PHPUnit_Framework_MockObject_MockObject */ @@ -61,6 +66,8 @@ public function setup() ->will($this->returnValue($this->fileListMock)); $this->libraryFilesMock = $this->getMockBuilder(\Magento\Framework\View\File\CollectorInterface::class) ->getMock(); + $this->moduleManagerMock = $this->getMockBuilder(\Magento\Framework\Module\ModuleManagerInterface::class) + ->getMock(); $this->loggerMock = $this->getMockBuilder(\Psr\Log\LoggerInterface::class) ->getMock(); @@ -81,6 +88,7 @@ public function testGetFilesEmpty() $this->libraryFilesMock, $this->baseFilesMock, $this->overriddenBaseFilesMock, + $this->moduleManagerMock, $this->loggerMock ); @@ -106,9 +114,12 @@ public function testGetFilesEmpty() */ public function testGetFiles($libraryFiles, $baseFiles, $themeFiles) { + + $returnFile = new \Magento\Framework\View\File('returnFile','myModule'); + $this->fileListMock->expects($this->at(0))->method('add')->with($this->equalTo($libraryFiles)); $this->fileListMock->expects($this->at(1))->method('add')->with($this->equalTo($baseFiles)); - $this->fileListMock->expects($this->any())->method('getAll')->will($this->returnValue(['returnedFile'])); + $this->fileListMock->expects($this->any())->method('getAll')->will($this->returnValue([$returnFile])); $subPath = '*'; $this->libraryFilesMock->expects($this->atLeastOnce()) @@ -125,11 +136,14 @@ public function testGetFiles($libraryFiles, $baseFiles, $themeFiles) ->method('getFiles') ->will($this->returnValue($themeFiles)); + $this->moduleManagerMock->expects($this->any())->method('isEnabled')->will($this->returnValue(true)); + $aggregated = new Aggregated( $this->fileListFactoryMock, $this->libraryFilesMock, $this->baseFilesMock, $this->overriddenBaseFilesMock, + $this->moduleManagerMock, $this->loggerMock ); @@ -137,7 +151,7 @@ public function testGetFiles($libraryFiles, $baseFiles, $themeFiles) $this->themeMock->expects($this->any())->method('getInheritedThemes') ->will($this->returnValue([$inheritedThemeMock])); - $this->assertEquals(['returnedFile'], $aggregated->getFiles($this->themeMock, $subPath)); + $this->assertEquals([$returnFile], $aggregated->getFiles($this->themeMock, $subPath)); } /**