Bug 11848 - Make Koha::I18N easier to use
Summary: Make Koha::I18N easier to use
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
Depends on: 8044
Blocks: 11668 11904
  Show dependency treegraph
 
Reported: 2014-02-26 15:53 UTC by Julian Maurice
Modified: 2014-12-07 20:07 UTC (History)
4 users (show)

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


Attachments
Bug 11848: Move language detection function in C4::Languages (9.25 KB, patch)
2014-02-26 15:54 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11848: Make Koha::I18N easier to use (2.60 KB, patch)
2014-02-26 15:54 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11848: Fix C4::Context::interface, add POD and UT (2.36 KB, patch)
2014-03-17 22:29 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11848: Move language detection function in C4::Languages (9.35 KB, patch)
2014-03-20 20:58 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11848: Make Koha::I18N easier to use (2.69 KB, patch)
2014-03-20 20:58 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11848: Fix C4::Context::interface, add POD and UT (2.55 KB, patch)
2014-03-20 20:58 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PASSED QA] Bug 11848: Move language detection function in C4::Languages (9.43 KB, patch)
2014-04-28 21:24 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11848: Make Koha::I18N easier to use (2.74 KB, patch)
2014-04-28 21:24 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11848: Fix C4::Context::interface, add POD and UT (3.68 KB, patch)
2014-04-28 21:24 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2014-02-26 15:53:48 UTC
Actually (with bug 8044), to translate a string in a Perl script or module you have to write:

    use CGI;
    use Koha::I18N;
    my $cgi = new CGI;
    my $lh = Koha::I18N->get_handle_from_context($cgi, 'intranet');
    print $lh->maketext('my translatable text');

With the patches I'll submit, this will become:

    use Koha::I18N;
    print gettext('my translatable text');
Comment 1 Julian Maurice 2014-02-26 15:54:50 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2014-02-26 15:54:54 UTC Comment hidden (obsolete)
Comment 3 Julian Maurice 2014-02-26 16:05:18 UTC
Test plan (for developpers):

1/ Edit a Perl script, for example mainpage.pl
2/ add "use Koha::I18N;" to the top of file
3/ add a translatable message somewhere in the script (this have to be after the call to get_template_and_user). For example:
   warn gettext("This is a translated warning");
4/ Create or update the PO files with
   misc/translator/translate create LANGCODE
or
   misc/translator/translate update LANGCODE
