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

Use realpath to obtain the webroot. #5252

Merged
merged 1 commit into from
Jun 21, 2017
Merged

Use realpath to obtain the webroot. #5252

merged 1 commit into from
Jun 21, 2017

Conversation

derkostka
Copy link
Contributor

Use realpath to get the correct webroot path, even if it is symlinked. Thanks @sileht: https://gist.github.com/sileht/5753118ee048352778c2a0c5b23ea47d

@mention-bot
Copy link

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

@derkostka
Copy link
Contributor Author

derkostka commented Jun 5, 2017

Please review !

This solves the issue of not finding the webroot in case that it is symlinked, see #5057

@pongraczi
Copy link

For me it is working in production since first published the workaround by the original author.

@jospoortvliet
Copy link
Member

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?

@kyrofa
Copy link
Member

kyrofa commented Jun 8, 2017

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

screenshot from 2017-06-08 14-45-14

@kyrofa
Copy link
Member

kyrofa commented Jun 8, 2017

I also still see these in my Firefox dev console:

screenshot from 2017-06-08 15-15-57

@pachulo
Copy link
Contributor

pachulo commented Jun 9, 2017

In Chrome console the warnings that @kyrofa talked about are more concise:

Resource interpreted as Stylesheet but transferred with MIME type text/html: "http://localhost/index.php/apps/files"

@derkostka
Copy link
Contributor Author

And was it working fine for you without that change ?

@pachulo
Copy link
Contributor

pachulo commented Jun 10, 2017

no @derkostka , at least for me it was the same before your change

@kyrofa
Copy link
Member

kyrofa commented Jun 10, 2017

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.

@derkostka
Copy link
Contributor Author

derkostka commented Jun 10, 2017

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

@jospoortvliet
Copy link
Member

We really need this in 12.0.1 so the Nc box can move forward with 12...

@nickvergessen
Copy link
Member

Can someone describe what exactly is symlinked?

@derkostka
Copy link
Contributor Author

The patch helps if the webroot (the folder given in the config) got symlinked

So in the config.php: /var/www/nextcloud

@pongraczi
Copy link

pongraczi commented Jun 20, 2017

Example:
/var/www/cbox.palyazatnelkul.hu -> /var/www/clients/client3/web15/

I use ispconfig3 and its filesystem layout. Webroot is, using the example above:
/var/www/cbox.palyazatnelkul.hu/web which is under the symlink.

Copy link
Member

@juliusknorr juliusknorr left a 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.

@juliusknorr
Copy link
Member

@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
Copy link

codecov bot commented Jun 20, 2017

Codecov Report

Merging #5252 into master will increase coverage by 0.02%.
The diff coverage is 50%.

@@             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
Impacted Files Coverage Δ Complexity Δ
lib/private/Template/CSSResourceLocator.php 0% <0%> (ø) 27 <0> (ø) ⬇️
lib/private/Template/ResourceLocator.php 64.81% <100%> (ø) 20 <0> (ø) ⬇️
lib/private/App/CodeChecker/InfoChecker.php 69.56% <0%> (-9.16%) 24% <0%> (+9%)
apps/systemtags/js/systemtagsinfoview.js 93.22% <0%> (-5.06%) 0% <0%> (ø)
lib/private/URLGenerator.php 75.67% <0%> (-1.96%) 46% <0%> (-18%)
core/js/systemtags/systemtagsinputfield.js 81.18% <0%> (-1.62%) 0% <0%> (ø)
apps/dav/lib/Connector/Sabre/BearerAuth.php 85.71% <0%> (-1.25%) 5% <0%> (-1%)
core/js/js.js 61.19% <0%> (-0.64%) 0% <0%> (ø)
lib/private/Template/SCSSCacher.php 68.85% <0%> (-0.51%) 34% <0%> (-1%)
apps/files/lib/Controller/ViewController.php 75.96% <0%> (-0.36%) 17% <0%> (-1%)
... and 38 more

Signed-off-by: Sebastian Kostka <sebastian.kostka@gmail.com>
@derkostka
Copy link
Contributor Author

derkostka commented Jun 20, 2017

Done. @juliushaertl Thx

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.

Works here 👍

@MorrisJobke MorrisJobke merged commit 6a1c499 into nextcloud:master Jun 21, 2017
@MorrisJobke
Copy link
Member

@derkostka Could you prepare a PR against stable12? Thanks

@derkostka
Copy link
Contributor Author

Sure: #5507
(i am getting used to GitHub now)

@kyrofa
Copy link
Member

kyrofa commented Jun 23, 2017

Nice, you can see that it's much better using the edge snap, which uses the daily master tarball:

$ sudo snap install nextcloud --channel=edge

Contrast that to the daily 12 snap (since #5507 hasn't landed yet):

$ sudo snap install nextcloud --channel=12/edge

And you'll see how terrible it is.

@jospoortvliet
Copy link
Member

@derkostka you're a hero, thanks a lot for the work on this!

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.

9 participants