Bug 16588 - Remove the Readonly dependency (again)
Summary: Remove the Readonly dependency (again)
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Packaging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Whiteboard:
Keywords:
Depends on: 7736 12478 14066
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-25 13:55 UTC by Robin Sheat
Modified: 2018-07-11 05:46 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 16588: Remove Readonly from Edifact (8.37 KB, patch)
2016-07-22 08:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16588: Remove Readonly from SearchEngine (15.30 KB, patch)
2016-07-22 08:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16588: Koha::SearchEngine must return a true value (584 bytes, patch)
2016-07-22 08:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16588: (follow-up) Remove Readonly from Edifact (997 bytes, patch)
2016-08-15 13:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16588: Remove Readonly from Edifact (8.43 KB, patch)
2016-09-29 15:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16588: Remove Readonly from SearchEngine (15.38 KB, patch)
2016-09-29 15:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16588: Koha::SearchEngine must return a true value (640 bytes, patch)
2016-09-29 15:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16588: (follow-up) Remove Readonly from Edifact (1.03 KB, patch)
2016-09-29 15:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Amended Patch (1.73 KB, patch)
2016-10-26 09:59 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 16588: Remove Readonly from SearchEngine (15.52 KB, patch)
2016-11-16 07:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16588: Koha::SearchEngine must return a true value (640 bytes, patch)
2016-11-16 07:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16588 Remove Readonly from Edifact::Order (1.73 KB, patch)
2016-11-16 07:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16588: Remove Readonly from the dependency list (1004 bytes, patch)
2016-11-16 07:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16588: Remove Readonly from SearchEngine (13.60 KB, patch)
2018-03-29 19:25 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16588: Koha::SearchEngine must return a true value (640 bytes, patch)
2018-03-29 19:25 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16588 Remove Readonly from Edifact::Order (1.76 KB, patch)
2018-03-29 19:25 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16588: Remove Readonly from the dependency list (1004 bytes, patch)
2018-03-29 19:25 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16588: Remove Readonly from SearchEngine (14.38 KB, patch)
2018-03-30 02:49 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16588: Koha::SearchEngine must return a true value (640 bytes, patch)
2018-03-30 02:49 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16588 Remove Readonly from Edifact::Order (1.76 KB, patch)
2018-03-30 02:49 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16588: Remove Readonly from the dependency list (1004 bytes, patch)
2018-03-30 02:49 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16588: Follow up with more missing changes (8.03 KB, patch)
2018-03-30 02:49 UTC, M. Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Sheat 2016-05-25 13:55:20 UTC
Doing a build in a clean environment:

#   Failed test 'use C4::Overdues;'
#   at t/00-load.t line 47.
#     Tried to use 'C4::Overdues'.
#     Error:  Can't locate Readonly.pm in @INC (you may need to install the Readonly module) (@INC contains: /build/koha-3.23~git+20160525154303.188b3b5b/blib/PERL_MODULE_DIR /build/koha-3.23~git+20160525154303.188b3b5b/blib/arch /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.22.1 /usr/local/share/perl/5.22.1 /usr/lib/x86_64-linux-gnu/perl5/5.22 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.22 /usr/share/perl/5.22 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base .) at /build/koha-3.23~git+20160525154303.188b3b5b/blib/PERL_MODULE_DIR/Koha/SearchEngine.pm line 22.

It should have been added in bug 7893, but apparently went missing some time later.
Comment 1 M. Tompsett 2016-06-24 14:36:46 UTC
Yes, I found this report as a result of noticing Readonly in the following:
Koha/Edifact/Order.pm
Koha/ElasticSearch.pm
Koha/SearchEngine.pm
Comment 2 Mirko Tietgen 2016-06-27 08:19:21 UTC
It was deleted by bug 14066. So it seems somebody reintroduced it.

