-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Use realpath to obtain the webroot. #5252
Conversation
@derkostka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @skjnldsv, @rullzer and @bartv2 to be potential reviewers. |
Please review ! This solves the issue of not finding the webroot in case that it is symlinked, see #5057 |
For me it is working in production since first published the workaround by the original author. |
Would be nice to get in 12.0.1 @LukasReschke so the Nextcloud Box can move to 12 To check, @kyrofa does this solve your problem for the Nextcloud Box? |
@jospoortvliet indeed this seems to make the file app usable once again. However, I still see the odd loading indicator when I initially visit it (to create the admin user/install nextcloud). Note that I've entered nothing. I guess that may be due to something else. |
In Chrome console the warnings that @kyrofa talked about are more concise:
|
And was it working fine for you without that change ? |
no @derkostka , at least for me it was the same before your change |
Indeed, @derkostka these issues happened without this PR, I just thought they were all due to the same issue, and apparently they're not. Or they might be and we're missing a realpath somewhere, who knows. |
I just tested it myself, #5289 seems to be a different topic: There, a Folder within the WebRoot is located elsewhere. This patch does not help there. @jospoortvliet @rullzer : Please review & merge so that at least this part works again. Maybe the original author of the SCSS change can have another look at #5289 |
We really need this in 12.0.1 so the Nc box can move forward with 12... |
Can someone describe what exactly is symlinked? |
The patch helps if the webroot (the folder given in the config) got symlinked So in the config.php: /var/www/nextcloud |
Example: I use ispconfig3 and its filesystem layout. Webroot is, using the example above: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I could reproduce the issue and this PR works.
@derkostka Can you add a signoff message to your commits and maybe squash them into one? https://github.com/nextcloud/server/blob/master/CONTRIBUTING.md#sign-your-work might be helpful for that. 😉 |
Codecov Report
@@ Coverage Diff @@
## master #5252 +/- ##
============================================
+ Coverage 54.14% 54.16% +0.02%
+ Complexity 22345 22302 -43
============================================
Files 1380 1380
Lines 85551 85395 -156
Branches 1329 1321 -8
============================================
- Hits 46323 46256 -67
+ Misses 39228 39139 -89
|
Signed-off-by: Sebastian Kostka <sebastian.kostka@gmail.com>
Done. @juliushaertl Thx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works here 👍
@derkostka Could you prepare a PR against stable12? Thanks |
Sure: #5507 |
Nice, you can see that it's much better using the edge snap, which uses the daily master tarball:
Contrast that to the daily 12 snap (since #5507 hasn't landed yet):
And you'll see how terrible it is. |
@derkostka you're a hero, thanks a lot for the work on this! |
Use realpath to get the correct webroot path, even if it is symlinked. Thanks @sileht: https://gist.github.com/sileht/5753118ee048352778c2a0c5b23ea47d