#27270 Further improvements to @krzkrz PR with changes from @sshymko, my change related to reattaching the plugin only to Layout and Pages (ancestor of Layout)

lbajsarowicz · lbajsarowicz · commit e38b1a116daf · 2020-04-28T13:30:24.000+02:00
diff --git a/app/code/Magento/Theme/Controller/Result/JsFooterPlugin.php b/app/code/Magento/Theme/Controller/Result/JsFooterPlugin.php
@@ -8,8 +8,10 @@
 namespace Magento\Theme\Controller\Result;
 
 use Magento\Framework\App\Config\ScopeConfigInterface;
+use Magento\Framework\App\Response\HttpInterface as HttpResponseInterface;
+use Magento\Framework\App\ResponseInterface;
+use Magento\Framework\View\Result\Layout;
 use Magento\Store\Model\ScopeInterface;
-use Magento\Framework\App\Response\Http;
 
 /**
  * Plugin for putting all JavaScript tags to the end of body.
@@ -34,26 +36,32 @@ public function __construct(ScopeConfigInterface $scopeConfig)
     /**
      * Moves all JavaScript tags to the end of body if this feature is enabled.
      *
-     * @param Http $subject
-     * @return void
+     * @param Layout $subject
+     * @param Layout $result
+     * @param HttpResponseInterface|ResponseInterface $httpResponse
+     * @return Layout (That should be void, actually)
+     * @SuppressWarnings(PHPMD.UnusedFormalParameter)
      */
-    public function beforeSendResponse(Http $subject)
+    public function afterRenderResult(Layout $subject, Layout $result, ResponseInterface $httpResponse)
     {
-        $content = (string)$subject->getContent();
+        if (!$this->isDeferEnabled()) {
+            return $result;
+        }
+
+        $content = (string)$httpResponse->getContent();
         $bodyEndTag = '</body';
-        $isEndBodyTagFound = strpos($content, $bodyEndTag) !== false;
-        $shouldMoveJsToBottom = $this->scopeConfig->isSetFlag(
-            self::XML_PATH_DEV_MOVE_JS_TO_BOTTOM,
-            ScopeInterface::SCOPE_STORE
-        );
+        $bodyEndTagFound = strrpos($content, $bodyEndTag) !== false;
 
-        if ($isEndBodyTagFound && $shouldMoveJsToBottom) {
+        if ($bodyEndTagFound) {
             $scripts = $this->extractScriptTags($content);
             if ($scripts) {
-                $content = str_replace($bodyEndTag, "$scripts\n$bodyEndTag", $content);
-                $subject->setContent($content);
+                $newBodyEndTagPosition = strrpos($content, $bodyEndTag);
+                $content = substr_replace($content, $scripts . "\n", $newBodyEndTagPosition, 0);
+                $httpResponse->setContent($content);
             }
         }
+
+        return $result;
     }
 
     /**
@@ -86,4 +94,17 @@ private function extractScriptTags(&$content): string
 
         return $scripts;
     }
+
+    /**
+     * Returns information whether moving JS to footer is enabled
+     *
+     * @return bool
+     */
+    private function isDeferEnabled(): bool
+    {
+        return $this->scopeConfig->isSetFlag(
+            self::XML_PATH_DEV_MOVE_JS_TO_BOTTOM,
+            ScopeInterface::SCOPE_STORE
+        );
+    }
 }
diff --git a/app/code/Magento/Theme/Test/Unit/Controller/Result/JsFooterPluginTest.php b/app/code/Magento/Theme/Test/Unit/Controller/Result/JsFooterPluginTest.php
@@ -7,13 +7,14 @@
 
 namespace Magento\Theme\Test\Unit\Controller\Result;
 
-use Magento\Theme\Controller\Result\JsFooterPlugin;
-use Magento\Framework\App\Response\Http;
-use PHPUnit\Framework\TestCase;
-use PHPUnit\Framework\MockObject\MockObject;
 use Magento\Framework\App\Config\ScopeConfigInterface;
-use Magento\Store\Model\ScopeInterface;
+use Magento\Framework\App\Response\Http;
 use Magento\Framework\TestFramework\Unit\Helper\ObjectManager as ObjectManagerHelper;
+use Magento\Framework\View\Result\Layout;
+use Magento\Store\Model\ScopeInterface;
+use Magento\Theme\Controller\Result\JsFooterPlugin;
+use PHPUnit\Framework\MockObject\MockObject;
+use PHPUnit\Framework\TestCase;
 
 /**
  * Unit test for Magento\Theme\Test\Unit\Controller\Result\JsFooterPlugin.
@@ -22,21 +23,18 @@ class JsFooterPluginTest extends TestCase
 {
     const STUB_XML_PATH_DEV_MOVE_JS_TO_BOTTOM = 'dev/js/move_script_to_bottom';
 
-    /**
-     * @var JsFooterPlugin
-     */
+    /** @var JsFooterPlugin */
     private $plugin;
 
