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

[stable12] {J,CS}SResourceLocator: account for symlinks in app path #7170

Merged
merged 1 commit into from Nov 20, 2017

Conversation

Projects
None yet
4 participants
@kyrofa
Member

kyrofa commented Nov 14, 2017

Currently, if the app path includes a symlink, the calculated webDir will be incorrect when generating CSS and URLs will be pointing to the wrong place, breaking CSS.

This PR is a backport of #7061, fixing #6028 for stable12 by using realpath when retrieving app path, which makes these issues go away.

{J,CS}SResourceLocator: account for symlinks in app path
Currently, if the app path includes a symlink, the calculated webDir
will be incorrect when generating CSS and URLs will be pointing to the
wrong place, breaking CSS.

Use realpath when retrieving app path, and these issues go away.

Fix #6028

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>

@kyrofa kyrofa changed the title from {J,CS}SResourceLocator: account for symlinks in app path to [stable12] {J,CS}SResourceLocator: account for symlinks in app path Nov 14, 2017

@MorrisJobke MorrisJobke added this to the Nextcloud 12.0.4 milestone Nov 14, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 14, 2017

Codecov Report

Merging #7170 into stable12 will increase coverage by 0.03%.
The diff coverage is 66.66%.

@@              Coverage Diff               @@
##             stable12    #7170      +/-   ##
==============================================
+ Coverage       53.76%   53.79%   +0.03%     
- Complexity      22587    22588       +1     
==============================================
  Files            1384     1384              
  Lines           86660    86663       +3     
  Branches         1329     1329              
==============================================
+ Hits            46593    46621      +28     
+ Misses          40067    40042      -25
Impacted Files Coverage Δ Complexity Δ
lib/private/Template/CSSResourceLocator.php 0% <0%> (ø) 27 <0> (ø) ⬇️
lib/private/Template/JSResourceLocator.php 50.9% <100%> (+50.9%) 23 <0> (+1) ⬆️
lib/private/Files/Cache/Propagator.php 94.93% <0%> (-1.27%) 16% <0%> (ø)
lib/private/Template/ResourceLocator.php 66.66% <0%> (+1.85%) 20% <0%> (ø) ⬇️

codecov bot commented Nov 14, 2017

Codecov Report

Merging #7170 into stable12 will increase coverage by 0.03%.
The diff coverage is 66.66%.

@@              Coverage Diff               @@
##             stable12    #7170      +/-   ##
==============================================
+ Coverage       53.76%   53.79%   +0.03%     
- Complexity      22587    22588       +1     
==============================================
  Files            1384     1384              
  Lines           86660    86663       +3     
  Branches         1329     1329              
==============================================
+ Hits            46593    46621      +28     
+ Misses          40067    40042      -25
Impacted Files Coverage Δ Complexity Δ
lib/private/Template/CSSResourceLocator.php 0% <0%> (ø) 27 <0> (ø) ⬇️
lib/private/Template/JSResourceLocator.php 50.9% <100%> (+50.9%) 23 <0> (+1) ⬆️
lib/private/Files/Cache/Propagator.php 94.93% <0%> (-1.27%) 16% <0%> (ø)
lib/private/Template/ResourceLocator.php 66.66% <0%> (+1.85%) 20% <0%> (ø) ⬇️
@kyrofa

This comment has been minimized.

Show comment
Hide comment
@kyrofa

kyrofa Nov 17, 2017

Member

@juliushaertl this review is identical to the one you've already done. Would you mind taking a quick look when you have a moment?

Member

kyrofa commented Nov 17, 2017

@juliushaertl this review is identical to the one you've already done. Would you mind taking a quick look when you have a moment?

@kyrofa kyrofa referenced this pull request Nov 17, 2017

Closed

Update to 12.0 #280

@MorrisJobke MorrisJobke referenced this pull request Nov 20, 2017

Merged

12.0.4 RC #7225

1 of 1 task complete

@rullzer rullzer merged commit 6bfeec0 into nextcloud:stable12 Nov 20, 2017

3 checks passed

codecov/patch 66.66% of diff hit (target 53.76%)
Details
codecov/project 53.79% (+0.03%) compared to 82e4d9b
Details
continuous-integration/drone/pr the build was successful
Details
@pipiche38

This comment has been minimized.

Show comment
Hide comment
@pipiche38

pipiche38 Dec 18, 2017

This issue seems to still be in 12.0.4
Here is an extract of my config file and if the app is stored on the /var/lib/nextcloud/apps directory - and not on the /usr/share/nextcloud/apps - I got the issue
'apps_paths' =>
array (
0 =>
array (
'path' => '/usr/share/nextcloud/apps',
'url' => '/apps',
'writable' => false,
),
1 =>
array (
'path' => '/var/lib/nextcloud/apps',
'url' => '/apps-appstore',
'writable' => true,
),
),

pipiche38 commented Dec 18, 2017

This issue seems to still be in 12.0.4
Here is an extract of my config file and if the app is stored on the /var/lib/nextcloud/apps directory - and not on the /usr/share/nextcloud/apps - I got the issue
'apps_paths' =>
array (
0 =>
array (
'path' => '/usr/share/nextcloud/apps',
'url' => '/apps',
'writable' => false,
),
1 =>
array (
'path' => '/var/lib/nextcloud/apps',
'url' => '/apps-appstore',
'writable' => true,
),
),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment