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

Repair step to clear frontend related caches #8144

Merged
merged 4 commits into from
Feb 5, 2018
Merged

Conversation

juliushaertl
Copy link
Member

This PR adds a repair step that will be executed during update and when calling occ maintenance:repair, that will remove any frontend related caches that caused issues in the past, as:

  • SCSS cache
  • combined JS files
  • imagePath cache

I also added some further logging to the SCSSCacher and the JSCombiner, so at least logs can tell if something was wrong there.

The initial idea was from #7984 to pregenerate files, but that would be a lot more logic to be implemented, since the list of js/css files is built dynamically on every request. Clearing those files from appdata should be enough there.

@codecov
Copy link

codecov bot commented Feb 1, 2018

Codecov Report

Merging #8144 into master will decrease coverage by 16.66%.
The diff coverage is 4.76%.

@@              Coverage Diff              @@
##             master    #8144       +/-   ##
=============================================
- Coverage     51.73%   35.07%   -16.67%     
- Complexity    25357    25362        +5     
=============================================
  Files          1598     1599        +1     
  Lines         95021    95057       +36     
  Branches       1376     1376               
=============================================
- Hits          49160    33339    -15821     
- Misses        45861    61718    +15857
Impacted Files Coverage Δ Complexity Δ
lib/private/Template/SCSSCacher.php 5.83% <0%> (-63.73%) 36 <3> (-3)
lib/private/Repair/ClearFrontendCaches.php 0% <0%> (ø) 4 <4> (?)
lib/private/Template/JSCombiner.php 0% <0%> (-88.18%) 30 <3> (+3)
lib/private/Repair.php 0% <0%> (-31.82%) 19 <0> (ø)
lib/private/Server.php 83.18% <22.22%> (-0.55%) 282 <1> (+1)
apps/dav/lib/CalDAV/Activity/Setting/Calendar.php 0% <0%> (-100%) 8% <0%> (ø)
apps/federation/lib/Hooks.php 0% <0%> (-100%) 4% <0%> (ø)
...ty/Exceptions/CrossSiteRequestForgeryException.php 0% <0%> (-100%) 1% <0%> (ø)
lib/public/RichObjectStrings/Definitions.php 0% <0%> (-100%) 2% <0%> (ø)
apps/dav/lib/CalDAV/Activity/Filter/Calendar.php 0% <0%> (-100%) 7% <0%> (ø)
... and 521 more

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code makes sense! 👍

rullzer
rullzer previously requested changes Feb 2, 2018
$output->info('Image cache cleared');

$this->scssCacher->resetCache();
$c = $this->cacheFactory->createDistributed('SCSS');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make more sense to do this in the scssCacher? I would expect if I call the reset cache function that also the Memorycache is cleared.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes. Just moved that over and added tests for those methods as well.

$output->info('SCSS cache cleared');

$this->jsCombiner->resetCache();
$c = $this->cacheFactory->createDistributed('JS');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

public function resetCache() {
$this->depsCache->clear();
$appDirectory = $this->appData->getDirectoryListing();
if(empty($appDirectory)){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really needed if you just loop over the array in the next step. But doesn't hurt.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, removed them as well.

@rullzer
Copy link
Member

rullzer commented Feb 3, 2018

Can you rebase upon master? Now that my fix is in. I just want green CI to be sure.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
Copy link
Member Author

Rebased.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@MorrisJobke
Copy link
Member

Failure is unrelated.

@MorrisJobke MorrisJobke merged commit 847bd0c into master Feb 5, 2018
@MorrisJobke MorrisJobke deleted the cache-update-occ branch February 5, 2018 11:15
@skjnldsv skjnldsv added the feature: caching Related to our caching system: scssCacher, jsCombiner... label May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug feature: caching Related to our caching system: scssCacher, jsCombiner... feature: scss
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants