Fix URL generation

[LukasReschke]

params in the OC.generateUrl function call only replaces all specified occurences of a key just like the l10n PHP functionality does.

This means that to build a query string we have to use OC.buildQueryString instead of the params parameters.

Fixes #16336 which is a regression introduced with 58a87d0 of #15652.

Without this fix downloading single files from a public shared folder is not possible.

---

# Testplan 1. Share a folder on master publicly 2. Access the public link and downloading single files does download the whole folder as a ZIP 3. Apply the patch, clear your cache, downloading single files and all works again

[LukasReschke]

cc @oparoz @PVince81 please test and review.

Thanks 😄

[LukasReschke]

I will think how to add unit tests meanwhile.

[ghost]

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/12429/
🚀 Test PASSed.🚀

[nickvergessen]

👍

[PVince81]

Am preparing unit tests, there is currently no test file for that section.

[PVince81]

@LukasReschke I added JS unit tests, please check them out 😄

[scrutinizer-notifier]

A new inspection was created.

[oparoz]

I think it's the wrong approach, you don't fix the exception, you fix the method.

This does not work properly:

core/core/js/js.js

Line 135 in 7526886

return text.replace(/{([^{}]*)}/g,

return text.replace(/{([^{}]*)}/g,
    function (a, b) {
        var r = (vars[b]);
        if(allOptions.escape) {
            return (typeof r === 'string' || typeof r === 'number') ? encodeURIComponent(r) : encodeURIComponent(a);
        } else {
            return (typeof r === 'string' || typeof r === 'number') ? r : a;
        }
    }
);

[PVince81]

@oparoz I think it is a confusion about how to use this. I was also confused the first time with generateUrl...
Here is an example how parameters can be used there: https://github.com/owncloud/core/blob/master/apps/files/js/search.js#L105

The substitution is done on "{xxx}" strings inside the URL, not appended.

[oparoz]

Ah, now I remember, thanks @PVince81

In this case, it might be cleaner to use something like this:
return OC.generateUrl('/s/' + token + '/download?path={path}&files={files}', params);

and at some point introduce another method which is smarter and adds the params to the URL.

[PVince81]

Feel free to make the change, I'm pretty sure @LukasReschke won't mind. 😉

If you do, please also make the token a parameter:
return OC.generateUrl('/s/{token}/download?path={path}&files={files}', params); (and make sure to have the token in "params"). I think this is the way it was designed to avoid string concatenation.

[ghost]

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/12434/
🚀 Test PASSed.🚀

[oparoz]

I tested as is and the proper URL was generated, but I don't mind it staying as is. It will probably be changed in a future PR.

But I think this call will fail as well:
https://github.com/owncloud/core/blob/fix-url-generation/apps/files_sharing/js/public.js#L157

I changed it at the same time

[PVince81]

Hmm right. It is likely that "linkTo" isn't used anywhere so nobody found the issue until now.

[oparoz]

It's very possible. A quick search didn't reveal any use of that method. Only calls to OC.linkTo

[LukasReschke]

Feel free to make the change, I'm pretty sure @LukasReschke won't mind. 😉

Absolutely. Generally speaking hijacking my PRs with good intentions and not too questionable (as in could cause discussions) is always appreciated from my site. Just don't squash it then please ;)

[LukasReschke]

Awesome @PVince81!!!!!

[PVince81]

I know it can be painful to "bootstrap" a test file when there is none.
But now that it's there, no more excuses for not adding more tests for the public page 😉

[rullzer]

If you select multiple files (not all of them). the download still generates a zip of the whole share.

[rullzer]

mmm apparently my cache was not completely flushed... it does work.

👍