Refactor webroot detection in resource locator#6332
Conversation
|
@tux-rampage, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @bartv2 and @skjnldsv to be potential reviewers. |
MorrisJobke
requested review from
juliusknorr,
nickvergessen,
rullzer and
skjnldsv
MorrisJobke
added
3. to review
|
One positive feedback already: #6028 (comment) |
| * @param string $root | ||
| * @return string|null The web root or null on failure | ||
| */ | ||
| protected function findWebRoot($root) |
There was a problem hiding this comment.
Please put it on the end of the line before this.
There was a problem hiding this comment.
What do you men? Putting the method to the bottom of the class?
There was a problem hiding this comment.
He means the bracket on the end of line :)
|
|
||
| if (!$webRoot) { | ||
| $realpath = realpath($root); | ||
|
|
MorrisJobke
left a comment
MorrisJobke
left a comment
There was a problem hiding this comment.
Code change makes sense and still works 👍
Comments from above still apply of course 😉 |
|
Do you want me to push a squashed commit, or will it be OK to simply push the requested changes? |
Squashed would be awesome :) |
The current implementation breaks installations with symlinks to directories inside the webroot (i.E. apps). With this change both variants, directory and symlinks, will be detected correctly. Fixes: nextcloud#6028 Signed-off-by: Axel Helmert <axel.helmert@luka.de>
tux-rampage
force-pushed
the
hotfix/6028-realpath-on-webroot
branch
from
7a52341 to
7a33b92
Compare
Codecov Report
@@ Coverage Diff @@
## master #6332 +/- ##
=============================================
- Coverage 53.32% 30.55% -22.77%
+ Complexity 22509 22500 -9
=============================================
Files 1407 1403 -4
Lines 87149 87080 -69
Branches 1329 1327 -2
=============================================
- Hits 46471 26608 -19863
- Misses 40678 60472 +19794
|
|
Added style fixes as requested - squashed commit |
skjnldsv
added
4. to release
| parent::append($root, $file, $webRoot, $throw); | ||
| } else { | ||
| if (!$webRoot) { | ||
| $tmpRoot = realpath($root); |
There was a problem hiding this comment.
This removed var is still used in line 134 which results in error msgs: Undefined variable: tmpRoot
There was a problem hiding this comment.
Nice catch, I ashamed to have missed that! 🙈
|
True, should I add another PR? |
|
@tux-rampage yes :) |
5 participants
The current implementation breaks installations with symlinks to
directories inside the webroot (i.E. apps).
With this change both variants, directory and symlinks) will be detected
correctly.
Fixes: #6028