Bug 6052 - Misnamed variable in templates/article/article.tpl
Misnamed variable in templates/article/article.tpl
Status: RESOLVED FIXED
Product: OJS
Classification: Unclassified
Component: Reading Tools
2.3.3
PC Mac OS X 10.6
: P5 normal
Assigned To: Matthew Crider
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-18 17:04 PDT by Matthew Crider
Modified: 2010-10-29 17:46 PDT (History)
3 users (show)

See Also:
Version Reported In:
Also Affects:


Attachments
Patch against OJS 2.3.3-2 (906 bytes, patch)
2010-10-25 12:10 PDT, Alec Smecher
Details | Diff
Patch against OJS 2.3.3-2 (914 bytes, patch)
2010-10-25 17:59 PDT, Matthew Crider
Details | Diff
Patch against OJS 2.3.3-2 (3.90 KB, patch)
2010-10-26 10:36 PDT, Matthew Crider
Details | Diff
Patch against PKP-lib HEAD (1.08 KB, patch)
2010-10-26 10:37 PDT, Matthew Crider
Details | Diff
Combined Patch against OJS pre-2.3.3-2 (3.90 KB, patch)
2010-10-26 12:02 PDT, Matthew Crider
Details | Diff
Combined Patch against OJS pre-2.3.3-2 (3.92 KB, patch)
2010-10-26 13:04 PDT, Matthew Crider
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Crider 2010-10-18 17:04:22 PDT
There is a variable called $noPluginKey in templates/article/article.tpl; It should be $noPluginText.
Comment 2 Alec Smecher 2010-10-25 12:10:30 PDT
Created attachment 3292 [details]
Patch against OJS 2.3.3-2

(Uploaded patch is equivalent to Matt's link in comment #1.)
Comment 3 tdthom 2010-10-25 16:34:21 PDT
The PDF fails to load for me in Firefox now with this change (on Windows 7). Reverting it back to $noPluginKey works. But neither work in IE8.
Comment 4 Matthew Crider 2010-10-25 17:59:05 PDT
Created attachment 3300 [details]
Patch against OJS 2.3.3-2

The noPluginText variable wasn't properly escaped--This should be what was causing the display issue.  tdthom, are you sure you have the acrobat plugin installed for internet explorer?
Comment 5 tdthom 2010-10-25 18:35:48 PDT
Thanks, that works. I am not sure what is going on with internet explorer - it does load pdfs from other sites in the browser window. If it helps IE8 reports this error when the page loads:
...
Message: Invalid argument.
Line: 116
Char: 165
Code: 0
URI: http://ajax.googleapis.com/ajax/libs/jquery/1.4.2/jquery.min.js
Comment 6 Alec Smecher 2010-10-26 09:24:59 PDT
Matt, you should use {...|escape:"javascript"} when escaping values that are dumped into Javascript variables; also, the same bug exists elsewhere on the page. Use the W3C validator.
Comment 7 Matthew Crider 2010-10-26 10:36:11 PDT
Created attachment 3301 [details]
Patch against OJS 2.3.3-2

Properly escapes all variables in PDF viewer script; Fixes some other W3C validation issues.
http://github.com/pkp/ojs/commit/149adc3cc80c472b1b30fb35da6fdfe3381f5412
Comment 8 Matthew Crider 2010-10-26 10:37:52 PDT
Created attachment 3302 [details]
Patch against PKP-lib HEAD

Some additional Validation warning fixes for the Google CDN script call.

http://github.com/pkp/pkp-lib/commit/2dd0eef871f496a80d3947fe3f7343ee58f4d5d2
Comment 9 Matthew Crider 2010-10-26 10:40:42 PDT
tdthom, I'm able to see the embedded PDFs fine in IE8, but you could very well have run into an issue fixed by these patches.  Could you apply the latest OJS patch and try again?
Comment 10 Alec Smecher 2010-10-26 11:10:54 PDT
For consistency -- and for JS-incapable browsers -- it's best to use "// -->" for the end HTML quote rather than just "-->" (see templates/article/footer.tpl and elsewhere for examples). Could you put together a single patch against OJS 2.3.3-2 that combines these?
Comment 11 Matthew Crider 2010-10-26 12:02:30 PDT
Created attachment 3303 [details]
Combined Patch against OJS pre-2.3.3-2

Combined patch of the above three patches and http://github.com/pkp/ojs/commit/b2d58392aafc6b53979d18478bb74153dc07485c
Comment 12 Alec Smecher 2010-10-26 12:35:00 PDT
There's still some inconsistency in the escaping of the {$pdfUrl} variable in JS in the last patch.
Comment 13 Matthew Crider 2010-10-26 13:04:16 PDT
Created attachment 3304 [details]
Combined Patch against OJS pre-2.3.3-2

Aargh, sorry Alec.
Comment 14 Alec Smecher 2010-10-27 15:00:49 PDT
tdthom, if you get the chance, can you try Matt's most recent patch to see if it corrects the problem?
Comment 15 tdthom 2010-10-27 17:40:06 PDT
Hi, sorry for the delay. I have installed the patches and they work fine in Firefox and IE8 in 32bit mode (but not 64bit, presumably because acrobat reader is 32bit). So yes, thanks for the patches.
Comment 16 Alec Smecher 2010-10-28 10:56:20 PDT
Thanks, tdthom, much appreciated. Matt, I think everything has been committed? If so, feel free to close. I've moved this onto the recommended patch list for OJS 2.3.3-2.
Comment 17 Matthew Crider 2010-10-28 11:00:28 PDT
Yes.  Glad to put this one to rest :)
Comment 18 Alec Smecher 2010-10-29 17:46:49 PDT
Committed to OJS 2.3.3-3