Failure to detect URI with no filename extension
Closed this issue · 11 comments
Millstone fails to detect URI to be localized from this style:
#test_table_3{ marker-file: url('http://localhost:8033/notfound'); marker-transform:'scale(0.2)'; [mapnik-geometry-type=1] { marker-placement:point; marker-type:ellipse; } [mapnik-geometry-type>1] { marker-placement:line; marker-type:arrow; marker-transform:scale(.5, .5); marker-clip:false; } marker-multi-policy:whole; }
Could have been introduced by 3d23d3e /cc @springmeyer
Simpler example, using the regexp found in code:
"#test_table_3{ marker-file: url('http://localhost:8033/notfound'); }".match(/[-a-zA-Z0-9@:%_\+.~#?&//=]{2,256}\.[a-z]{2,4}\b(\/[-a-zA-Z0-9@:%_\+.~#?&//=]*)?/gi);
I get a null ...
The \. part of the regexp seems to be the culprit. The RegEXP requires URIs to have an extension of 2 to 4 characters. I don't think that's a valid assumption.
happy to accept a better regex if you can find or come up with one. thanks for the report.
I remember that part of the code, last time seen when fixing the
bug with special characters in urls (like the plus "+" sign).
At that time I remember I haven't found an elegant way to extract
all urls at once, but getting out the whole url() part could be
easy. Than a later processing should extract the actual url.
I mean that I could work on it but would take more than just a single
regexp. Maybe using a function as the pattern would help in that
it could extract the urls and replace with the localized version.
Can take a look tomorrow, if you don't beat me at it
Can you give an hint about where to add a test for this extraction process ?
what kind of hint are you looking for?
path of an existing testcase, if any, where I can just add a few lines for the new case
could be added to https://github.com/mapbox/millstone/blob/master/test/markers/style.mss and then the line here (https://github.com/mapbox/millstone/blob/master/test/markers.test.js#L31) would need to be updated.
I tried adding a test but to my surprise it worked fine. Maybe it's due to the presence of a dot in the domain name (my problem was with fetching from localhost). The test you point me at really wants to download a file so I can't just fake an url, so unless we start our own server as part of the testsuite, there's no way to automate test for this.
In any case, here's the patch adding a test for extension-less url. I'm not sure if the obtained translation is acceptable as uses the ampersend as part of the local path:
diff --git a/test/markers.test.js b/test/markers.test.js
index 54e6209..d48d5db 100644
--- a/test/markers.test.js
+++ b/test/markers.test.js
@@ -28,7 +28,7 @@ it('correctly localizes remote image/svg files', function(done) {
millstone.resolve(options, function(err, resolved) {
assert.equal(err,undefined,err);
assert.equal(resolved.Stylesheet[0].id, 'style.mss');
- assert.equal(resolved.Stylesheet[0].data, '// a url like https:example.com in the comments\n#points { one/marker-file: url(\'/tmp/millstone-test/e33af80e-Cup_of_coffee.svg\'); two/marker-file: url(\'/tmp/millstone-test/e33af80e-Cup_of_coffee.svg\'); three/marker-file: url(\'/tmp/millstone-test/ce5fcc0b-SVG-logo.svg\'); four/marker-file: url("/tmp/millstone-test/c953e0d1-pin-m-fast-food+AA0000.png"); five/marker-file:url("/tmp/millstone-test/a2278544-octocat-svg/a2278544-octocat-svg.svg"); }');
+ assert.equal(resolved.Stylesheet[0].data, '// a url like https:example.com in the comments\n#points { one/marker-file: url(\'/tmp/millstone-test/e33af80e-Cup_of_coffee.svg\'); two/marker-file: url(\'/tmp/millstone-test/e33af80e-Cup_of_coffee.svg\'); three/marker-file: url(\'/tmp/millstone-test/ce5fcc0b-SVG-logo.svg\'); four/marker-file: url("/tmp/millstone-test/c953e0d1-pin-m-fast-food+AA0000.png"); five/marker-file:url("/tmp/millstone-test/a2278544-octocat-svg/a2278544-octocat-svg.svg"); six/marker-file:url("/tmp/millstone-test/7b9b9979-fff&text=x/7b9b9979-fff&text=x.png"); }\n');
assert.deepEqual(resolved.Layer, [
{
"name": "points",
diff --git a/test/markers/style.mss b/test/markers/style.mss
index 13267ae..7d3d665 100644
--- a/test/markers/style.mss
+++ b/test/markers/style.mss
@@ -1,2 +1,2 @@
// a url like https:example.com in the comments
-#points { one/marker-file: url('http://upload.wikimedia.org/wikipedia/commons/7/72/Cup_of_coffee.svg'); two/marker-file: url('http://upload.wikimedia.org/wikipedia/commons/7/72/Cup_of_coffee.svg'); three/marker-file: url('http://upload.wikimedia.org/wikipedia/en/c/ce/SVG-logo.svg'); four/marker-file: url("http://a.tiles.mapbox.com/v3/marker/pin-m-fast-food+AA0000.png"); five/marker-file:url("http://ds.io/octocat-svg"); }
\ No newline at end of file
+#points { one/marker-file: url('http://upload.wikimedia.org/wikipedia/commons/7/72/Cup_of_coffee.svg'); two/marker-file: url('http://upload.wikimedia.org/wikipedia/commons/7/72/Cup_of_coffee.svg'); three/marker-file: url('http://upload.wikimedia.org/wikipedia/en/c/ce/SVG-logo.svg'); four/marker-file: url("http://a.tiles.mapbox.com/v3/marker/pin-m-fast-food+AA0000.png"); five/marker-file:url("http://ds.io/octocat-svg"); six/marker-file:url("http://dummyimage.com/16x16/000/fff&text=x"); }
okay, test mod added, please re-open new issue if you have anything remaining.