Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
anonymous-matomo-user opened this issue Jan 14, 2014 · 17 comments
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.

Comments

@anonymous-matomo-user
Copy link

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
Copy link
Member

mattab commented Feb 8, 2014

it was also reported in #4646

@timo-bes
Copy link
Member

timo-bes commented Feb 9, 2014

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
Copy link
Member

timo-bes commented Feb 9, 2014

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
Copy link
Member

timo-bes commented Feb 9, 2014

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
Copy link
Member

timo-bes commented Feb 9, 2014

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
Copy link
Member

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
Copy link
Member

tsteur commented Feb 10, 2014

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

In 376d677: refs #4523 updating documentation for disable_merged_assets

@timo-bes
Copy link
Member

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

mattab commented Feb 11, 2014

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
Copy link
Member

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
Copy link
Member

tsteur commented Apr 13, 2014

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

@anonymous-matomo-user anonymous-matomo-user added this to the 2.x - The Great Piwik 2.x Backlog milestone Jul 8, 2014
sabl0r pushed a commit to sabl0r/piwik that referenced this issue Sep 23, 2014
…ith Julien. Note: CSS is in this case not regenerated in case of any plugin updates/downgrades or manuall uninstall
@mattab mattab modified the milestones: Long term, Mid term Dec 5, 2016
@mattab mattab removed this from the Long term milestone Dec 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Projects
None yet
Development

No branches or pull requests

5 participants