Image macro does not understand the port in URL protocol

[anonymous]

That url works ​http://192.168.200.202/icq/76576576576

That does not ​http://192.168.200.202:8080/icq/76576576576

and returns the following error Error: Macro Image(​http://192.168.200.202:8080/icq/9879879798) failed Attachment '​http://192.168.200.202:8080/icq/9879879798' does not exist.

[hyuga <hyugaricdeau@…>]

This issue still exists in 0.11b1. The problem begins at line 366​:

366             parts = filespec.split(':')
367             url = raw_url = None
368             attachment = None
369             if len(parts) == 3:                 # realm:id:attachment-filename
370                 realm, id, filename = parts
371                 attachment = Resource(realm, id).child('attachment', filename)

A URL containing a port number is split into three parts, and Trac parses this as a realm, id, and filename.

[osimons]

Patch that makes image macro handle external urls better regardless of form.

[osimons]

Reviewing the current Image macro code, there are many pitfalls with current way of doing things. Even assuming that there will not be more than max. 3 parts (with port) is not enough. I looked at the new Google Chart api, and they even use ':' inside the query string. The macro just sees the full string, and will have no idea about this.

To make this work consistently regardless of how the url looks, I had to lift it out into its own top-level comparison. Patch for this is attached​.

It also adds a server-relative alternative for externals, so that one can write '/images/mypicture.jpg'. This works better for servers that have both http and https access for same content.

Does it work as expected for various urls? Please review.

[Christian Boos]

Looks good, +1

Also, it would be worth taking this opportunity for extending the [[Image]] unit-tests (source:/trunk/trac/wiki/tests/macros.py​).

[osimons]

Replying to cboos:

Also, it would be worth taking this opportunity for extending the [[Image]] unit-tests (source:/trunk/trac/wiki/tests/macros.py​).

Had a look at them to see how they work, and even when changing existing tests to plainly wrong results I can't get any failures from running them. Do we need some sort of Image macro dummy stub as well? What am I missing?

[Christian Boos]

I don't know… it works fine for me, e.g.

trac/wiki/tests/macros.py

@@ -5 +5 @@
 
 IMAGE_MACRO_TEST_CASES=u"""
 ============================== source: Image, no other arguments
-[[Image(source:test.png)]]
+[[Image(sourc:test.png)]]
 ------------------------------
 <p>
 <a style="padding:0; border:none" href="/browser/test.png"><img src="/browser/test.png?format=raw" alt="source:test.png" title="source:test.png" /></a>
@@ -13 +13 @@
 ------------------------------
 [[Image(...)]]
 ============================== source: Image, nolink
-[[Image(source:test.png, nolink)]]
+[[Image(source:text.png, nolink)]]
 ------------------------------
 <p>
 <img src="/browser/test.png?format=raw" alt="source:test.png" title="source:test.png" />

results in:

$ python setup.py test -s trac.wiki.tests.macros.suite
running test
...
Test source: Image, no other arguments ... FAIL
Test source: Image, no other arguments ... ok
Test source: Image, nolink ... FAIL
Test source: Image, normal args ... ok
Test source: Image, size arg ... ok
Test source: Image, keyword alignment ... ok
Test http: Image, nolink ... ok

======================================================================
FAIL: Test source: Image, no other arguments
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\workspace\src\trac\repos\trunk\trac\wiki\tests\formatter.py", line 155, in test
    raise AssertionError( # See below for details
AssertionError:
--------------- expected:
...
--------------- actual:
...

etc.

[osimons]

Adding some tests for new functionality - and a fix for a markup issue it revealed.

[osimons]

Right, got the tests added and working. My mistake as usual… New patch attached.

Anyway, the tests then revealed an isse with the href="" escaping the Google test url I use so that & showed as $amp;.

Chatted with coderanger, and this is the solution we ended up with (I think). Basically remove any double-quotes that can terminate the attribute, and then just mark it as safe. title= and alt= is still escaped, of course.

Is this OK? Any better ways of doing it?

[osimons]

Fixed in [6413] - also adding server- and project-relative linking to input alternatives.

(Turns out I was over-complicating the issue of quoting and escaping - html escaping of all content seems fine.)