-    /**
-     * @var ScopeConfigInterface|MockObject
-     */
+    /** @var ScopeConfigInterface|MockObject */
     private $scopeConfigMock;
 
-    /**
-     * @var Http|MockObject
-     */
+    /** @var Http|MockObject */
     private $httpMock;
 
+    /** @var Layout|MockObject */
+    private $layoutMock;
+
     /**
      * @inheritdoc
      */
@@ -48,6 +46,7 @@ protected function setUp(): void
             ->getMockForAbstractClass();
 
         $this->httpMock = $this->createMock(Http::class);
+        $this->layoutMock = $this->createMock(Layout::class);
 
         $objectManager = new ObjectManagerHelper($this);
         $this->plugin = $objectManager->getObject(
@@ -59,11 +58,11 @@ protected function setUp(): void
     }
 
     /**
-     * Data Provider for testBeforeSendResponse()
+     * Data Provider for testAfterRenderResult()
      *
      * @return array
      */
-    public function sendResponseDataProvider(): array
+    public function renderResultDataProvider(): array
     {
         return [
             'content_with_script_tag' => [
@@ -98,31 +97,29 @@ public function sendResponseDataProvider(): array
      * @param bool $isSetFlag
      * @param string $result
      * @return void
-     * @dataProvider sendResponseDataProvider
+     * @dataProvider renderResultDataProvider
      */
-    public function testBeforeSendResponse($content, $isSetFlag, $result): void
+    public function testAfterRenderResult($content, $isSetFlag, $result): void
     {
-        $this->httpMock->expects($this->once())
-            ->method('getContent')
+        // Given (context)
+        $this->httpMock->method('getContent')
             ->willReturn($content);
 
-        $this->scopeConfigMock->expects($this->once())
-            ->method('isSetFlag')
-            ->with(
-                self::STUB_XML_PATH_DEV_MOVE_JS_TO_BOTTOM,
-                ScopeInterface::SCOPE_STORE
-            )
+        $this->scopeConfigMock->method('isSetFlag')
+            ->with(self::STUB_XML_PATH_DEV_MOVE_JS_TO_BOTTOM, ScopeInterface::SCOPE_STORE)
             ->willReturn($isSetFlag);
 
+        // Expects
         $this->httpMock->expects($this->any())
             ->method('setContent')
             ->with($result);
 
-        $this->plugin->beforeSendResponse($this->httpMock);
+        // When
+        $this->plugin->afterRenderResult($this->layoutMock, $this->layoutMock, $this->httpMock);
     }
 
     /**
-     * Data Provider for testBeforeSendResponseIfGetContentIsNotAString()
+     * Data Provider for testAfterRenderResultIfGetContentIsNotAString()
      *
      * @return array
      */
@@ -136,21 +133,25 @@ public function ifGetContentIsNotAStringDataProvider(): array
     }
 
     /**
-     * Test BeforeSendResponse if content is not a string
+     * Test AfterRenderResult if content is not a string
      *
      * @param string $content
      * @return void
      * @dataProvider ifGetContentIsNotAStringDataProvider
      */
-    public function testBeforeSendResponseIfGetContentIsNotAString($content): void
+    public function testAfterRenderResultIfGetContentIsNotAString($content): void
     {
+        $this->scopeConfigMock->method('isSetFlag')
+            ->with(self::STUB_XML_PATH_DEV_MOVE_JS_TO_BOTTOM, ScopeInterface::SCOPE_STORE)
+            ->willReturn(true);
+
         $this->httpMock->expects($this->once())
             ->method('getContent')
             ->willReturn($content);
 
         $this->httpMock->expects($this->never())
             ->method('setContent');
 
-        $this->plugin->beforeSendResponse($this->httpMock);
+        $this->plugin->afterRenderResult($this->layoutMock, $this->layoutMock, $this->httpMock);
     }
 }
diff --git a/app/code/Magento/Theme/etc/frontend/di.xml b/app/code/Magento/Theme/etc/frontend/di.xml
@@ -27,9 +27,11 @@
         <plugin name="result-messages" type="Magento\Theme\Controller\Result\MessagePlugin"/>
     </type>
     <type name="Magento\Framework\App\Response\Http">
-        <plugin name="result-js-footer" type="Magento\Theme\Controller\Result\JsFooterPlugin"/>
         <plugin name="asyncCssLoad" type="Magento\Theme\Controller\Result\AsyncCssPlugin"/>
     </type>
+    <type name="Magento\Framework\View\Result\Layout">
+        <plugin name="result-js-footer" type="Magento\Theme\Controller\Result\JsFooterPlugin"/>
+    </type>
     <type name="Magento\Theme\Block\Html\Header\CriticalCss">
         <arguments>
             <argument name="filePath" xsi:type="string">css/critical.css</argument>
