URL: trim leading slashes of file URL paths #278
Conversation
|
When I implement this in whatwg-url I'm left with a single failing test and currently unsure as how to fix it: |
|
I think the problem is that https://url.spec.whatwg.org/#file-slash-state doesn't copy the base's host over and we never tested that path before... I guess code coverage isn't everything. |
|
Should be all good now. Also updated the tests PR. |
url.bs
Outdated
| <li><p>If <var>url</var>'s <a for=url>scheme</a> is "<code>file</code>" and <a>c</a> is | ||
| <a>EOF code point</a>, "<code>?</code>", or "<code>#</code>", then while <var>url</var>'s | ||
| <a for=url>path</a>'s <a for=list>size</a> is not 1 and <var>url</var>'s | ||
| <a for=url>path</a>[0] is the empty string, <a>validation error</a>, <a for=list>remove</a> |
There was a problem hiding this comment.
Path initially can be empty (e.g.: file://) and then path[0] can not be accessed. So I think "is not 1" must be changed to "is greater than 1",
There was a problem hiding this comment.
Either we don't have coverage or that doesn't cause failure in the JavaScript implementation.
There was a problem hiding this comment.
In JavaScript path[0] in such case is undefined, so test path[0] === "" fails. But for example in C++ this can cause exception or runtime error.
There was a problem hiding this comment.
Yeah, that's what I figured. Should maybe assert for length being 0 to make sure we have coverage.
We should probably also clarify in the Infra Standard that [] indexing only works for actual entries (or make it work like JavaScript somehow, but not sure that's better).
There was a problem hiding this comment.
I think just changing "path's size is not 1" to "path's size is greater than 1" solves this problem, because path[0] then will be not accessed.
There was a problem hiding this comment.
So with file:// I think path will still be non-empty due to 1.4.2. We don't have any testcase that triggers path's size being 0. Maybe we don't test file:// explicitly though.
(That's the reason I wanted to add the assert first. I want to make sure it would actually be triggered. I agree that greater than is what we want here.)
There was a problem hiding this comment.
Yeah, I can't trigger it with file://. I'll change to greater than though for clarity.
Also copy file URL hosts correctly for path-absolute input. Tests: web-platform-tests/wpt#5195. Closes #234 by superseding it. Fixes #232.
annevk
force-pushed
the
annevk/file-url-path-slashes
branch
from
b0e1446 to
91bcea4
Compare
Tests: web-platform-tests/wpt#5195.
Closes #234 by superseding it. Fixes #232.
Preview | Diff