Skip to content

Cache final result of update check - #7562

Merged
rullzer merged 1 commit into
masterfrom
fix-wrongly-cached-result
Dec 19, 2017
Merged

Cache final result of update check#7562
rullzer merged 1 commit into
masterfrom
fix-wrongly-cached-result

Conversation

@MorrisJobke

Copy link
Copy Markdown
Member

If the parsed data is not a valid response we should not cache it and only cache the preprocessed result set.

Fixes#7442

This method is called in

$data = $this->updater->check();

If the $data instead of $tmp is cached, then the returned value in the first call and the second call could be different, because a different object is cached causing the array access error in #7442.

I also added a unit test to verify that a missing attribute still results in a proper result with all keys.

If the parsed data is not a valid response we should not cache it and only cache the preprocessed result set.
Fixes#7442
Signed-off-by: Morris Jobke <hey@morrisjobke.de>

@rullzerrullzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense

@codecov

codecovBot commented Dec 18, 2017

Copy link
Copy Markdown

Codecov Report

Merging #7562 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@ Coverage Diff @@## master #7562 +/- ##
============================================
- Coverage 51.18% 51.17% -0.01% 
Complexity 24874 24874 ============================================
Files 1601 1601 Lines 94701 94700 -1 Branches 1368 1368 ============================================
- Hits 48474 48467 -7 - Misses 46227 46233 +6
Impacted FilesCoverage ΔComplexity Δ
lib/private/Updater/VersionCheck.php92.1% <100%> (-0.21%)7 <0> (ø)
core/js/js.js62.98% <0%> (-0.57%)0% <0%> (ø)
apps/files_trashbin/lib/Trashbin.php72.28% <0%> (-0.25%)136% <0%> (ø)

@rullzer
rullzer merged commit e550a3d into masterDec 19, 2017
@rullzer
rullzer deleted the fix-wrongly-cached-result branch December 19, 2017 09:12
@MorrisJobkeMorrisJobke mentioned this pull request Jan 2, 2018
30 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewWaiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@MorrisJobke@rullzer@icewind1991