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.
Yes, I found this report as a result of noticing Readonly in the following: Koha/Edifact/Order.pm Koha/ElasticSearch.pm Koha/SearchEngine.pm
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.
I'd say, let's remove it (again) and use the constant pragma instead http://perlmaven.com/constants-and-read-only-variables-in-perl
(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.
Any volunteers?
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
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
Created attachment 53638 [details] [review] Bug 16588: Koha::SearchEngine must return a true value
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
(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.
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.
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?
Created attachment 54477 [details] [review] Bug 16588: (follow-up) Remove Readonly from Edifact
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>
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>
Created attachment 55936 [details] [review] Bug 16588: Koha::SearchEngine must return a true value Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 55937 [details] [review] Bug 16588: (follow-up) Remove Readonly from Edifact Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
I did not test Edifact but got Kyle Hall to take a look and he approved
WHY?????? Using constant instead of readonly is more bug prone - If you must remove readonly use simple scalars instead of constant
(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.
(In reply to Jonathan Druart from comment #20) perl core*
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.
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
Patch doesn't apply - can we also get a quick sign-off on the last patch from Colin to complete this?
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>
Created attachment 57517 [details] [review] Bug 16588: Koha::SearchEngine must return a true value Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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
Created attachment 57519 [details] [review] Bug 16588: Remove Readonly from the dependency list
Is there any reason we have BIBLIOS_INDEX and AUTHORITIES_INDEX defined in Koha::SearchEngine::Elasticsearch?
(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?
(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.
(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.
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.
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.
I'm taking this, to bring it back to a state where it can apply.
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>
Created attachment 73445 [details] [review] Bug 16588: Koha::SearchEngine must return a true value Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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
Created attachment 73447 [details] [review] Bug 16588: Remove Readonly from the dependency list
Still some issues to work on. Please do not think I am finished. Sorry for being spammy.
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>
Created attachment 73449 [details] [review] Bug 16588: Koha::SearchEngine must return a true value Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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
Created attachment 73451 [details] [review] Bug 16588: Remove Readonly from the dependency list
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.
I was hoping for a sign off on my follow up. Now this needs a rebase, and it's messy.
It looks like this is still valid: Koha/Edifact/Order.pm:use Readonly qw( Readonly ); Koha/Edifact/Order.pm:Readonly::Scalar my $seg_terminator => q{'}; Koha/Edifact/Order.pm:Readonly::Scalar my $separator => q{+}; Koha/Edifact/Order.pm:Readonly::Scalar my $component_separator => q{:}; Koha/Edifact/Order.pm:Readonly::Scalar my $release_character => q{?}; Koha/Edifact/Order.pm:Readonly::Scalar my $NINES_12 => 999_999_999_999; Koha/Edifact/Order.pm:Readonly::Scalar my $NINES_14 => 99_999_999_999_999; Koha/Edifact/Order.pm:Readonly::Scalar my $CHUNKSIZE => 35; Koha/Edifact/Order.pm: Readonly my %S => { Koha/SearchEngine.pm:use Readonly qw( Readonly ); Koha/SearchEngine.pm:Readonly our $BIBLIOS_INDEX => 'biblios'; Koha/SearchEngine.pm:Readonly our $AUTHORITIES_INDEX => 'authorities'; Koha/SearchEngine/Elasticsearch.pm:use Readonly qw( Readonly ); Koha/SearchEngine/Elasticsearch.pm:Readonly our $BIBLIOS_INDEX => 'biblios'; Koha/SearchEngine/Elasticsearch.pm:Readonly our $AUTHORITIES_INDEX => 'authorities'; cpanfile:requires 'Readonly', '2.00';