I added both deps to the packages for 16.05.01. And a red readonly::xs on the about page already triggered the first question again.
Comment 3 Jonathan Druart 2016-06-27 12:52:01 UTC
I'd say, let's remove it (again) and use the constant pragma instead http://perlmaven.com/constants-and-read-only-variables-in-perl
Comment 4 M. Tompsett 2016-06-27 13:05:16 UTC
(In reply to Jonathan Druart from comment #3)
> I'd say, let's remove it (again) and use the constant pragma instead
> http://perlmaven.com/constants-and-read-only-variables-in-perl

And add something to the PERL section of the Coding Guidelines indicating the preferred method for defining read-only constants.
Comment 5 Mirko Tietgen 2016-07-22 05:50:10 UTC
Any volunteers?
Comment 6 Jonathan Druart 2016-07-22 08:14:06 UTC
Created attachment 53636 [details] [review]
Bug 16588: Remove Readonly from Edifact

Bug 14066 has removed the Readonly dependency, but in the meanwhile, 2
long standing reports reintroduced it: Edifact and ElasticSearch

This patchset will remove it from these 2 modules.

Test plan:
No idea how to test Edifact, I have not tested this patch
Comment 7 Jonathan Druart 2016-07-22 08:14:09 UTC
Created attachment 53637 [details] [review]
Bug 16588: Remove Readonly from SearchEngine

Test plan:
Make sure you are able to reindex and search for records at the OPAC
using ElasticSearch
Comment 8 Jonathan Druart 2016-07-22 08:14:12 UTC
Created attachment 53638 [details] [review]
Bug 16588: Koha::SearchEngine must return a true value
Comment 9 Colin Campbell 2016-08-08 15:00:36 UTC
Would it not be preferable to add Readonly rather than replace usages of Readonly by constant reintroducing exactly the problems with constant readonly addresses (see the Readonly doc). I thought Readonly was in the dependencies list previously.

It would be safer to replace the readonly vars with simple variables than using the constant pragma
Comment 10 Jonathan Druart 2016-08-09 08:34:18 UTC
(In reply to Colin Campbell from comment #9)
> Would it not be preferable to add Readonly rather than replace usages of
> Readonly by constant reintroducing exactly the problems with constant
> readonly addresses (see the Readonly doc). I thought Readonly was in the
> dependencies list previously.
> 
> It would be safer to replace the readonly vars with simple variables than
> using the constant pragma

I asked for feedbacks on bug 14066 comment 2 and did not get any cons so decided to continue on this way.
We only use constant with scalars so far, therefore the different problems with the constant pragma should not impact us too much.
Comment 11 M. Tompsett 2016-08-13 16:07:24 UTC
Since this touches a lot of { index => ... } stuff, I thought I'd add bug 16746 to the see also, given my counter patch and discussion on the bug regarding elastic vs. zebra calls.
Comment 12 M. Tompsett 2016-08-13 16:23:17 UTC
After applying this, there was still Readonly in Koha/Edifact/Order.pm, which is related in part to bug 16703's adding it back in, I think. Does _const really need Readonly for it to work?
Comment 13 Jonathan Druart 2016-08-15 13:56:26 UTC
Created attachment 54477 [details] [review]
Bug 16588: (follow-up) Remove Readonly from Edifact
Comment 14 Nick Clemens 2016-09-29 15:57:42 UTC
Created attachment 55934 [details] [review]
Bug 16588: Remove Readonly from Edifact

Bug 14066 has removed the Readonly dependency, but in the meanwhile, 2
long standing reports reintroduced it: Edifact and ElasticSearch

This patchset will remove it from these 2 modules.

Test plan:
No idea how to test Edifact, I have not tested this patch

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Nick Clemens 2016-09-29 15:57:47 UTC
Created attachment 55935 [details] [review]
Bug 16588: Remove Readonly from SearchEngine

Test plan:
Make sure you are able to reindex and search for records at the OPAC
using ElasticSearch

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens 2016-09-29 15:57:53 UTC
Created attachment 55936 [details] [review]
Bug 16588: Koha::SearchEngine must return a true value

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2016-09-29 15:57:59 UTC
Created attachment 55937 [details] [review]
Bug 16588: (follow-up) Remove Readonly from Edifact

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Nick Clemens 2016-09-29 15:58:37 UTC
I did not test Edifact but got Kyle Hall to take a look and he approved
Comment 19 Colin Campbell 2016-09-30 07:45:36 UTC
WHY??????

Using constant instead of readonly is more bug prone - If you must remove readonly use simple scalars instead of constant
Comment 20 Jonathan Druart 2016-09-30 08:05:39 UTC
(In reply to Colin Campbell from comment #19)
> WHY??????
> 
> Using constant instead of readonly is more bug prone - If you must remove
> readonly use simple scalars instead of constant

Because it has been added, then removed because it was not really used 14066, then re-added but not correctly (without the dependency).
There are no really advantages (regarding at how we use it) to depend on Readonly, and constant is in the perl code.
This bug prevent packages to build correctly in a clean environment.
I have already answered you on comment 10 about Readonly vs constant, you can also see bug 14066 comment 2 (more than 2 years ago).
A patch was submitted (2 months ago) when nobody wanted to fix it.

Feel free to submit a counter patch, but I am out.
Comment 21 Jonathan Druart 2016-09-30 08:06:11 UTC
(In reply to Jonathan Druart from comment #20)

perl core*
Comment 22 Mirko Tietgen 2016-09-30 08:11:17 UTC
Can we please not re-discuss this from the beginning now that we are almost there? It was brought up months ago and now it got a lot of work put into it.

I need this for packaging (one way or the other) and I'd appreciate if it could come soon. To go back to re-re-add readonly (or how many re-s that are by now) please open a new bug and base your patches on this one.
Comment 23 Colin Campbell 2016-10-26 09:59:22 UTC
Created attachment 56853 [details] [review]
Amended Patch

Remove the Readonly dependency from Edifact::Order in a way that does not introduce minor logic changes - the readonly variables are now simple scalars and hashes
Comment 24 Katrin Fischer 2016-11-06 10:40:16 UTC
Patch doesn't apply - can we also get a quick sign-off on the last patch from Colin to complete this?
Comment 25 Jonathan Druart 2016-11-16 07:16:11 UTC
Created attachment 57516 [details] [review]
Bug 16588: Remove Readonly from SearchEngine

Test plan:
Make sure you are able to reindex and search for records at the OPAC
using ElasticSearch

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 26 Jonathan Druart 2016-11-16 07:16:18 UTC
Created attachment 57517 [details] [review]
Bug 16588: Koha::SearchEngine must return a true value

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 27 Jonathan Druart 2016-11-16 07:16:23 UTC
Created attachment 57518 [details] [review]
Bug 16588 Remove Readonly from Edifact::Order

Remove Readonly dependency from Edifact::Order

Do not replace with Constant as we want these variables to
interpolate and do not wish to change the logic in constructing the
service string advice
Comment 28 Jonathan Druart 2016-11-16 07:16:27 UTC
Created attachment 57519 [details] [review]
Bug 16588: Remove Readonly from the dependency list
Comment 29 Srdjan Jankovic 2017-02-28 00:43:04 UTC
Is there any reason we have BIBLIOS_INDEX and AUTHORITIES_INDEX defined in Koha::SearchEngine::Elasticsearch?
Comment 30 Jonathan Druart 2017-02-28 08:11:18 UTC
(In reply to Srdjan Jankovic from comment #29)
> Is there any reason we have BIBLIOS_INDEX and AUTHORITIES_INDEX defined in
> Koha::SearchEngine::Elasticsearch?

Where do you want to have them defined?
Comment 31 Srdjan Jankovic 2017-03-01 00:07:38 UTC
(In reply to Jonathan Druart from comment #30)
> (In reply to Srdjan Jankovic from comment #29)
> > Is there any reason we have BIBLIOS_INDEX and AUTHORITIES_INDEX defined in
> > Koha::SearchEngine::Elasticsearch?
> 
> Where do you want to have them defined?

Well they are already defined with the same values in Koha::SearchEngine. I just wanted to know if they relate to the same thing.
Comment 32 Jonathan Druart 2017-03-07 16:06:54 UTC
(In reply to Srdjan Jankovic from comment #31)
> (In reply to Jonathan Druart from comment #30)
> > (In reply to Srdjan Jankovic from comment #29)
> > > Is there any reason we have BIBLIOS_INDEX and AUTHORITIES_INDEX defined in
> > > Koha::SearchEngine::Elasticsearch?
> > 
> > Where do you want to have them defined?
> 
> Well they are already defined with the same values in Koha::SearchEngine. I
> just wanted to know if they relate to the same thing.

Yes, in my understanding they are.
One could be removed.
Comment 33 Katrin Fischer 2017-10-07 22:29:59 UTC
There is a conflict that I tried to fix, but maybe it's better you take a look.
Tried to locate other cases that need to be fixed: $ git grep '\$Koha:'

Koha/Authorities.pm:    my $searcher = Koha::SearchEngine::Search->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX });
Koha/Authorities.pm:    my $searcher = Koha::SearchEngine::Search->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX });

Looks suspicious.
Comment 34 M. Tompsett 2018-01-30 18:46:09 UTC
The removal of Readonly::XS is actually a separate problem, in that even if we were keeping Readonly, we shouldn't be using or checking for Readonly::XS. See bug 20103 instead. This will conflict with the last patch.
Comment 35 M. Tompsett 2018-03-29 19:24:43 UTC
I'm taking this, to bring it back to a state where it can apply.
Comment 36 M. Tompsett 2018-03-29 19:25:38 UTC
Created attachment 73444 [details] [review]
Bug 16588: Remove Readonly from SearchEngine

Test plan:
Make sure you are able to reindex and search for records at the OPAC
using ElasticSearch

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 37 M. Tompsett 2018-03-29 19:25:42 UTC
Created attachment 73445 [details] [review]
Bug 16588: Koha::SearchEngine must return a true value

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 38 M. Tompsett 2018-03-29 19:25:45 UTC
Created attachment 73446 [details] [review]
Bug 16588 Remove Readonly from Edifact::Order

Remove Readonly dependency from Edifact::Order

Do not replace with Constant as we want these variables to
interpolate and do not wish to change the logic in constructing the
service string advice
Comment 39 M. Tompsett 2018-03-29 19:25:49 UTC
Created attachment 73447 [details] [review]
Bug 16588: Remove Readonly from the dependency list
Comment 40 M. Tompsett 2018-03-29 19:29:44 UTC
Still some issues to work on. Please do not think I am finished. Sorry for being spammy.
Comment 41 M. Tompsett 2018-03-30 02:49:23 UTC
Created attachment 73448 [details] [review]
Bug 16588: Remove Readonly from SearchEngine

Test plan:
Make sure you are able to reindex and search for records at the OPAC
using ElasticSearch

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 42 M. Tompsett 2018-03-30 02:49:28 UTC
Created attachment 73449 [details] [review]
Bug 16588: Koha::SearchEngine must return a true value

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 43 M. Tompsett 2018-03-30 02:49:32 UTC
Created attachment 73450 [details] [review]
Bug 16588 Remove Readonly from Edifact::Order

Remove Readonly dependency from Edifact::Order

Do not replace with Constant as we want these variables to
interpolate and do not wish to change the logic in constructing the
service string advice
Comment 44 M. Tompsett 2018-03-30 02:49:36 UTC
Created attachment 73451 [details] [review]
Bug 16588: Remove Readonly from the dependency list
Comment 45 M. Tompsett 2018-03-30 02:49:40 UTC
Created attachment 73452 [details] [review]
Bug 16588: Follow up with more missing changes

Hunting for _INDEX and $Koha::SearchEngine provided
more places to tweak.

Added 'use Koha::SearchEngine;' where the constant was referenced.