344493 - Due to the name of the extension and the way uploaded image files are generated, no images appear

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, defect)

Description

[tnarik]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

I just added some preview images to my Ook? Video Ook! extension but no image appear and I sometimes find an error such as "/ook.php not found"
I guess it has to do with the name of the extension, which is used as part of the URL and generates an error as "?" is part of the name.

Reproducible: Always

Steps to Reproduce:
1. visit https://addons.mozilla.org/firefox/2584/
2. check the URL of the image that doesn't appear

or (maybe)
1. create an extension with a name with strange characters such as "?"
2. upload preview images
3. no images appear and the URLs are not very "canonical"

Actual Results:  
no image appeared

Expected Results:  
preview images appearing

Comment 1

[Cameron]

<img src="/images/previews/ook?_video_ook!-2.jpg" ... >

Ugly.

Comment 2

[tnarik]

If I could get the code maybe I could help solving this bug.

Comment 3

[Cameron]

(In reply to comment #2)
> If I could get the code maybe I could help solving this bug.
> 
http://lxr.mozilla.org/mozilla/source/webtools/update/developers/previews.php
http://developer.mozilla.org/en/docs/Mozilla_Source_Code_Via_CVS

Comment 4

[tnarik]

Thank you. Just after writing I looked and found it.

It seems the file is created (https://addons.mozilla.org/images/previews/ook%3F_video_ook!-2.jpg), so the problem is only on the generation of the URLs, which should be URLencoded everywhere to prevent this behaviour.
Another alternative is to modify previews.php to replace more characters and not only "_", as in line 180.

Right now I don't have CVS access, but I will try to check out all the code and produce a patch for the first alternative (which I find less disruptive).

Comment 5

[tnarik]

Attachment: temptative patch for this bug.

Hope this resolves this problem.

Comment 6

[Cameron]

(In reply to comment #5)
> Created an attachment (id=230454) [edit]
> temptative patch for this bug.
> 
> Hope this resolves this problem.
> 

My apologies - the code for the public part of the site is in /webtools/addons/
It's only the developer part of the site that's in /webtools/update/

As such, patching /update/index.php, /update/extensions/moreinfo.php, /update/extensions/showlist.php etc. won't work.

Comment 7

[tnarik]

Oh, I see.
Anyway, the developer interface do also have this problem, so this patch (or something similar) should (or could, because I can live with that although I will prefer not to) be applied to fix that part of the website (the management of previews, ...) .

Comment 8

[tnarik]

Attachment: Proposed patch to fix this bug for the public (not logged in) website

I hope both patches will solve this issue.

Comment 9

[tnarik]

How can I help more solving this bug?

Comment 10

[Wil Clouser [:clouserw]]

Attachment: proposed patch to move encoding into object

Since I'm tired, here is the IRC log:

23:46 <@clouserw> it looks like the preview paths are stored in the db as /images/previews/xxxx.yyy
23:46 <@clouserw> so his patch is urlencoding the slashes too, which breaks things
23:46 <@clouserw> it's an easy fix (and I already did it) to just urlencode() the filename and not the two directories before hand
23:47 <@clouserw> and that doesn't effect anything else in the code at this point
23:47 <@Cameron> rightyo
23:47 <@clouserw> but it might become one of those "wtf were we thinking" things later on, when a variable called "PreviewURI" contains a string that is half-urlencoded
23:48 <@clouserw> and really, is url encoding a job of the templates?
23:48 <@clouserw> which means we shouldn't urlencode in the object
23:49 <@clouserw> anyway, I'm too tired to think it all the way through right now :)
23:49 <@Cameron> "wtf were we thinking" sums up a lot of things these days heh :)
23:49 <@clouserw> well, I'm trying to avoid another day like that ;)
23:49 <@Cameron> :)

Summary:

The db is storing more than just the filename as PreviewURI.  This patch encodes just the filename part of the string.  I don't like having a half-encoded string, but it doesn't hurt any current code, and running urldecode() on it will result in the same thing whether it is half or fully encoded.

Comment 11

[tnarik]

Somehow I see data and the presentation of data as different issues. for me, the PreviewURI data on the DB shouldn't be URLencoded (even less in such an hybrid mode) and those data should be made visible and useful where needed (therefor the urlencoding on templates, where maybe you could just print them if you wanted).

For me, the cleaner way of fixing this issue would be storing just the filename on the DB, urlencode() it on the templates and adding there the /images/previews path. But that means a lot of changes and maybe "/images/previews/" is not always the path used. And the DB should be modified.

Anyway, that's just my view, but I now it's hard and boring coding that.

Comment 12

[Wil Clouser [:clouserw]]

Well, the it sounds like the DB is going to be up for a redesign pretty shortly anyway (l10n style), so in an effort to just get stuff done, I'm committing my patch to CVS.  

This should be in effect at the next update.