Skip to content
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

Regression: webDAV-functionality broken on Windows #228

Closed
KennethRobbeFrucon opened this issue Jun 10, 2021 · 7 comments · Fixed by #233
Closed

Regression: webDAV-functionality broken on Windows #228

KennethRobbeFrucon opened this issue Jun 10, 2021 · 7 comments · Fixed by #233
Assignees
Labels
bug Something isn't working
Milestone

Comments

@KennethRobbeFrucon
Copy link

KennethRobbeFrucon commented Jun 10, 2021

The following line was updated in version 2.7.0, but this breaks all webDAV-functionalities on Windows:

var endpoint = require('path').join(WEBDAV_BASE, path)

It uses path.join to join two path segments. However, path.join() is meant to interact with file paths, while this is an URI. This results in a path with backslashes on Windows, which isn't a valid URI. This can be seen by running the unit tests on Windows. All tests for lib/webdav.js fail due to these types of errors:

AssertionError: expected '\\on\\demandware.servlet\\webdav\\Sites\\cartridges\\mycode.zip' to equal '/on/demandware.servlet/webdav/Sites/cartridges/mycode.zip'

In the past, the path was just concatenated:

var endpoint = WEBDAV_BASE + path

@KennethRobbeFrucon
Copy link
Author

@jbachelet / @tobiaslohr Could you please look into this as you did / merged the change that caused the issue? Was this just an oversight or was there a specific reasoning behind using path.join here?

@tobiaslohr
Copy link
Contributor

@KennethRobbeFrucon Agree! This is a bug and from what I can see, there was not a specific reason path.join was used instead. @jordanebachelet please correct me if I'm wrong

@tobiaslohr tobiaslohr added the bug Something isn't working label Jun 16, 2021
@tobiaslohr tobiaslohr self-assigned this Jun 16, 2021
@tobiaslohr
Copy link
Contributor

@jordanebachelet any input you can give, before we revert this particular change?

@jbachelet
Copy link
Contributor

jbachelet commented Jun 25, 2021

@tobiaslohr Apologize for the issue, I was only able to test it on mac, where it works fine.
We should revert this change then

@tobiaslohr
Copy link
Contributor

@jbachelet Thanks. Do you have bandwidth to do this?

@tobiaslohr tobiaslohr added this to the 2.7.2 milestone Jul 5, 2021
@zlatinz
Copy link

zlatinz commented Jul 5, 2021

@jbachelet @tobiaslohr will https://nodejs.org/api/url.html#url_new_url_input_base work instead?
e.g.

var endpoint = new URL(path, WEBDAV_BASE).toString();

@tobiaslohr
Copy link
Contributor

Provided a fix in #233, please review and approve

tobiaslohr added a commit that referenced this issue Jul 5, 2021
#228 fix regression, properly concatenate URL path
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants