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

fix(theming): Don't reset the cachebuster value when we reset theming #46428

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

SystemKeeper
Copy link
Contributor

@SystemKeeper SystemKeeper commented Jul 10, 2024

Summary

From the other PR:

By default there is a Pragma: no-cache header set due to the default
value no-cache of session.cache-limiter, which will cause Chrome and
iOS to not cache even with a different Cache-Control header set on the
response.

After the PR was merged the cypress-tests theming/AdminSettings kept failing. Since we now don't have a pragma: no-cache header anymore, we now rely on cache-control header and our cachebuster implementation to make sure that we get the correct files. For some files, we rely on a cachebuster based on appVersion and cachebuster value:

protected function getVersionHashSuffix($path = false, $file = false) {
if ($this->config->getSystemValueBool('debug', false)) {
// allows chrome workspace mapping in debug mode
return "";
}
$themingSuffix = '';
$v = [];
if ($this->config->getSystemValueBool('installed', false)) {
if (\OC::$server->getAppManager()->isInstalled('theming')) {
$themingSuffix = '-' . $this->config->getAppValue('theming', 'cachebuster', '0');
}
$v = \OC_App::getAppVersions();
}
// Try the webroot path for a match
if ($path !== false && $path !== '') {
$appName = $this->getAppNamefromPath($path);
if (array_key_exists($appName, $v)) {
$appVersion = $v[$appName];
return '?v=' . substr(md5($appVersion), 0, 8) . $themingSuffix;
}
}
// fallback to the file path instead
if ($file !== false && $file !== '') {
$appName = $this->getAppNamefromPath($file);
if (array_key_exists($appName, $v)) {
$appVersion = $v[$appName];
return '?v=' . substr(md5($appVersion), 0, 8) . $themingSuffix;
}
}
return '?v=' . self::$versionHash . $themingSuffix;
}

To make sure individual tests are reliable, we reset the theming settings completely before each test:

public function undoAll(): void {
$this->config->deleteAppValues('theming');
$this->increaseCacheBuster();
}

This also results in a removed cachebuster value and therefore we get the same urls in different tests which (with the changes of the other PR) results in electron to use a cached version of the file, instead of requesting the latest one.
To fix this, we keep the cachebuster value when resetting theming and increase it afterwards.

TODO

  • Don't increase twice

Checklist

Signed-off-by: Marcel Müller <marcel-mueller@gmx.de>
@SystemKeeper SystemKeeper changed the title fix(theming): Don't reset the cachebuster value when we reset themeing fix(theming): Don't reset the cachebuster value when we reset theming Jul 10, 2024
@SystemKeeper SystemKeeper force-pushed the fix/noid/fix-cypress-test-cachebuster branch from 9b54602 to 4f12584 Compare July 10, 2024 21:57
@SystemKeeper
Copy link
Contributor Author

/backport to stable29

@SystemKeeper
Copy link
Contributor Author

/backport to stable28

@@ -437,7 +437,11 @@
* Revert all settings to the default value
*/
public function undoAll(): void {
// Remember the current cachebuster value, as we do not want to reset this value
// Otherwise this can lead to caching issues as the value might be known to a browser already
$cacheBusterKey = $this->config->getAppValue('theming', 'cachebuster', '0');

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\IConfig::getAppValue has been marked as deprecated
$this->config->deleteAppValues('theming');
$this->config->setAppValue('theming', 'cachebuster', $cacheBusterKey);

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\IConfig::setAppValue has been marked as deprecated
@SystemKeeper SystemKeeper merged commit d707441 into master Jul 11, 2024
167 of 169 checks passed
@SystemKeeper SystemKeeper deleted the fix/noid/fix-cypress-test-cachebuster branch July 11, 2024 05:24
@blizzz blizzz mentioned this pull request Jul 24, 2024
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 tests Related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants