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

Invalidate merged Noncore JS / CSS in case of any plugin update/downgrade/uninstall #4655

Closed
tsteur opened this issue Feb 10, 2014 · 17 comments
Assignees
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Milestone

Comments

@tsteur
Copy link
Member

tsteur commented Feb 10, 2014

In case we are in production mode (merged assets is enabled) and there is a plugin upgrade/downgrade/uninstall we should regenerate the merged Noncore JS / CSS if needed.

@julienmoumne
Copy link
Member

This should already work, see first line of [https://github.com/piwik/piwik/blob/master/core/Filesystem.php#L24] :

/**
* Called on Core install, update, plugin enable/disable
* Will clear all cache that could be affected by the change in configuration being made
*/
    public static function deleteAllCacheOnUpdate($pluginName = false)
    {
        AssetManager::getInstance()->removeMergedAssets($pluginName);
        View::clearCompiledTemplates();
        Cache::deleteTrackerCache();
    }

@tsteur
Copy link
Member Author

tsteur commented Feb 10, 2014

During update/downgrade/... it is not called I think. At least this is what I've experienced. I will try to reproduce it later. We should definitely remove the first line of that comment as a function does not know when it is called :)

@tsteur
Copy link
Member Author

tsteur commented Feb 12, 2014

To reproduce I installed the CustomAlerts plugin. Afterwards I increased the version number form 0.1.12 to 0.1.13 in plugin.json and added the following CSS to the alerts.less file

body, html {
  background-color: red;
}

In database Piwik recognized the update but did not recompile the CSS. Same for downgrading.

@julienmoumne
Copy link
Member

If you are manually editing files, you are then in "development mode" right?

disable_merged_assets should then be set to true

@tsteur
Copy link
Member Author

tsteur commented Feb 12, 2014

That's what I would normally do. But to reproduce this behavior on production I disabled merged assets (=false).

@julienmoumne
Copy link
Member

Why would you edit files in a production environment?

@tsteur
Copy link
Member Author

tsteur commented Feb 12, 2014

This is basically what happens when you update a 3rd party plugin in production environment

@julienmoumne
Copy link
Member

Is there no button for the user to press in order to trigger the update?

@julienmoumne
Copy link
Member

By 3rd party you mean a plugin not published in the marketplace right?

In that case, maybe the simple solution is to document the fact the tmp directory must be manually emptied when a plugin is manually updated.

@tsteur
Copy link
Member Author

tsteur commented Feb 12, 2014

We experienced this issue also when updating plugins via Marketplace or when uploading a plugin ZIP in the UI. When you are running multiple PHP nodes you usually want to update manually though. Documenting could be a solution but can be problematic since you maybe do not directly notice that you forgot to clean the tmp directory (which also causes a rebuild of all JS files). In our case we didn't notice it in the beginning since everything still kinda worked but didn't. Maybe we can delete the assets in case any plugin changes? We do already have the installed version in the option table and can compare it with the current plugin version in the plugin.json file before it is actually updated.

@tsteur
Copy link
Member Author

tsteur commented Feb 17, 2014

In 237d8ef: fixes #4655 assets are not invalidated after a plugin change. To fix this issue (at least for now) we always compare the cache buster. For JavaScript in production this is always fast as the hash is based only on plugin names + version. For css we have to concat the content of all raw css/less files which should be still fast, especially since the css is cached in the browser afterwards. @julien let me know what you think about this and we can change it otherwise

@tsteur
Copy link
Member Author

tsteur commented Feb 17, 2014

In 3da7d94: refs #4655 updated description of merged assets setting

@timo-bes
Copy link
Member

The setting disable_merged_assets is mentioned multiple times in the code as well. I'd suggest updating all descriptions. Just do a quick search for disable_merged_assets to see what I mean: at a first glance, StylesheetUIAssetFetcher and AssetManager need updates as well.

Or maybe it would be better to remove the duplicate documentation and refer to the comment in global.ini from the other places.

@tsteur
Copy link
Member Author

tsteur commented Feb 17, 2014

In 2a00eb6: refs #4655 updated documentation as the merged_assets flag is not really relevant for CSS at the moment

@tsteur
Copy link
Member Author

tsteur commented Feb 17, 2014

In b150893: refs #4655 updated description of merged assets option

@tsteur
Copy link
Member Author

tsteur commented Feb 17, 2014

In 5c12352: refs #4655 actually here we can refer to the comment in the config

@tsteur
Copy link
Member Author

tsteur commented Feb 17, 2014

Thx Timo! I removed it in one place and updated another. Probably we still need the documentation in two places. The inline doc for developers (developer.piwik.org) and the other one for people who want to configure Piwik and just browse through the config.

@tsteur tsteur added this to the 2.1 - Piwik 2.1 milestone Jul 8, 2014
@tsteur tsteur self-assigned this Jul 8, 2014
@tsteur tsteur modified the milestones: 2.4.0 - Piwik 2.4.0, 2.1 - Piwik 2.1 Jul 11, 2014
sabl0r pushed a commit to sabl0r/piwik that referenced this issue Sep 23, 2014
…e. To fix this issue (at least for now) we always compare the cache buster. For JavaScript in production this is always fast as the hash is based only on plugin names + version. For css we have to concat the content of all raw css/less files which should be still fast, especially since the css is cached in the browser afterwards. @julien let me know what you think about this and we can change it otherwise
sabl0r pushed a commit to sabl0r/piwik that referenced this issue Sep 23, 2014
sabl0r pushed a commit to sabl0r/piwik that referenced this issue Sep 23, 2014
…is not really relevant for CSS at the moment
sabl0r pushed a commit to sabl0r/piwik that referenced this issue Sep 23, 2014
sabl0r pushed a commit to sabl0r/piwik that referenced this issue Sep 23, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Projects
None yet
Development

No branches or pull requests

3 participants