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 scss webroot and url rewrite #7631

Merged
merged 8 commits into from
Jan 2, 2018

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Dec 27, 2017

Fixes #7534
Also fixes the following situations:

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

➡️ Fix the massiv log spamming of not finding the webroot

TODO:

  • Testing

@codecov
Copy link

codecov bot commented Dec 27, 2017

Codecov Report

Merging #7631 into master will increase coverage by 0.01%.
The diff coverage is 80%.

@@             Coverage Diff              @@
##             master    #7631      +/-   ##
============================================
+ Coverage     51.17%   51.18%   +0.01%     
- Complexity    24886    24889       +3     
============================================
  Files          1602     1602              
  Lines         94752    94798      +46     
  Branches       1368     1368              
============================================
+ Hits          48486    48522      +36     
- Misses        46266    46276      +10
Impacted Files Coverage Δ Complexity Δ
lib/private/Template/CSSResourceLocator.php 0% <0%> (ø) 26 <0> (ø) ⬇️
lib/private/Template/SCSSCacher.php 71.09% <100%> (+1.25%) 37 <3> (+1) ⬆️
apps/dav/composer/composer/autoload_static.php 0% <0%> (ø) 2% <0%> (+1%) ⬆️
core/templates/login.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
core/Controller/LoginController.php 84.02% <0%> (+6.16%) 39% <0%> (+1%) ⬆️

@skjnldsv skjnldsv force-pushed the fix-scss-webroot-and-url-rewrite branch 4 times, most recently from 1d9da6c to 5a9d097 Compare December 28, 2017 09:09
@rullzer rullzer added this to the Nextcloud 13 milestone Dec 28, 2017
@skjnldsv skjnldsv force-pushed the fix-scss-webroot-and-url-rewrite branch 16 times, most recently from e5bb608 to c732f56 Compare December 29, 2017 06:37
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 29, 2017
@skjnldsv
Copy link
Member Author

Test finally fixed! :)
@LukasReschke you forgot to revert back the original WEBROOT in multiple tests, it's now fixed. I added you as reviewer for that 😉

@zauguin please test! 😃

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>
@skjnldsv skjnldsv force-pushed the fix-scss-webroot-and-url-rewrite branch from c732f56 to c0c4443 Compare December 30, 2017 04:56
@MorrisJobke MorrisJobke mentioned this pull request Jan 2, 2018
30 tasks
@zauguin
Copy link

zauguin commented Jan 2, 2018

Works fine. Awesome👍

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works here 👍

@MorrisJobke MorrisJobke merged commit aac9a1c into master Jan 2, 2018
@MorrisJobke MorrisJobke deleted the fix-scss-webroot-and-url-rewrite branch January 2, 2018 14:03
@skjnldsv
Copy link
Member Author

skjnldsv commented Jan 3, 2018

We need a backport imho :)
@karlitschek @MorrisJobke @rullzer

@MorrisJobke
Copy link
Member

@skjnldsv Do it ;)

@karlitschek
Copy link
Member

Backport is fine. Please note that this change has the potential to break in esoteric/non standard webserver setups. So good testing is important.

@skjnldsv
Copy link
Member Author

skjnldsv commented Jan 3, 2018

@karlitschek that's why I added tests with various installation instances to make sure it doesn't break. :)

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.

13.0.0 Beta 3: Wrong paths in App CSS files with multiple app_paths
6 participants