(LANGCODE should be enable in syspref 'languages')
5/ In misc/translator/po/LANGCODE-messages.po you should have your string, translate it (using a text editor or a PO file editor, make sure you don't have the "fuzzy" flag for this string).
6/ Go to mainpage.pl with active language being English with your browser and check your logs. You should see your string "This is a translated warning".
7/ Now change language to LANGCODE. Check your logs, you should have the string translated.

Note: I chose to name the sub 'gettext' because it's the default keyword for xgettext for Perl. We can change it to whatever we want.
Comment 4 Julian Maurice 2014-02-26 16:07:47 UTC
Note 2: The purpose of Koha::I18N is *not* to translate warning messages (warnings should not be in the language of the person actually using Koha), but it's the quickest way to test this (I think)
Comment 5 Mark Tompsett 2014-03-14 18:40:18 UTC
Comment on attachment 25664 [details] [review]
Bug 11848: Move language detection function in C4::Languages

Review of attachment 25664 [details] [review]:
-----------------------------------------------------------------

::: C4/Context.pm
@@ +1252,5 @@
>  
> +sub interface {
> +    my ($class, $interface) = @_;
> +
> +    if (defined $interface) {

You are using interface as both a get and set, correct?

@@ +1253,5 @@
> +sub interface {
> +    my ($class, $interface) = @_;
> +
> +    if (defined $interface) {
> +        $interface ||= 'opac';

Why not // instead?
Also, interface is defined. This line is pointless.

@@ +1254,5 @@
> +    my ($class, $interface) = @_;
> +
> +    if (defined $interface) {
> +        $interface ||= 'opac';
> +        $context->{interface} = $interface;

perhaps a lc or uc may be useful? maybe some validation of the value?

@@ +1257,5 @@
> +        $interface ||= 'opac';
> +        $context->{interface} = $interface;
> +    }
> +
> +    return $context->{interface};

What if you haven't set the interface and you get undef? It would seem the default assumption is 'opac'. Perhaps add a "// 'opac'"?
Comment 6 Mark Tompsett 2014-03-15 02:19:57 UTC
Comment on attachment 25664 [details] [review]
Bug 11848: Move language detection function in C4::Languages

Review of attachment 25664 [details] [review]:
-----------------------------------------------------------------

::: t/Templates.t
@@ +42,4 @@
>    },
>  );
>  
> +delete $ENV{HTTP_ACCEPT_LANGUAGE};

Good catch on this typo that has been there since 2013-07-12 14:57:11.
Comment 7 Julian Maurice 2014-03-17 09:13:07 UTC
> You are using interface as both a get and set, correct?
Correct!

> Why not // instead?
> Also, interface is defined. This line is pointless.
C4::Context->interface # getter only
C4::Context->interface('') # getter and setter, set to opac because '' is a defined but false value

> perhaps a lc or uc may be useful? maybe some validation of the value?
> What if you haven't set the interface and you get undef? It would seem the
> default assumption is 'opac'. Perhaps add a "// 'opac'"?
Sure. I will provide a patch for these issues. But not immediately, maybe next week.

Thanks for the review! :)
Comment 8 Julian Maurice 2014-03-17 22:29:26 UTC Comment hidden (obsolete)
Comment 9 Julian Maurice 2014-03-17 22:31:41 UTC
Finally, earlier than expected ;)
Comment 10 Bernardo Gonzalez Kriegel 2014-03-20 20:58:15 UTC Comment hidden (obsolete)
Comment 11 Bernardo Gonzalez Kriegel 2014-03-20 20:58:28 UTC Comment hidden (obsolete)
Comment 12 Bernardo Gonzalez Kriegel 2014-03-20 20:58:39 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2014-04-28 21:24:36 UTC
Created attachment 27747 [details] [review]
[PASSED QA] Bug 11848: Move language detection function in C4::Languages

and store interface (intranet, opac) in context to not have to pass it
as parameter.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
No koha-qa errors

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comments on last patch.
Comment 14 Katrin Fischer 2014-04-28 21:24:39 UTC
Created attachment 27748 [details] [review]
[PASSED QA] Bug 11848: Make Koha::I18N easier to use

Instead of writing

  use CGI;
  use Koha::I18N;
  my $cgi = new CGI;
  my $lh = Koha::I18N->get_handle_from_context($cgi, 'intranet');
  print $lh->maketext('my translatable text');

you can now write

  use Koha::I18N;
  print gettext('my translatable text');

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
No errors

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 15 Katrin Fischer 2014-04-28 21:24:42 UTC
Created attachment 27749 [details] [review]
[PASSED QA] Bug 11848: Fix C4::Context::interface, add POD and UT

1/ Edit a Perl script, for example mainpage.pl
2/ add "use Koha::I18N;" to the top of file
3/ add a translatable message somewhere in the script (this have
   to be after the call to get_template_and_user). For example:
   warn gettext("This is a translated warning");
4/ Create or update the PO files with
   misc/translator/translate create LANGCODE
or
   misc/translator/translate update LANGCODE
   (LANGCODE should be enable in syspref 'languages')
5/ In misc/translator/po/LANGCODE-messages.po you should have
   your string, translate it (using a text editor or a PO file
   editor, make sure you don't have the "fuzzy" flag for this
   string).
6/ Go to mainpage.pl with active language being English with your
   browser and check your logs. You should see your string "This
   is a translated warning".
7/ Now change language to LANGCODE. Check your logs, you should
   have the string translated.

Note: I chose to name the sub 'gettext' because it's the default
keyword for xgettext for Perl. We can change it to whatever we want.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Follow test plan, work as described.
No koha-qa errors.
Tests pass

Fixed small merge conflict on t/Context.t

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.
Copied test plan from bug.
Comment 16 Galen Charlton 2014-05-05 04:56:56 UTC
Pushed to master.  Thanks, Julian!