Skip to content

[Stable12] fix scss webroot and url rewrite - #7688

Merged
MorrisJobke merged 15 commits into
stable12from
stable12-fix-scss-webroot-and-url-rewrite
Jan 5, 2018
Merged

[Stable12] fix scss webroot and url rewrite#7688
MorrisJobke merged 15 commits into
stable12from
stable12-fix-scss-webroot-and-url-rewrite

Conversation

@skjnldsv

@skjnldsvskjnldsv commented Jan 3, 2018

Copy link
Copy Markdown
Member

Backport of #7631

Allow scss to proberly work and cache file whatever situation they're in:

  • Sub-directory nc installation
  • Sub-directory nc installation with a non-root app_path
  • Basic nc installation with a non-root path

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Fixed tests
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
- With root installation
- Core css
- App inside server root
- Secondary apps directory outside server root
- With an installation in a sub directory
- Core css
- App inside server root
- Secondary apps directory outside server root
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@codecov

codecovBot commented Jan 4, 2018

Copy link
Copy Markdown

Codecov Report

Merging #7688 into stable12 will decrease coverage by 16.66%.
The diff coverage is 0%.

@@ Coverage Diff @@## stable12 #7688 +/- ##
===============================================
- Coverage 53.92% 37.26% -16.67% 
Complexity 22712 22712 ===============================================
Files 1385 1385 Lines 86996 86998 +2 Branches 1330 1330 ===============================================
- Hits 46913 32416 -14497 - Misses 40083 54582 +14499
Impacted FilesCoverage ΔComplexity Δ
lib/private/Template/CSSResourceLocator.php0% <0%> (-72.14%)26 <0> (ø)
lib/private/Template/SCSSCacher.php6.29% <0%> (-63.31%)35 <3> (ø)
...yption/Exceptions/ModuleAlreadyExistsException.php0% <0%> (-100%)1% <0%> (ø)
lib/private/App/CodeChecker/EmptyCheck.php0% <0%> (-100%)6% <0%> (ø)
...pps/files/lib/Activity/Settings/FavoriteAction.php0% <0%> (-100%)8% <0%> (ø)
...mments/tests/Unit/Controller/NotificationsTest.php0% <0%> (-100%)4% <0%> (ø)
apps/systemtags/lib/Settings/Admin.php0% <0%> (-100%)3% <0%> (ø)
...ddleware/Security/Exceptions/NotAdminException.php0% <0%> (-100%)1% <0%> (ø)
apps/dav/lib/CalDAV/Activity/Setting/Calendar.php0% <0%> (-100%)8% <0%> (ø)
.../Exceptions/EncryptionHeaderKeyExistsException.php0% <0%> (-100%)1% <0%> (ø)
... and 481 more

@juliusknorr

juliusknorr commented Jan 4, 2018

Copy link
Copy Markdown
Member

@skjnldsv This breaks core icons after changing the color value in the theming app (what triggers rebuilding the core scss):

.icon-settings {
background-image: url('//core/css/../img/actions/settings.svg?v=1');
}

Also happens on master after #7631 🙈

@juliusknorrjuliusknorr 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.

See comment.

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv

Copy link
Copy Markdown
MemberAuthor

@juliushaertl Hum, strange, I fixed it though. I don't get why there was this error.
Could you check again?

@juliusknorr

Copy link
Copy Markdown
Member

Opened #7704 so we don't forget to fix that on master as well.

@kyrofakyrofa 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.

I see what @juliushaertl is saying on the snap as well (but not in 12.0.4). Assuming that wasn't introduced in this PR (was it?), +1 from me. I see no behavioral change other than that.

@kyrofakyrofa 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.

Oh wait, #7631 was this PR to master, huh. Doh! That means this PR does introduce this behavior.

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

@kyrofa yes! :p
Let me check

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv

Copy link
Copy Markdown
MemberAuthor

@juliushaertl can you check again, Maybe we could get rid of the cache reset now that it will proberly erase the files when compiling again? :)

skjnldsvand others added 2 commits January 4, 2018 19:11
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr

Copy link
Copy Markdown
Member

@kyrofa Please have another look :)

Maybe we could get rid of the cache reset now that it will proberly erase the files when compiling again? :)

I don't think so, since we only detect changed variables globally and not per file, we need to make sure that all scss files are invalidated when variables change.

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv

Copy link
Copy Markdown
MemberAuthor

@juliushaertl Okay here for me! :)

@kyrofakyrofa 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.

There we go, that seems to work perfectly, even when changing the theme color.

@MorrisJobke
MorrisJobke merged commit 28d77ef into stable12Jan 5, 2018
@MorrisJobke
MorrisJobke deleted the stable12-fix-scss-webroot-and-url-rewrite branch January 5, 2018 16:29
@MorrisJobkeMorrisJobke mentioned this pull request Jan 8, 2018
13 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@skjnldsv@juliusknorr@rullzer@kyrofa@MorrisJobke