Bug 16455 - TagsExternalDictionary does not work under Plack
Summary: TagsExternalDictionary does not work under Plack
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 16444
Blocks: 16637
  Show dependency treegraph
 
Reported: 2016-05-05 15:57 UTC by Jonathan Druart
Modified: 2017-12-07 22:16 UTC (History)
7 users (show)

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


Attachments
Bug 16455: Remove the "Too late to run INIT block" from C4::Tags (2.11 KB, patch)
2016-05-05 16:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16455: Remove the "Too late to run INIT block" from C4::Tags (2.17 KB, patch)
2016-06-03 07:55 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 16455: Remove the "Too late to run INIT block" from C4::Tags (2.27 KB, patch)
2016-06-04 12:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16697: Translatability: Fix problem with isolated "'s"in request.tt (3.14 KB, patch)
2016-06-09 13:43 UTC, Marc Véron
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-05-05 15:57:43 UTC
The pref TagsExternalDictionary is used to tell Lingua::Ispell to use an other dictionary, different from the default one (/usr/bin/ispell).

To do so we need to set $Lingua::Ispell::path to the expected path.
It's currently done in the INIT block.

If you try to use C4::Tags, you will get the famous "Too late to run INIT block at C4/Tags.pm line 74." warning. Plack use the INIT block to load functions at run time, when we are using C4::Tags when hitting a pl script, the compilation phase is finished and it's "too late to run INIT block" from C4::Tags.

I do not really know if it has an impact on the behavior of Lingua::Ispell (i.e. is the path redefined?), but I know that this INIT block is not executed when we want.
Comment 1 Jonathan Druart 2016-05-05 16:00:24 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2016-06-02 03:13:29 UTC
Oops... duplicated effort, but for different reasons. See bug 16637. I think adding the PerlDependencies.pm for Lingua::Ispell might be an idea. Otherwise you may get an explosion when you fill it in with the default value /usr/bin/ispell, because Lingua::Ispell isn't guaranteed to be installed.
Comment 3 Jonathan Druart 2016-06-03 07:29:00 UTC
(In reply to M. Tompsett from comment #2)
> Oops... duplicated effort, but for different reasons. See bug 16637. I think
> adding the PerlDependencies.pm for Lingua::Ispell might be an idea.
> Otherwise you may get an explosion when you fill it in with the default
> value /usr/bin/ispell, because Lingua::Ispell isn't guaranteed to be
> installed.

I'd prefer to keep this one (especially because I have already submitted other patches using the "does not work under plack" pattern in commit msg). And maybe create another one for the missing deps.
Comment 4 Chris Cormack 2016-06-03 07:55:57 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2016-06-04 06:56:33 UTC
QA: Claiming this one too now
Comment 6 Marcel de Rooy 2016-06-04 12:46:34 UTC
Created attachment 52051 [details] [review]
Bug 16455: Remove the "Too late to run INIT block" from C4::Tags

The pref TagsExternalDictionary is used to tell Lingua::Ispell to use an
other dictionary, different from the default one (/usr/bin/ispell).

To do so we need to set $Lingua::Ispell::path to the expected path.
It's currently done in the INIT block.

If you try to use C4::Tags, you will get the famous "Too late to run
INIT block at C4/Tags.pm line 74." warning. Plack use the INIT block to
load functions at run time, when we are using C4::Tags when hitting a pl
script, the compilation phase is finished and it's "too late to run INIT
block" from C4::Tags.

I do not really know if it has an impact on the behavior of
Lingua::Ispell (i.e. is the path redefined?), but I know that this INIT
block is not executed when we want.

Test plan:
under Plack,
- hit /cgi-bin/koha/opac-search.pl and confirm that the warning does no
longer appears
- Use another dictionnary (??), fill TagsExternalDictionary with its
  path and confirm that it is used by the tags approval system

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Mark Tompsett 2016-06-04 15:07:42 UTC
Comment on attachment 52051 [details] [review]
Bug 16455: Remove the "Too late to run INIT block" from C4::Tags

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

::: C4/Tags.pm
@@ +57,4 @@
>  	}
>  	if ($ext_dict) {
>  		require Lingua::Ispell;
> +        import Lingua::Ispell qw(spellcheck add_word_lc);

Why is save_dictionary missing?
Comment 8 Marcel de Rooy 2016-06-04 15:09:49 UTC
(In reply to M. Tompsett from comment #7)
> Comment on attachment 52051 [details] [review] [review]
> Bug 16455: Remove the "Too late to run INIT block" from C4::Tags
> 
> Review of attachment 52051 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Tags.pm
> @@ +57,4 @@
> >  	}
> >  	if ($ext_dict) {
> >  		require Lingua::Ispell;
> > +        import Lingua::Ispell qw(spellcheck add_word_lc);
> 
> Why is save_dictionary missing?

Is not used. At least I looked for it..
Comment 9 Mark Tompsett 2016-06-04 15:43:55 UTC
(In reply to Marcel de Rooy from comment #8)
> (In reply to M. Tompsett from comment #7)
> > Why is save_dictionary missing?
> Is not used. At least I looked for it..

Okay. I went hunting further, and agree it isn't used. Just made applying the patch difficult. So I worked on 16637 without this, since there is no overlap in files.
Comment 10 Brendan Gallagher 2016-06-06 17:36:22 UTC
Pushed to Master - Should be in the November 2016 Release.  Thanks
Comment 11 Marc Véron 2016-06-09 13:43:45 UTC Comment hidden (obsolete)
Comment 12 Marc Véron 2016-06-09 13:45:32 UTC
Comment on attachment 52193 [details] [review]
Bug 16697: Translatability: Fix problem with isolated "'s"in request.tt

Sorry for the wrong attachment, I was distracted and mistaked.
Comment 13 Frédéric Demians 2016-06-15 07:40:05 UTC
Pushed in 16.05. Will be in 16.05.01.
Comment 14 Julian Maurice 2016-06-16 08:31:03 UTC
Patch pushed to 3.22.x, will be in 3.22.8