Bug 10074 - Encoding problems for login and installer
Summary: Encoding problems for login and installer
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on: 6554
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-18 14:19 UTC by Tomás Cohen Arazi
Modified: 2014-12-07 20:02 UTC (History)
2 users (show)

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


Attachments
Bug 10074 - Revert small change in C4/Templates.pm (1.09 KB, patch)
2013-04-18 14:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 10074 - Encoding problems in templates (1.55 KB, patch)
2013-04-18 18:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10074 - Encoding problems in templates (1.68 KB, patch)
2013-04-18 18:33 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 10074 - Encoding problems in templates (1.62 KB, patch)
2013-04-18 19:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 10074 - Encoding problems in templates (installer/InstallAuth.pm) (1.16 KB, patch)
2013-04-18 19:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 10074 - Encoding problems in templates (installer/InstallAuth.pm) (1.29 KB, patch)
2013-04-18 19:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10074 - Encoding problems in templates (1.70 KB, patch)
2013-04-18 21:08 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10074 - Encoding problems in templates (installer/InstallAuth.pm) (1.37 KB, patch)
2013-04-18 21:08 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 10074 - Encoding problems in templates (1.62 KB, patch)
2013-04-19 12:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10074 - Encoding problems in templates (1.69 KB, patch)
2013-04-19 13:35 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 10074 - Encoding problems in templates (installer/InstallAuth.pm) (1.45 KB, patch)
2013-04-19 13:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10074 - Encoding problems at login time (1.69 KB, patch)
2013-04-19 13:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10074 - Encoding problems for installer (1.34 KB, patch)
2013-04-19 13:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10074: QA Followup for InstallAuth.pm (1.02 KB, patch)
2013-04-19 13:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2013-04-18 14:19:37 UTC
We need to revert small change in C4/Templates.pm
Comment 1 Tomás Cohen Arazi 2013-04-18 14:20:22 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2013-04-18 14:22:21 UTC
To test:
- Install the es-ES translations
- Load the login page on the staff site
- "Contraseña" (for password) looks broken
- Apply the patch and reload
- The word in not broken anymore.
Comment 3 Tomás Cohen Arazi 2013-04-18 18:12:26 UTC Comment hidden (obsolete)
Comment 4 Bernardo Gonzalez Kriegel 2013-04-18 18:33:15 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2013-04-18 19:24:33 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2013-04-18 19:26:01 UTC
After talking with Jared on IRC, we concluded this was the best short-term solution. On the mid/long-term we should move output somewhere else.
Comment 7 Tomás Cohen Arazi 2013-04-18 19:28:39 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2013-04-18 19:34:52 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2013-04-18 20:11:33 UTC
This looks good to me. Will try to sign it off if no one else beats me to it. Note that the problem actually is related to a print statement while stdout is not set to utf-8. I corrected a similar situation somewhere in Koha with a use open statement to utf-8.
Comment 10 Tomás Cohen Arazi 2013-04-18 21:03:05 UTC
(In reply to comment #9)
> This looks good to me. Will try to sign it off if no one else beats me to
> it. Note that the problem actually is related to a print statement while
> stdout is not set to utf-8. I corrected a similar situation somewhere in
> Koha with a use open statement to utf-8.

That's correct. The only lib that should be printing to stdout should be C4::Output anyway.
Comment 11 Bernardo Gonzalez Kriegel 2013-04-18 21:08:36 UTC Comment hidden (obsolete)
Comment 12 Bernardo Gonzalez Kriegel 2013-04-18 21:08:43 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2013-04-19 09:32:15 UTC
Tomas,
I am wondering if you should "Use" the Output module in Auth.pm.
You only need the module if the login fails.
So you could require it at that moment in run-time (without the additional import when using a file).
The use statement will always require Output at compile-time. So in terms of performance it would be better to require. 

In other words, what about this in C4/Auth.pm:

    require C4::Output;
    C4::Output::output_html_with_http_headers( $query, $cookie, $template->output);

What do you think?
Comment 14 Tomás Cohen Arazi 2013-04-19 12:38:34 UTC Comment hidden (obsolete)
Comment 15 Bernardo Gonzalez Kriegel 2013-04-19 13:35:10 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2013-04-19 13:38:49 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2013-04-19 13:41:48 UTC
Comment on attachment 17546 [details] [review]
Bug 10074 - Encoding problems in templates (installer/InstallAuth.pm)

Trouble with utf8 on git itself..
Comment 18 Marcel de Rooy 2013-04-19 13:44:03 UTC
Created attachment 17548 [details] [review]
Bug 10074 - Encoding problems at login time

Bug 6554 patched output_html_with_http_headers to encode utf8 data, and Templates.pm to expect utf8 data to be encoded.
(At least) the staff login screen outputs directly to STDOUT (Auth.pm does, WHICH IS WRONG!) and wasn't fixed to do the encoding first.

This patch makes it use output_html_with_http_headers and solves the problem.

Changed 'use' for 'require' as jcamins and marcelr suggested.

Regards
To+

Sponsored-by: Universidad Nacional de Cordoba
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2013-04-19 13:47:16 UTC
Created attachment 17549 [details] [review]
Bug 10074 - Encoding problems for installer

Bug 6554 patched output_html_with_http_headers to encode utf8 data, and Templates.pm to expect utf8 data to be encoded.
The Installer page login screen outputs directly to STDOUT (InstallAuthAuth.pm does, WHICH IS WRONG!) and wasn't fixed to do the encoding first.

This patch makes it use output_html_with_http_headers and solves the problem.

Regards
To+

Sponsored-by: Universidad Nacional de Cordoba
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2013-04-19 13:47:59 UTC
Created attachment 17550 [details] [review]
Bug 10074: QA Followup for InstallAuth.pm

Moving use Output.pm to optional require.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2013-04-19 13:56:14 UTC
QA Comment:
Good catch, Tomas. 
Moved your second use to a require too in InstallAuth.pm.
Improving code by using output routine in module instead of printing directly to stdout without taking care of proper encoding.
This does not revert the change in Templates.pm btw, but corrects encoding for login and installer template. Note that a separate encode statement could have done the job too, but this is even better.

Passed QA
Comment 22 Marcel de Rooy 2013-04-19 13:58:34 UTC
Tomas: I had problems with Córdoba. Has to do with the git version.
Comment 23 Jared Camins-Esakov 2013-04-20 03:07:51 UTC
This patch has been pushed to master and 3.12.x.
Comment 24 Marcel de Rooy 2013-05-06 07:37:03 UTC
Reverted. Closing this report.