Bug 19223 - Avoid encoding issues in plugins by providing helper methods to output headers correctly
Summary: Avoid encoding issues in plugins by providing helper methods to output header...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Kyle M Hall (khall)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-31 13:26 UTC by Magnus Enger
Modified: 2019-10-14 19:56 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
The current plugin writing practice is to craft the response header in the controller methods. This patchset adds new helper methods for plugin authors to use when dealing with output on their plugins. This way the end-user experience is better, and the plugin author's tasks easier.
Version(s) released in:
Circulation function:

Attachments
Encoding problems around the EDS plugin (56.28 KB, image/png)
2017-10-13 09:35 UTC, Magnus Enger 		Details
Bug 19223: Unit tests for output and output_html methods (2.25 KB, patch)
2018-03-27 19:01 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 19223: Add methods to correctly handle plugin-generated output (3.03 KB, patch)
2018-03-27 19:01 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 19223: Unit tests for output and output_html methods (2.30 KB, patch)
2018-03-31 09:39 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Bug 19223: Add methods to correctly handle plugin-generated output (3.08 KB, patch)
2018-03-31 09:40 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Bug 19223: Unit tests for output and output_html methods (2.38 KB, patch)
2018-04-03 18:56 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 19223: Add methods to correctly handle plugin-generated output (3.16 KB, patch)
2018-04-03 18:57 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2017-08-31 13:26:48 UTC 
I have created a couple of plugins for Koha: 

https://github.com/Libriotech/koha-plugin-overview which queries the Koha database and reports things like number og biblio, items, patrons etc. 

https://github.com/Libriotech/koha-plugin-btj which receives data from an outside source, saves it to the database, creates records and items and displays some of the data to users. 

The second one does not show any encoding problems, but the first one does. When  I look at /cgi-bin/koha/plugins/run.pl?class=Koha::Plugin::No::Libriotech::Overview&method=report&detail=items and have the language set to Norwegian, there are problems with the output from the plugin as well as the other parts of the page, such as the main menu at the very top of the page. 

I can't see that I have done anything very different in those two plugins, and yet they behave differently. 

Anyone got a clue if this is a problem with my code or with the plugin system?
Comment 1 Magnus Enger 2017-10-13 09:35:28 UTC 
Created attachment 68057 [details]
Encoding problems around the EDS plugin

The problem with encodings also shows up when the EDS API plugin is installed.
Comment 2 Magnus Enger 2018-03-07 12:17:31 UTC 
Still a problem, with Koha 17.05.06.000 and version 17.11 of the EDS plugin.
Comment 3 Katrin Fischer 2018-03-07 13:16:36 UTC 
Moving forward with plugins, I think we need to make sure that encodings are correctly handled. Upping severity.
Comment 4 Kyle M Hall (khall) 2018-03-07 13:27:32 UTC 
(In reply to Katrin Fischer from comment #3)
> Moving forward with plugins, I think we need to make sure that encodings are
> correctly handled. Upping severity.

The crux of the matter is the question of the bug being in Koha's plugin system, or is it a bug in the plugin?
Comment 5 Magnus Enger 2018-03-07 13:43:10 UTC 
(In reply to Kyle M Hall from comment #4)
> The crux of the matter is the question of the bug being in Koha's plugin
> system, or is it a bug in the plugin?

I have seen it with more than one plugin. But those plugins might be based off each other (I know I have copied the KitchenSink plugin as a starter for my own plugins), so they might have the same bug...
Comment 6 Katrin Fischer 2018-03-07 16:36:27 UTC 
Maybe some best practice is needed or a fix, hard to tell - EDS plugin is hard to test, is there another we could take a look at more closely?
Comment 7 Tomás Cohen Arazi (tcohen) 2018-03-16 17:51:40 UTC 
(In reply to Katrin Fischer from comment #6)
> Maybe some best practice is needed or a fix, hard to tell - EDS plugin is
> hard to test, is there another we could take a look at more closely?

When I put 'Tomás' in the author metadata for a plugin, it gets double-encoded in Koha.
Comment 8 Tomás Cohen Arazi (tcohen) 2018-03-27 17:17:03 UTC 
Steps to reproduce:

- Install some translation:
  $ kshell
 k$ cd misc/translator
 k$ perl translate install es-ES
 k$ exit
  $ restart_all
- Enable es-ES in the sysprefs (languages)
- Choose 'Español'
- Upload a plugin (https://github.com/bywatersolutions/koha-plugin-csv2marc/releases)
- Choose to configure the plugin
=> FAIL: Double-encoded chars on the header, everywhere actually.
Comment 9 Tomás Cohen Arazi (tcohen) 2018-03-27 18:12:47 UTC 
Common practice is to have the plugins print their output, but it is not obvious that headers need to be generated correctly, etc. I will provide a patch that adds helper methods, and this needs to be highlighted in the KitchenSink plugin somehow.
Comment 10 Tomás Cohen Arazi (tcohen) 2018-03-27 19:01:28 UTC 
Created attachment 73348 [details] [review]
Bug 19223: Unit tests for output and output_html methods
Comment 11 Tomás Cohen Arazi (tcohen) 2018-03-27 19:01:32 UTC 
Created attachment 73349 [details] [review]
Bug 19223: Add methods to correctly handle plugin-generated output

This patch introduces two methods to be used by plugin authors:

->output
->output_html

They are basically wrappers for the helper methods from C4::Output
(output_html_with_http_headers and output_with_http_headers).

Plugin authors can use them, or keep the current flexibility of handling
the headers themselves in their code.

The KitchenSink plugin should be updated to highlight this.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Plugins.t
=> FAIL: The methods are not implemented
- Apply this patch
- Run:
 k$ prove t/db_dependent/Plugins.t
=> SUCCESS: Tests pass, and they are meaningful
- Sign off :-D

Sponsored-by: ByWater Solutions
Comment 12 Katrin Fischer 2018-03-31 09:36:02 UTC 
Hi Tomas, can you submit a pull request to the kitchen sink plugin to make this easy to test?
Comment 13 Katrin Fischer 2018-03-31 09:39:51 UTC 
Created attachment 73512 [details] [review]
Bug 19223: Unit tests for output and output_html methods

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2018-03-31 09:40:24 UTC 
Created attachment 73513 [details] [review]
Bug 19223: Add methods to correctly handle plugin-generated output

This patch introduces two methods to be used by plugin authors:

->output
->output_html

They are basically wrappers for the helper methods from C4::Output
(output_html_with_http_headers and output_with_http_headers).

Plugin authors can use them, or keep the current flexibility of handling
the headers themselves in their code.

The KitchenSink plugin should be updated to highlight this.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Plugins.t
=> FAIL: The methods are not implemented
- Apply this patch
- Run:
 k$ prove t/db_dependent/Plugins.t
=> SUCCESS: Tests pass, and they are meaningful
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Kyle M Hall (khall) 2018-04-03 18:56:57 UTC 
Created attachment 73589 [details] [review]
Bug 19223: Unit tests for output and output_html methods

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall (khall) 2018-04-03 18:57:04 UTC 
Created attachment 73590 [details] [review]
Bug 19223: Add methods to correctly handle plugin-generated output

This patch introduces two methods to be used by plugin authors:

->output
->output_html

They are basically wrappers for the helper methods from C4::Output
(output_html_with_http_headers and output_with_http_headers).

Plugin authors can use them, or keep the current flexibility of handling
the headers themselves in their code.

The KitchenSink plugin should be updated to highlight this.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Plugins.t
=> FAIL: The methods are not implemented
- Apply this patch
- Run:
 k$ prove t/db_dependent/Plugins.t
=> SUCCESS: Tests pass, and they are meaningful
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Jonathan Druart 2018-04-04 19:07:30 UTC 
Pushed to master for 18.05, thanks to everybody involved!
Comment 19 Nick Clemens (kidclamp) 2018-05-14 21:15:29 UTC 
Pushed to stable for 17.11.06

Awesome work all!
Comment 20 Fridolin Somers 2018-05-22 10:20:46 UTC 
Is this for 17.05.x also ?
t/db_dependent/Plugins.t is actually OK