Skip to content

Conversation

@PVince81
Copy link
Contributor

Description

Affects the URL as displayed in the files app cog icon and also the one
returned in the "Webdav-Location" header

Related Issue

Follow up of #30383 (comment)

Motivation and Context

How Has This Been Tested?

Unit tests

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Affects the URL as displayed in the files app cog icon and also the one
returned in the "Webdav-Location" header
@codecov
Copy link

codecov bot commented Feb 16, 2018

Codecov Report

Merging #30503 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #30503   +/-   ##
=========================================
  Coverage     61.59%   61.59%           
  Complexity    18505    18505           
=========================================
  Files          1090     1090           
  Lines         61104    61104           
=========================================
  Hits          37640    37640           
  Misses        23464    23464
Impacted Files Coverage Δ Complexity Δ
apps/files/lib/Controller/ViewController.php 90.97% <100%> (ø) 27 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9eb7fe...5a1c687. Read the comment docs.

@SamuAlfageme
Copy link

@PVince81 does seem to work like a charm now 👍

I've noticed there's a difference on how the rawurlencode() and \OCP\Util::encodePath() work out. i.e. normal PROPFINDs contents look like this:

<d:response>
    <d:href>/remote.php/dav/files/lun@/tigr@/leon%c3%a9/</d:href>
    <d:propstat>

Notice how @ are not translated into %40 - IIRC with this PR the Webdav-Locationfor that response would be /remote.php/dav/files/lun%40/tigr@/leon%c3%a9/ (the uid part is rawurlencoded). Since both routes resolve correctly I'll consider this pretty minor though, dunno if it's something we might want to adjust.

LGTM otherwise.

@PVince81
Copy link
Contributor Author

Yeah, I remember that difference as Sabre has its own encoding logic. I seem to remember that I had to encode the "@" signs to %40 in Javascript else some Webdav backend code would not work correctly. So here I opted for the same approach to keep it safe.

Thanks for testing!

Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

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

Tested 👍

@DeepDiver1975 DeepDiver1975 merged commit 8810cc2 into master Feb 23, 2018
@DeepDiver1975 DeepDiver1975 deleted the encode-webdav-location-header branch February 23, 2018 13:43
@DeepDiver1975
Copy link
Member

backporting ....

@lock
Copy link

lock bot commented Aug 1, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants