| Summary: | Avoid encoding issues in plugins by providing helper methods to output headers correctly | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Magnus Enger <magnus> |
| Component: | Staff interface | Assignee: | Tomás Cohen Arazi (tcohen) <tomascohen> |
| Status: | CLOSED FIXED | QA Contact: | Kyle M Hall (khall) <kyle> |
| Severity: | major | ||
| Priority: | P5 - low | CC: | contact, fridolin.somers, gmcharlt, jonathan.druart, kyle, nick, tomascohen |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| 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
Bug 19223: Unit tests for output and output_html methods Bug 19223: Add methods to correctly handle plugin-generated output Bug 19223: Unit tests for output and output_html methods Bug 19223: Add methods to correctly handle plugin-generated output Bug 19223: Unit tests for output and output_html methods Bug 19223: Add methods to correctly handle plugin-generated output |
||
|
Description
Magnus Enger
2017-08-31 13:26:48 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.
Still a problem, with Koha 17.05.06.000 and version 17.11 of the EDS plugin. Moving forward with plugins, I think we need to make sure that encodings are correctly handled. Upping severity. (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? (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... 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? (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. 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. 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. Created attachment 73348 [details] [review] Bug 19223: Unit tests for output and output_html methods 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 Hi Tomas, can you submit a pull request to the kitchen sink plugin to make this easy to test? 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> 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> 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> 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> Pushed to master for 18.05, thanks to everybody involved! Pushed to stable for 17.11.06 Awesome work all! Is this for 17.05.x also ? t/db_dependent/Plugins.t is actually OK |