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

Avoid file system access on checking if an image exists #30624

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

juliusknorr
Copy link
Member

Save some time and potential file system setup when obtaining capabilities (which is done on every template response) for gathering theming capabilities by not getting the background image file but just checking if one is configured.

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

I see why it was done the old way. But it basically means the cache (appconfig) is not trusted, so it's not helping.

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81
Copy link
Member

1) OCA\Theming\Tests\ImageManagerTest::testGetImageUrl
Expectation failed for method name is "getFolder" when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

2) OCA\Theming\Tests\ImageManagerTest::testGetImageUrlDefault
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'logo/logo.png?v=0'
+'?v=0'

/drone/src/apps/theming/tests/ImageManagerTest.php:158

3) OCA\Theming\Tests\ImageManagerTest::testGetImageUrlAbsolute
Expectation failed for method name is "getFolder" when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

@juliushaertl please adjust the tests

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jan 20, 2022
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the theming/background-image branch from 90376e2 to 3d0b5c1 Compare February 2, 2022 10:26
@juliusknorr
Copy link
Member Author

Tests adjusted, samba failure unrelated.

@juliusknorr juliusknorr merged commit 20f1971 into master Feb 3, 2022
@juliusknorr juliusknorr deleted the theming/background-image branch February 3, 2022 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants