Theming: invalidate merged CSS asset, when any Imported stylesheets are modified (in dev mode)

[anonymous-matomo-user]

When Piwik is in development mode, all changes made to the stylesheets should immediately be visible in the browser. However, this is only true for the main stylesheet (theme.less). Any change made to stylesheets that are imported via @import into the main stylesheet are not visible until the main stylesheet is updated too.

> To fix this issue:

• the generateCacheBuster() method needs to be updated to retrieve the list of @imported files', and invalidate based on modified time or content.
• maybe we need to recursively go into the @import'ed files to look for others imported.

[mattab]

it was also reported in #4646

[timo-bes]

The documentation in the global config and some PHP sources still mentions that disable_merged_assets works for CSS files (search the codebase for disable_merged_assets to find the places). If this ticket is not fixed soon, at least the documentation should be adjusted because wrong documentation is quite confusing.

[timo-bes]

What does this mean "When Piwik is in development mode"? Do you mean disable_merged_assets=1 or is there another setting?

Couldn't find this in the docs. While searching, I found a typo here:

http://developer.piwik.org/guides/getting-started-part-1

"You're development environment is setup"

Should be "your development environment is set up"

[timo-bes]

I think the intention of Piwik\AssetManager\UIAssetMerger is that assets are always generated when disable_merged_assets=1.

In shouldGenerate(), the method shouldCompareExistingVersion() is called, which checks the disable_merged_assets flag. If disable_merged_assets=1, shouldCompareExistingVersion() returns false and I think, shouldGenerate() is meant to return true. However, the last line in the method is "return false". I guess this is not by intention, or is it?

If the last line in shouldGenerate() is changed to "return true", less stylesheets are always compiled when disable_merged_assets=1.

Therefore, I propose:

• Change the line to return true
• Update source documentation such that it says that disable_merged_assets=1 causes JS files to be included individually and less files to be compiled each time
• Keep the to do on the list, that UIAssetMerger could check whether the file or its includes have been modified before recompiling

This is the first time I get involved in the new way assets are handled, which is why I'm asking before committing the changes.

[timo-bes]

Having thought about this for another few minutes, I guess the intention of UIAssetMerger could also be: if disable_merged_assets=1, generate only if something has changed in the less files. In this case, the method shouldCompareExistingVersion() should not negate the result of isMergedAssetsDisabled().

The logic at the moment is: if disable_merged_assets=1, never generate - which clearly is not what we want, right? Unless I understand the code incorrectly, there is a bug. Please clarify the expected behavior. Either this comment or the previous one could be a first step towards the complete fix of this ticket.

[julienmoumne]

Replying to EZdesign:

The documentation in the global config and some PHP sources still mentions that disable_merged_assets works for CSS files (search the codebase for disable_merged_assets to find the places). If this ticket is not fixed soon, at least the documentation should be adjusted because wrong documentation is quite confusing.

Since Less has been introduced, it not possible to include CSS and Less files individually.
(It would actually be possible if we included a JS Less compiler.)

Replying to EZdesign:

What does this mean "When Piwik is in development mode"? Do you mean disable_merged_assets=1 or is there another setting?

Currently, Development Mode = disable_merged_assets=1
I would agree Piwik needs a dev mode flag.

Replying to EZdesign:

I think the intention of Piwik\AssetManager\UIAssetMerger is that assets are always generated when disable_merged_assets=1.

No.

When disable_merged_assets=1, the compiled less file will only be reprocessed if individual less files are modified.

In shouldGenerate(), the method shouldCompareExistingVersion() is called, which checks the disable_merged_assets flag. If disable_merged_assets=1, shouldCompareExistingVersion() returns false and I think, shouldGenerate() is meant to return true. However, the last line in the method is "return false". I guess this is not by intention, or is it?

The logic is a bit more involved :

In production mode (isMergedAssetsDisabled == False), CSS files and JS files are never modified by hand.

This means merged files never need to be compared (shouldCompareExistingVersion() == False).

The only times merged files in production mode need to be rebuilt are when plugins are activated/deactivated and when piwik is updated.

When this happens, the merged files are systematically deleted using AssetManager::removeMergedAssets();

When merged files are deleted, they are rebuilt, see UIAssetMerger::shouldGenerate() :

        if(!$this->mergedAsset->exists())
            return true;

In development mode and only in development mode (isMergedAssetsDisabled = true), individual less files can be manually updated.

In development mode, we therefore have to check whether the compiled less is up to date or not.

This is to avoid re-compiling the less files at every request while ensuring modifications in individual less files are taken into account.

Hence, shouldCompareExistingVersion :

return (isMergedAssetsDisabled == true);

Replying to EZdesign:

Having thought about this for another few minutes, I guess the intention of UIAssetMerger could also be: if disable_merged_assets=1, generate only if something has changed in the less files. In this case, the method shouldCompareExistingVersion() should not negate the result of isMergedAssetsDisabled().

There has been a regression in [https://github.com/piwik/piwik/commit/ca4779fa8582a7031a149a2c113a322b2c2f2090]

---

Now back to the issue described in the description.

The problem with @imports is that they are not explicitly declared by plugins using hooks.

The way the compiled Less file is checked for updates is by comparing the md5 value of the concatenation of declared less/css files.

To fix this issue, the generateCacheBuster() method needs to be updated to retrieve the @imported files' content and add them to the declared concatenated assets.

[tsteur]

In 5930f3d: refs #4523 reverting some changes of ca4779f as discussed with Julien. Note: CSS is in this case not regenerated in case of any plugin updates/downgrades or manuall uninstall

[timo-bes]

Thanks for the comments, Julien.

With the fix, the intended logic is (if I understand it correctly): the css is regenerated when disable_merged_assets=1 and an included less file has changed.

Shouldn't we update the documentation in the source code to describe the logic this way?

[julienmoumne]

Replying to Thomas Steur:

In 5930f3d: >> refs #4523 reverting some changes of ca4779f as discussed with Julien.

Thanks Thomas.

Note: CSS is in this case not regenerated in case of any plugin updates/downgrades or manuall uninstall

Not entirely true see comment:1:ticket:4655

Replying to EZdesign:

With the fix, the intended logic is (if I understand it correctly): the css is regenerated when disable_merged_assets=1 and an included less file has changed.

Correct

[timo-bes]

In 376d677: refs #4523 updating documentation for disable_merged_assets

[timo-bes]

Maybe I'm mistaken, but wouldn't it suffice to use the modification timestamp of the latest less file contained in the merged CSS as a cache buster? When one of files changes, its timestamp will be used, which will differ from the previous latest modification date (i.e., the cache buster).

This would be easier to implement and also faster because we don't have to load the contents of all less files in order to hash it.

Alternatively, we could compare the timestamp of the generated CSS to the latest modification date of a less file. If a less file is newer, the CSS has to be generated again. This is pretty much the same mechanism without using the cache buster stuff.

[julienmoumne]

Replying to EZdesign:

Maybe I'm mistaken, but wouldn't it suffice to use the modification timestamp of the latest less file contained in the merged CSS as a cache buster? When one of files changes, its timestamp will be used, which will differ from the previous latest modification date (i.e., the cache buster).

This would be easier to implement and also faster because we don't have to load the contents of all less files in order to hash it.

Alternatively, we could compare the timestamp of the generated CSS to the latest modification date of a less file. If a less file is newer, the CSS has to be generated again. This is pretty much the same mechanism without using the cache buster stuff.

I think the content of the files is being used because using the modification date is error prone.

Nevertheless, this does not bring a solution to the issue described in the ticket.

The problem with @imports is that they are not explicitly declared by plugins using hooks.

To fix this issue, the generateCacheBuster() method needs to be updated to retrieve the list of @imported files'.

[timo-bes]

@julien: Using the latest timestamp of a contained file would solve the issue, if @imports were parsed and considered as well.

What do you mean using the modification date is error prone? Which problems can occur?

[julienmoumne]

Replying to EZdesign:

@julien: Using the latest timestamp of a contained file would solve the issue, if @imports were parsed and considered as well.

What do you mean using the modification date is error prone? Which problems can occur?

I don'thave concrete evidences modification dates are unreliable.

It's really just a feeling.

Feel free to discard it.

[mattab]

Cheers for sharing your thoughts.
I tried summarized the discussion in the ticket description:

#> To fix this issue:

• the generateCacheBuster() method needs to be updated to retrieve the list of @imported files', and invalidate based on modified time or content.
• maybe we need to recursively go into the @import'ed files to look for others imported.

[julienmoumne]

The less compiler we currently use appear to have caching features we may leverage :http://leafo.net/lessphp/docs/#compiling

The compileChecked method is like compileFile, but it only compiles if the output file
doesnt exist or its older than the input file

[tsteur]

In 454825c: refs #4523 whenever a css or less file changes, delete compiled css. I put this as a "workaround" in here as it solves the problem but maybe we wanna implement a solution in PHP though? If not, we could put this in another place maybe or leave it under tests. To make use of this just execute "cd tests/angularjs && grunt" after installing all the packages (see README). grunt will be notified whenever a file changes and immediately remove the compiled css