Bug 21377 - Variable declarations erroneously filtered
Summary: Variable declarations erroneously filtered
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P2 critical (vote)
Assignee: Andrew Isherwood
QA Contact: Julian Maurice
URL:
Keywords:
Depends on: 7317 13618
Blocks:
  Show dependency treegraph
 
Reported: 2018-09-19 08:28 UTC by Andrew Isherwood
Modified: 2019-10-14 19:56 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21377: Remove erroneous "html" filter use (1.87 KB, patch)
2018-09-19 08:37 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21377: (follow-up) Add raw filter (1.53 KB, patch)
2018-09-26 09:14 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21377: (follow-up) Re-remove raw filter (2.75 KB, patch)
2018-09-26 09:34 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21377: Remove erroneous "html" filter use (1.92 KB, patch)
2018-09-26 16:35 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21377: (follow-up) Add raw filter (1.58 KB, patch)
2018-09-26 16:35 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21377: (follow-up) Re-remove raw filter (2.80 KB, patch)
2018-09-26 16:35 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21377: Remove erroneous "html" filter use (3.16 KB, patch)
2018-09-27 13:55 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2018-09-19 08:28:58 UTC
In the main ILL template file ill-requests.tt a few variable declarations that are not strings have the "html" filter applied to them, this breaks them. These filters should be removed.
Comment 1 Andrew Isherwood 2018-09-19 08:37:42 UTC
Created attachment 79079 [details] [review]
Bug 21377: Remove erroneous "html" filter use

The "html" filter that is being applied to these variable declarations
is inappropriate and has been removed.

To test:

- Before applying the patch, on master, view the "Manage request" page
for an ILL request
- TEST: Observe that, apart from the "Edit request" button, the various
available action buttons do not display correctly
- Apply the patch
- TEST: Refresh the page and observe that the buttons now display
correctly
Comment 2 Andrew Isherwood 2018-09-26 08:08:48 UTC
Upping the priority of this as it impacts every ILL bug
Comment 3 Andrew Isherwood 2018-09-26 08:09:31 UTC
...including master
Comment 4 Julian Maurice 2018-09-26 08:48:07 UTC
If variables should not be escaped, you have to be explicit about it by using the $raw filter:

https://wiki.koha-community.org/wiki/Coding_Guidelines#HTML9:_filter_all_the_variables
Comment 5 Andrew Isherwood 2018-09-26 09:14:49 UTC
Created attachment 79401 [details] [review]
Bug 21377: (follow-up) Add raw filter

Rather than just removing the html filter, it should be replaced with
the raw filter
Comment 6 Andrew Isherwood 2018-09-26 09:15:49 UTC
Thanks for the pointer Julian, I'd completely missed the purpose of the raw filter, thanks for flagging it up!
Comment 7 Andrew Isherwood 2018-09-26 09:25:54 UTC
Actually, I've just noticed that adding the raw filter breaks it (stupid browser caching when I tested it). This suggests the raw filter doesn't just pass the variable straight through and somehow breaks variables that aren't just strings. I'll do some digging...
Comment 8 Andrew Isherwood 2018-09-26 09:29:01 UTC
I'm not sure applying any filter is appropriate. I'm just assigning a variable based on another variable, there's no displaying going on. So, actually looking at what I'm doing, I'm not surprised it breaks it!
Comment 9 Andrew Isherwood 2018-09-26 09:34:23 UTC
Created attachment 79402 [details] [review]
Bug 21377: (follow-up) Re-remove raw filter

This patch removes the raw filter that was erroneously applied, these
are variable declarations, they don't need filtering.

I've also simplified things by removing two of the extraneous variable
declarations.
Comment 10 Julian Maurice 2018-09-26 09:47:17 UTC
(In reply to Andrew Isherwood from comment #7)
> Actually, I've just noticed that adding the raw filter breaks it (stupid
> browser caching when I tested it). This suggests the raw filter doesn't just
> pass the variable straight through and somehow breaks variables that aren't
> just strings. I'll do some digging...

That's odd. The raw filter should return exactly what it has been given. But as theses variables are not directly displayed, it should be ok without filters. Sorry for the extra work
Comment 11 Andrew Isherwood 2018-09-26 09:55:49 UTC
No worries :) It actually ended up with me simplifying things a little bit, so was worthwhile!
Comment 12 Owen Leonard 2018-09-26 16:35:16 UTC
Created attachment 79437 [details] [review]
Bug 21377: Remove erroneous "html" filter use

The "html" filter that is being applied to these variable declarations
is inappropriate and has been removed.

To test:

- Before applying the patch, on master, view the "Manage request" page
for an ILL request
- TEST: Observe that, apart from the "Edit request" button, the various
available action buttons do not display correctly
- Apply the patch
- TEST: Refresh the page and observe that the buttons now display
correctly

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 13 Owen Leonard 2018-09-26 16:35:20 UTC
Created attachment 79438 [details] [review]
Bug 21377: (follow-up) Add raw filter

Rather than just removing the html filter, it should be replaced with
the raw filter

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 14 Owen Leonard 2018-09-26 16:35:23 UTC
Created attachment 79439 [details] [review]
Bug 21377: (follow-up) Re-remove raw filter

This patch removes the raw filter that was erroneously applied, these
are variable declarations, they don't need filtering.

I've also simplified things by removing two of the extraneous variable
declarations.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 15 Julian Maurice 2018-09-27 13:55:08 UTC
Created attachment 79512 [details] [review]
Bug 21377: Remove erroneous "html" filter use

The "html" filter that is being applied to these variable declarations
is inappropriate and has been removed.
I've also simplified things by removing two of the extraneous variable
declarations.

To test:

- Before applying the patch, on master, view the "Manage request" page
for an ILL request
- TEST: Observe that, apart from the "Edit request" button, the various
available action buttons do not display correctly
- Apply the patch
- TEST: Refresh the page and observe that the buttons now display
correctly

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 16 Julian Maurice 2018-09-27 13:56:32 UTC
Squashed all patches into one, to make it easier to read
Comment 17 Nick Clemens 2018-09-28 19:53:28 UTC
Awesome work all!

Pushed to master for 18.11
Comment 18 Martin Renvoize 2018-10-03 09:13:15 UTC
Dependencies/Cause not in 18.05.x series.