Remove encodeURI code #384
Conversation
The errors that required encodeURIComponent in the past have been mittigated by the following commit on nextcloud viewer. nextcloud/viewer@02e1b1a The code with hasScheme may be removed, viewer now returns a full properly encoded URI thanks to the work done in nextcloud-router fixes #381 Signed-off-by: Robbert Gurdeep Singh <git@beardhatcode.be>
beardhatcode
force-pushed
the
fix/encoding
branch
from
0ac3a44 to
6d81ffb
Compare
|
I tested this for local files with the name "2020_opgave 🪓 NL .pdf" in a folder that also contained an emoji in its name Someone still needs to test if federation works. But it should, the previewer that gets called with the full URI: |
|
/backport to stable21 |
|
/backport to stable20 |
|
I was wondering if there is a way to ship updates to this app outside a release of Nextcloud, and if that is something that would be appropriate for this issue? |
|
This bug breaks the PDF viewer in all NC 21.0.2 installations. I guess one of the most used features. I've applied the hotfix from @beardhatcode manually, but in the interest of all NC users this update should be pushed ASAP and not only with the next NC release. Just my 2 cent. However I wonder how this did pass release testing ... |
|
@MorrisJobke maybe this is something you can make a quick release? 🤔 |
No unfortunately :( |
|
@beardhatcode maybe it would be worth adding one or two cypress tests here as well, if you have time? Thanks a lot for all you did and do 🤗 🚀 |
| encodedDavPath() { | ||
| const hasScheme = this.davPath.indexOf('://') !== -1 | ||
|
|
||
| if (this.davPath.indexOf(generateUrl('/s/')) === 0) { | ||
| const host = window.location.protocol + '//' + window.location.host | ||
| const url = new URL(hasScheme ? this.davPath : host + this.davPath) | ||
| const path = this.filename.replace(this.basename, '') | ||
| url.searchParams.set('path', path) | ||
| url.searchParams.set('files', this.basename) | ||
| return hasScheme ? url.toString() : url.toString().substr(host.length) | ||
| } | ||
|
|
||
| const pathSections = this.davPath.split('/') | ||
|
|
||
| // Ignore scheme and domain in the loop (note that the scheme | ||
| // delimiter, "//", creates an empty section when split by "/"). | ||
| const initialSection = hasScheme ? 3 : 0 | ||
|
|
||
| let encodedDavPath = '' | ||
| for (let i = initialSection; i < pathSections.length; i++) { | ||
| if (pathSections[i] !== '') { | ||
| encodedDavPath += '/' + encodeURIComponent(pathSections[i]) | ||
| } | ||
| } | ||
|
|
||
| if (hasScheme) { | ||
| encodedDavPath = pathSections[0] + '//' + pathSections[2] + encodedDavPath | ||
| } | ||
|
|
||
| return encodedDavPath | ||
| }, |
There was a problem hiding this comment.
Good rid of this legacy code! 🧹
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
/backport to stable20 |
|
/backport to stable21 |
|
The patch worked here, thanks. |
|
Many thanks of this fix, but how long will we need to wait to this get into an actual Nextcloud release? I'm fairly flabbergasted that 21.0.2 was released without basic PDF support! Really very disappointing 😞 |
The next regular release is scheduled for 2021-07-01 (https://github.com/nextcloud/server/wiki/Maintenance-and-Release-Schedule) |
The errors that required encodeURIComponent in the past have been
mittigated by the following commit on nextcloud viewer.
nextcloud/viewer@02e1b1a
The code with hasScheme may be removed, viewer now returns a full
properly encoded URI thanks to the work done in nextcloud-router
fixes #381
ℹ️ You can teporarily apply this patch by replacing
apps/files_pdfviewer/js/files_pdfviewer-main.json your instance with this file from this PR. Clear your browser's cache and try to open pdfs.That should fix the issue. Please leave feedback if that does or doesn't work for you.