Store scss variables under a different prefix for each theming config version

[juliusknorr]

The main issue with using the general theming- prefix is that with APCu caching
the cache is not shared between processes, so when trying to reset the cache
through the CLI, e.g. when updating the theming config the old cache is never
invalidated.

To reproduce:

• Configure APCu cache only
• Be not in debug mode
• Run occ theming:config color '#ffffaa'
• Reload the page and see that the color didn't update
• Running occ maintenance:repair also doesn't help

[juliusknorr]

I'm actually wondering if there might be cases where this also causes issues with the SCSS cacher, mainly in cases where there is no distributed redis available and APCu is used instead. I'm not sure yet how exactly the APCu cache is shared across apache/php-fpm workers but I could imagine that this would be something to investigate further on if we run into issues there again.

[skjnldsv]

Oooooh 😮

[faily-bot]

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 36088: failure

nodb

Show full log

There were 26 warnings:
1) Test\AppFramework\Controller\AuthPublicShareControllerTest::testAuthenticateAuthenticated
Trying to configure method "isAuthenticated" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) OCA\DAV\Tests\unit\BackgroundJob\UpdateCalendarResourcesRoomsBackgroundJobTest::testRun
Passing an array of interface names to createMock() for creating a test double that implements multiple interfaces is deprecated and will no longer be supported in PHPUnit 9.
Passing an array of interface names to getMockBuilder() for creating a test double that implements multiple interfaces is deprecated and will no longer be supported in PHPUnit 9.
3) OCA\DAV\Tests\unit\CalDAV\CalendarTest::testConfidentialClassification with data set #0 (3, false)
No method rule is set
4) OCA\DAV\Tests\Command\MoveCalendarTest::testMove
Using assertContains() with string haystacks is deprecated and will not be supported in PHPUnit 9. Refactor your test to use assertStringContainsString() or assertStringContainsStringIgnoringCase() instead.
5) ExpirationTest::testParseRetentionObligation with data set #0 ('disabled', null, null, null)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
6) ExpirationTest::testParseRetentionObligation with data set #1 ('auto', 30, -1, true)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
7) ExpirationTest::testParseRetentionObligation with data set #2 ('auto,auto', 30, -1, true)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
8) ExpirationTest::testParseRetentionObligation with data set #3 ('auto, auto', 30, -1, true)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
9) ExpirationTest::testParseRetentionObligation with data set #4 ('auto, 3', -1, 3, true)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
10) ExpirationTest::testParseRetentionObligation with data set #5 ('5, auto', 5, -1, true)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
11) ExpirationTest::testParseRetentionObligation with data set #6 ('3, 5', 3, 5, false)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
12) ExpirationTest::testParseRetentionObligation with data set #7 ('10, 3', 10, 10, false)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
13) OCA\Files_Versions\Tests\ExpirationTest::testParseRetentionObligation with data set #0 ('disabled', null, null, null)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
14) OCA\Files_Versions\Tests\ExpirationTest::testParseRetentionObligation with data set #1 ('auto', -1, -1, true)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
15) OCA\Files_Versions\Tests\ExpirationTest::testParseRetentionObligation with data set #2 ('auto,auto', -1, -1, true)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
16) OCA\Files_Versions\Tests\ExpirationTest::testParseRetentionObligation with data set #3 ('auto, auto', -1, -1, true)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
17) OCA\Files_Versions\Tests\ExpirationTest::testParseRetentionObligation with data set #4 ('auto, 3', -1, 3, true)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
18) OCA\Files_Versions\Tests\ExpirationTest::testParseRetentionObligation with data set #5 ('5, auto', 5, -1, true)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
19) OCA\Files_Versions\Tests\ExpirationTest::testParseRetentionObligation with data set #6 ('3, 5', 3, 5, false)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
20) OCA\Files_Versions\Tests\ExpirationTest::testParseRetentionObligation with data set #7 ('10, 3', 10, 10, false)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
21) OCA\Files_Versions\Tests\ExpirationTest::testParseRetentionObligation with data set #8 ('g,a,r,b,a,g,e', -1, -1, true)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
22) OCA\Files_Versions\Tests\ExpirationTest::testParseRetentionObligation with data set #9 ('-3,8', -1, -1, true)
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.
23) OCA\Settings\Tests\Mailer\NewUserMailHelperTest::testGenerateTemplateWithPasswordResetToken
Method getTime may not return value of type string, its return declaration is ": int"
24) OCA\TwoFactorBackupCodes\Tests\Unit\Listener\ActivityPublisherTest::testHandleCodesGeneratedEvent
Method publish may not return value of type Mock_IEvent_00171e20, its return declaration is ": void"
25) OCA\UpdateNotification\Tests\Notification\BackgroundJobTest::testCreateNotifications with data set #1 ('app2', '1.0.1', '1.0.0', '1.0.0', true, array('user1'), array(array('user1')))
Method notify may not return value of type Mock_INotification_fe829bef, its return declaration is ": void"
26) OCA\UpdateNotification\Tests\Notification\BackgroundJobTest::testCreateNotifications with data set #2 ('app3', '1.0.1', false, false, true, array('user2', 'user3'), array(array('user2'), array('user3')))
Method notify may not return value of type Mock_INotification_fe829bef, its return declaration is ": void"
--
There were 2 failures:
1) OCA\Theming\Tests\ThemingDefaultsTest::testGetScssVariablesCached
Expectation failed for method name is "createDistributed" when invoked 1 time(s)
Parameter 0 for invocation OCP\ICacheFactory::createDistributed('theming--'): OCP\ICache does not match expected value.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'theming-'
+'theming--'
/drone/src/apps/theming/lib/ThemingDefaults.php:294
/drone/src/apps/theming/tests/ThemingDefaultsTest.php:642
2) OCA\Theming\Tests\ThemingDefaultsTest::testGetScssVariables
Expectation failed for method name is "createDistributed" when invoked 1 time(s)
Parameter 0 for invocation OCP\ICacheFactory::createDistributed('theming-0-'): OCP\ICache does not match expected value.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'theming-'
+'theming-0-'
/drone/src/apps/theming/lib/ThemingDefaults.php:294
/drone/src/apps/theming/tests/ThemingDefaultsTest.php:685

mysql8.0-php7.4

Show full log

There was 1 error:
1) Test\Files\ObjectStore\ObjectStoreStorageTest::testCopyFromSameStorage with data set #2 ('/source with space.txt', '/target.txt')
copy(/tmp/oc_tmp_RWfiht-folder/urn:oid:1366): failed to open stream: No such file or directory
/drone/src/lib/private/Files/Storage/Local.php:351
/drone/src/lib/private/Files/ObjectStore/StorageObjectStore.php:98
/drone/src/lib/private/Files/ObjectStore/ObjectStoreStorage.php:579
/drone/src/lib/private/Files/ObjectStore/ObjectStoreStorage.php:560
/drone/src/lib/private/Files/ObjectStore/ObjectStoreStorage.php:542
/drone/src/lib/private/Files/Storage/Common.php:608
/drone/src/tests/lib/Files/Storage/Storage.php:597
--
There were 2 warnings:
1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
--
There was 1 failure:
1) Test\Share20\DefaultShareProviderTest::testGetSharedWithUser with data set #2 ('smb::whatever', 'files_trashbin/files/test.txt', 'files_trashbin/files/test2.txt')
Failed asserting that '44' matches expected 0.
/drone/src/tests/lib/Share20/DefaultShareProviderTest.php:958

[juliusknorr]

Will have a look at the tests.

[MorrisJobke]

@juliushaertl But this is using createDistributed which should not get an APCu instance, right? Or did you manually configure APCu as distributed cache?

[MorrisJobke]

Code makes sense. 👍

[juliusknorr]

@juliushaertl But this is using createDistributed which should not get an APCu instance, right? Or did you manually configure APCu as distributed cache?

It falls back to the local cache class if none is configured:

server/lib/private/Memcache/Factory.php

Lines 82 to 84 in cb05782

	if (!$distributedCacheClass) {
	$distributedCacheClass = $localCacheClass;
	}

[MorrisJobke]

It falls back to the local cache class if none is configured:

That is not good ... because code actually depends on the fact, that if requesting a distributed one, then also the requirements of actually sharing this cache between everything needs to be met, otherwise such stuff can happen. 🙈

[juliusknorr]

Hmm, i also wondered if about that. The main issue is that falling back to a NullCache would of course have quite some performance impact on instances that do not configure a distributed cache, which I would assume are quite some out there.

[juliusknorr]

/backport to stable19

[juliusknorr]

/backport to stable20

[MorrisJobke]

The main issue is that falling back to a NullCache would of course have quite some performance impact on instances that do not configure a distributed cache, which I would assume are quite some out there.

But trading a potential performance impact (that can be solved by properly configuring it) for a lot of hidden bugs that only appear under specific race conditions is not a good deal IMO.