Fix issue 24666 (Static content is deploying for disabled modules)

[Echron]

Make the output of css file dependent on the status (enabled/disabled) of a module

Description (*)

Fixed logic for css preprocessor to check if module is enabled before including its less / css resources to the generated static styles.

Fixed Issues (if relevant)

1. Static content is deploying for disabled modules #24666: Static content is deploying for disabled modules

Improving unittest of PR #24772

[m2-assistant]

Hi @Echron. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[hostep]

Just to have some status here, we looked at this issue during the contribution day of #mleu with @Echron and @okorshenko and it turns out that the solution is probably a lot deeper in that the agregated file list for css/less files isn't getting filtered based on if a module is enabled or disabled, and even if the module is not installed at all (Magento Commerce modules styling exist in the Blank theme, while the modules themselves are not available in Open Source and even then the generated css code contains styles from those Commerce modules).

After some playing around, it looks like this might be a better fix:

diff --git a/app/code/Magento/Developer/etc/di.xml b/app/code/Magento/Developer/etc/di.xml
index 98adcbb3a82..43082746c53 100644
--- a/app/code/Magento/Developer/etc/di.xml
+++ b/app/code/Magento/Developer/etc/di.xml
@@ -204,7 +204,7 @@
         <arguments>
             <argument name="libraryFiles" xsi:type="object">Magento\Framework\Css\PreProcessor\File\Collector\Library</argument>
             <argument name="baseFiles" xsi:type="object">cssSourceBaseFilesSorted</argument>
-            <argument name="overriddenBaseFiles" xsi:type="object">cssSourceOverriddenBaseFiles</argument>
+            <argument name="overriddenBaseFiles" xsi:type="object">cssSourceOverriddenBaseFilesSorted</argument>
         </arguments>
     </type>

@@ -224,6 +224,16 @@
         </arguments>
     </virtualType>

+    <virtualType name="cssSourceOverriddenBaseFilesSorted" type="Magento\Framework\View\File\Collector\Decorator\ModuleDependency">
+        <arguments>
+            <argument name="subject" xsi:type="object">cssSourceOverriddenBaseFilesFiltered</argument>
+        </arguments>
+    </virtualType>
+    <virtualType name="cssSourceOverriddenBaseFilesFiltered" type="Magento\Framework\View\File\Collector\Decorator\ModuleOutput">
+        <arguments>
+            <argument name="subject" xsi:type="object">cssSourceOverriddenBaseFiles</argument>
+        </arguments>
+    </virtualType>
     <virtualType name="cssSourceOverriddenBaseFiles" type="Magento\Framework\View\File\Collector\Override\Base">
         <arguments>
             <argument name="subDir" xsi:type="string">web</argument>

This shaves off about ~60KB of generated css on a vanilla Magento Open Source installation.

[ihor-sviziev]

@

Just to have some status here, we looked at this issue during the contribution day of #mleu with @Echron and @okorshenko and it turns out that the solution is probably a lot deeper in that the agregated file list for css/less files isn't getting filtered based on if a module is enabled or disabled, and even if the module is not installed at all (Magento Commerce modules styling exist in the Blank theme, while the modules themselves are not available in Open Source and even then the generated css code contains styles from those Commerce modules).

After some playing around, it looks like this might be a better fix:

diff --git a/app/code/Magento/Developer/etc/di.xml b/app/code/Magento/Developer/etc/di.xml
index 98adcbb3a82..43082746c53 100644
--- a/app/code/Magento/Developer/etc/di.xml
+++ b/app/code/Magento/Developer/etc/di.xml
@@ -204,7 +204,7 @@
         <arguments>
             <argument name="libraryFiles" xsi:type="object">Magento\Framework\Css\PreProcessor\File\Collector\Library</argument>
             <argument name="baseFiles" xsi:type="object">cssSourceBaseFilesSorted</argument>
-            <argument name="overriddenBaseFiles" xsi:type="object">cssSourceOverriddenBaseFiles</argument>
+            <argument name="overriddenBaseFiles" xsi:type="object">cssSourceOverriddenBaseFilesSorted</argument>
         </arguments>
     </type>

@@ -224,6 +224,16 @@
         </arguments>
     </virtualType>

+    <virtualType name="cssSourceOverriddenBaseFilesSorted" type="Magento\Framework\View\File\Collector\Decorator\ModuleDependency">
+        <arguments>
+            <argument name="subject" xsi:type="object">cssSourceOverriddenBaseFilesFiltered</argument>
+        </arguments>
+    </virtualType>
+    <virtualType name="cssSourceOverriddenBaseFilesFiltered" type="Magento\Framework\View\File\Collector\Decorator\ModuleOutput">
+        <arguments>
+            <argument name="subject" xsi:type="object">cssSourceOverriddenBaseFiles</argument>
+        </arguments>
+    </virtualType>
     <virtualType name="cssSourceOverriddenBaseFiles" type="Magento\Framework\View\File\Collector\Override\Base">
         <arguments>
             <argument name="subDir" xsi:type="string">web</argument>

This shaves off about ~60KB of generated css on a vanilla Magento Open Source installation.

For sure it will be more "understandable", but need to compare performance of these two options, looks like from performance perspective changes from this PR is better

[hostep]

Hi @ihor-sviziev, according to the discussion with @okorshenko at #mleu, the preferred solution was to use decorators as a solution as that's fixing the root problem.

Out of curiosity: how significant are the performance differences?

[DrewML]

(Drive by comment, sorry!)

Can't remember if the JS/Grunt setup piggybacks on the PHP code's logic for collecting files. If not, we might need to fix that problem on the JS side too, to prevent making this problem worse

[hostep]

Good point @DrewML

But it looks like we are safe. Running grunt exec triggers: php bin/magento dev:source-theme:deploy ... which is affected by the changes proposed in this PR.

$ grunt exec --verbose
Initializing
Command-line options: --verbose, --gruntfile=Gruntfile.js

Reading "Gruntfile.js" Gruntfile...OK

Registering Gruntfile tasks.
Initializing config...OK
Loading "Gruntfile.js" tasks...OK
+ black-list-generator, clean-black-list, default, deploy, documentation, legacy-build, mage-minify, prod, refresh, spec, static

Running tasks: exec

Loading "grunt-exec" plugin

Registering "node_modules/grunt-exec/tasks" tasks.
Loading "exec.js" tasks...OK
+ exec

Running "exec" task

Running "exec:blank" (exec) task
Verifying property exec.blank exists in config...OK
File: [no files]

grunt --force clean:blank && php bin/magento dev:source-theme:deploy css/styles-m css/styles-l css/email css/email-inline --type=less --locale=en_US --area=frontend --theme=Magento/blank
buffer   : disabled
timeout  : infinite
killSig  : SIGTERM
shell    : true
command  : grunt --force clean:blank && php bin/magento dev:source-theme:deploy css/styles-m css/styles-l css/email css/email-inline --type=less --locale=en_US --area=frontend --theme=Magento/blank
args     : []
stdio    : [ignore,pipe,pipe]
cwd      : .
exitcodes: 0
pid     : 27914
Running "clean:blank" (clean) task
>> 268 paths cleaned.

Done.

[DrewML]

Thanks for verifying @hostep!

[hostep]

@ihor-sviziev: I'm still planning on implementing the alternative solution one day (if I find the time), since it was discussed to be a better solution.

[ihor-sviziev]

@magento run all tests

[ihor-sviziev]

@hostep sure, no problem

[mrtuvn]

hi guys any update on this PR

[ihor-sviziev]

Hi,
Unfortunately still not have enough time to compare two versions using blackfire profiler just to compare which option will be better. I think I’ll have time for it next week

[mrtuvn]

Hi can i keep continue this PR ? Seem quite long inactivity no response from @Echron

[ihor-sviziev]

@mrtuvn yes, please update PR

[ihor-sviziev]

Just looked on the #27888 that is based on @hostep idea - it's not breaking any tests and works fine. I decided to accept it instead of this PR.
I'm closing this PR.

[m2-assistant]

Hi @Echron, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.