On bug 17591 we discovered that there was something weird going on with the way we export and use subroutines/modules. I tried to standardize our EXPORT to use EXPORT_OK only.
Created attachment 57394 [details] [review] Bug 17600: Standardize our EXPORT_OK On bug 17591 we discovered that there was something weird going on with the way we export and use subroutines/modules. This patch tries to standardize our EXPORT to use EXPORT_OK only. That way we will need to explicitely define the subroutine we want to use from a module. There are still a lot of things to do, it would be good to write a script to do what's need to be done.
I just wanted to comment that I like this idea. Straight from perl.org: If you must export try to use @EXPORT_OK in preference to @EXPORT and avoid short or common symbol names to reduce the risk of name clashes.
(In reply to Jonathan Druart from comment #1) > On bug 17591 we discovered that there was something weird going on with > the way we export and use subroutines/modules. > This patch tries to standardize our EXPORT to use EXPORT_OK only. Probably Plack related? Would be good to list these things somewhere centrally too. Some saw some things too that you cannot understand perl-wise, but seem to be Plack peculiarities.
(In reply to Marcel de Rooy from comment #3) > (In reply to Jonathan Druart from comment #1) > > On bug 17591 we discovered that there was something weird going on with > > the way we export and use subroutines/modules. > > This patch tries to standardize our EXPORT to use EXPORT_OK only. > > Probably Plack related? > Would be good to list these things somewhere centrally too. > Some saw some things too that you cannot understand perl-wise, but seem to > be Plack peculiarities. No it's not Plack related. It comes from tests, see bug 17591.
+1 from me, this would be a great move forward for Koha :)
Those BEGIN blocks of ugliness will get cleaned up quite a bit with this. Plus, standards as per comment #2. Is this still being worked on, or did you forget to change it to needs sign off?
(In reply to M. Tompsett from comment #6) > Those BEGIN blocks of ugliness will get cleaned up quite a bit with this. > Plus, standards as per comment #2. Is this still being worked on, or did you > forget to change it to needs sign off? Answer in the commit message: (In reply to Jonathan Druart from comment #1) > That way we will need to explicitely define the subroutine we want to > use from a module. > There are still a lot of things to do, it would be good to write > a script to do what's need to be done.
Created attachment 57950 [details] [review] Bug 17600: Standardize our EXPORT_OK On bug 17591 we discovered that there was something weird going on with the way we export and use subroutines/modules. This patch tries to standardize our EXPORT to use EXPORT_OK only. That way we will need to explicitely define the subroutine we want to use from a module. There are still a lot of things to do, it would be good to write a script to do what's need to be done.
Created attachment 57951 [details] [review] Bug 17600: Remove useless extra spaces
Created attachment 57952 [details] [review] Bug 17600: continue replace of EXPORT with EXPORT_OK
Created attachment 57953 [details] [review] Bug 17600: Explicitly export the subroutines used This patch is generated using the export.pl script
Created attachment 57954 [details] [review] [DO NOT PUSH] Script to export the subroutines used This ugly script has been used to generate the last patch
So, what's next? It won't be feasible to test all the changes. So we need to decide if it worth the try to push these patches as it. At least it would be great to get a review from someone else.
Ok so, a lot of things is broken with these patches: - from tests: use_ok should be be used since it does not export subroutines - from pm: subroutines are not exported sometimes whereas the script assumes they were things are working when they are expecting not to (get_template_and_user from C4::Auth is not exported by scripts use it and it works). Maybe it would be easier not to export anything and use fully qualified names instead?
(In reply to Jonathan Druart from comment #14) > Maybe it would be easier not to export anything and use fully qualified > names instead? If someone has "use Foo;" in their code, they probably would rather write Bar() than Foo::Bar(); in their code. I know I would.
(In reply to M. Tompsett from comment #15) > (In reply to Jonathan Druart from comment #14) > > Maybe it would be easier not to export anything and use fully qualified > > names instead? > > If someone has "use Foo;" in their code, they probably would rather write > Bar() than Foo::Bar(); in their code. I know I would. It is what is done here, you have to export them explicitly. use Foo qw( bar ); bar();
Put in QA queue to get feedbacks from QAer. More than 2 signoffs from QA team members would be great.
(In reply to Jonathan Druart from comment #17) > Put in QA queue to get feedbacks from QAer. More than 2 signoffs from QA > team members would be great. Will have a look later. But as you state, it needs another signoff too.
This one is also on my list for today :)
This all looks solid to me, and is another move in the right direction in my opinion. Was about to add my Passed QA mark when I realised I'd need to rebase as the project moved whilst I was testing. Could you rebase Jonathan?
Created attachment 61994 [details] [review] Bug 17600: Standardize our EXPORT_OK On bug 17591 we discovered that there was something weird going on with the way we export and use subroutines/modules. This patch tries to standardize our EXPORT to use EXPORT_OK only. That way we will need to explicitely define the subroutine we want to use from a module. There are still a lot of things to do, it would be good to write a script to do what's need to be done.
Created attachment 61995 [details] [review] Bug 17600: Remove useless extra spaces
Created attachment 61996 [details] [review] Bug 17600: continue replace of EXPORT with EXPORT_OK
Created attachment 61997 [details] [review] Bug 17600: Explicitly export the subroutines used This patch is generated using the export.pl script
Created attachment 61998 [details] [review] Bug 17600: Rerun the script to update use statements
Applying: Bug 17600: Explicitly export the subroutines used fatal: sha1 information is lacking or useless (C4/Auth_with_cas.pm). Repository lacks necessary blobs to fall back on 3-way merge.
Created attachment 62209 [details] [review] Bug 17600: Standardize our EXPORT_OK On bug 17591 we discovered that there was something weird going on with the way we export and use subroutines/modules. This patch tries to standardize our EXPORT to use EXPORT_OK only. That way we will need to explicitely define the subroutine we want to use from a module. There are still a lot of things to do, it would be good to write a script to do what's need to be done.
Created attachment 62210 [details] [review] Bug 17600: Remove useless extra spaces
Created attachment 62211 [details] [review] Bug 17600: continue replace of EXPORT with EXPORT_OK
Created attachment 62212 [details] [review] Bug 17600: Explicitly export the subroutines used This patch is generated using the export.pl script
Created attachment 62213 [details] [review] Bug 17600: Rerun the script to update use statements
So, this is a PQA from me.. Did Marcel also add his PQA.. if so we should do one last rebase and push the bugger ;)
(In reply to Martin Renvoize from comment #32) > So, this is a PQA from me.. Did Marcel also add his PQA.. if so we should do > one last rebase and push the bugger ;) Did you add a signoff line? I still have a look this week.
Created attachment 62940 [details] [review] Bug 17600: Standardize our EXPORT_OK On bug 17591 we discovered that there was something weird going on with the way we export and use subroutines/modules. This patch tries to standardize our EXPORT to use EXPORT_OK only. That way we will need to explicitely define the subroutine we want to use from a module. There are still a lot of things to do, it would be good to write a script to do what's need to be done. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 62941 [details] [review] Bug 17600: Remove useless extra spaces Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 62942 [details] [review] Bug 17600: continue replace of EXPORT with EXPORT_OK Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 62943 [details] [review] Bug 17600: Explicitly export the subroutines used This patch is generated using the export.pl script Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I have now ;)
(In reply to Martin Renvoize from comment #38) > I have now ;) Great, Martin. Thanks.
Applying: Bug 17600: Rerun the script to update use statements fatal: sha1 information is lacking or useless (C4/Auth_with_shibboleth.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 17600: Rerun the script to update use statements Somehow I have the feeling that we are running more frequently against these errors. Something to do with different git versions ? I am using git version 2.1.4 on Jessie. (Should be the default version?)
Created attachment 62958 [details] [review] Bug 17600: Rerun the script to update use statements
Created attachment 62959 [details] [review] Bug 17600: re-Rerun the script to update use statements
(In reply to Marcel de Rooy from comment #40) > Applying: Bug 17600: Rerun the script to update use statements > fatal: sha1 information is lacking or useless (C4/Auth_with_shibboleth.pm). > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. > Patch failed at 0001 Bug 17600: Rerun the script to update use statements > > Somehow I have the feeling that we are running more frequently against these > errors. Something to do with different git versions ? > I am using git version 2.1.4 on Jessie. (Should be the default version?) One patch was not in the correct order.
(In reply to Jonathan Druart from comment #43) > (In reply to Marcel de Rooy from comment #40) > > Applying: Bug 17600: Rerun the script to update use statements > > fatal: sha1 information is lacking or useless (C4/Auth_with_shibboleth.pm). > > Repository lacks necessary blobs to fall back on 3-way merge. > > Cannot fall back to three-way merge. > > Patch failed at 0001 Bug 17600: Rerun the script to update use statements > > > > Somehow I have the feeling that we are running more frequently against these > > errors. Something to do with different git versions ? > > I am using git version 2.1.4 on Jessie. (Should be the default version?) > > One patch was not in the correct order. OK. It does apply now. Trying to have a look tomorrow.
Note that I do not support an inclusion of this patch in 17.05 (too many side-effects possible), but should be pushed at the beginning of the next cycle.
(In reply to Jonathan Druart from comment #45) > Note that I do not support an inclusion of this patch in 17.05 (too many > side-effects possible), but should be pushed at the beginning of the next > cycle. OK. Postponing QA in that case.
Applying: Bug 17600: Explicitly export the subroutines used fatal: sha1 information is lacking or useless (C4/Auth_with_cas.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. If it applies Friday morning, I will start with it ;) Just using git 2.1.4 on Jessie..
Created attachment 64387 [details] [review] Bug 17600: Standardize our EXPORT_OK On bug 17591 we discovered that there was something weird going on with the way we export and use subroutines/modules. This patch tries to standardize our EXPORT to use EXPORT_OK only. That way we will need to explicitely define the subroutine we want to use from a module. There are still a lot of things to do, it would be good to write a script to do what's need to be done. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 64388 [details] [review] Bug 17600: Remove useless extra spaces Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 64389 [details] [review] Bug 17600: continue replace of EXPORT with EXPORT_OK Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 64390 [details] [review] Bug 17600: Explicitly export the subroutines used This patch is generated using the export.pl script Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 64391 [details] [review] Bug 17600: Rerun the script to update use statements
Created attachment 64392 [details] [review] Bug 17600: re-Rerun the script to update use statements
Too many errors "subroutine is not exported by the C4::XXX module"
Created attachment 64466 [details] [review] Bug 17600: Standardize our EXPORT_OK On bug 17591 we discovered that there was something weird going on with the way we export and use subroutines/modules. This patch tries to standardize our EXPORT to use EXPORT_OK only. That way we will need to explicitely define the subroutine we want to use from a module. There are still a lot of things to do, it would be good to write a script to do what's need to be done. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 64467 [details] [review] Bug 17600: Remove useless extra spaces Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 64468 [details] [review] Bug 17600: continue replace of EXPORT with EXPORT_OK Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 64469 [details] [review] Bug 17600: Explicitly export the subroutines used This patch is generated using the export.pl script Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 64470 [details] [review] Bug 17600: Rerun the script to update use statements
Created attachment 64471 [details] [review] Bug 17600: re-Rerun the script to update use statements
Created attachment 64472 [details] [review] Bug 17600: Export subroutine from modules
Created attachment 64473 [details] [review] Update script to export - export for scripts
Created attachment 64474 [details] [review] Bug 17600: Explicitly export the subroutines used - scripts
Created attachment 64475 [details] [review] Bug 17600: (follow-up) Explicitly export the subroutines used - scripts manual adjustements
Created attachment 64476 [details] [review] Bug 17600: Some more...
Hard to say if this change is going to bring us more trouble or it is going to help us. I send it to the QA queue to get QAers opinions.
- require Exporter; - $debug = $ENV{DEBUG}; - @ISA = qw(Exporter); - @EXPORT_OK = qw(check_api_auth_cas checkpw_cas login_cas logout_cas login_cas_url); -} + require Exporter; + $debug = $ENV{DEBUG}; + @ISA = qw(Exporter); +porter); +RT_OK = qw( +OK = qw( +ultipleAuth +ipleAuth +etMultipleAuth +ipleAuth +ogout_cas +gout_cas Funny things happening here.. It seems that git or qa tools did not like Auth_with_cas ??
Auth_with_ldap too btw
Well, it seems to be git or git bz that mangles those files. Reapplying patch 0008 triggers the same problem with me.
Just noting that these patches are in git 2.11.0 and git 2.1.4 apparently does not like them. Too bad :)
Can we rearrange these patches too? All patches that should not be pushed together? 0009? All patches for modules and for scripts or so ?
(In reply to Marcel de Rooy from comment #71) > Can we rearrange these patches too? > All patches that should not be pushed together? 0009? > All patches for modules and for scripts or so ? I am not sure to understand what you mean here. How would you like me to rearrange these patches? Which one is 0009?
It is not possible to maintain these patches up-to-date. Here is a remote branch I am not going to rebase every week: https://github.com/joubu/Koha/commits/bug_17600 I will update it when people will be ready to approve this approach.
(In reply to Jonathan Druart from comment #72) > (In reply to Marcel de Rooy from comment #71) > > Can we rearrange these patches too? > > All patches that should not be pushed together? 0009? > > All patches for modules and for scripts or so ? > > I am not sure to understand what you mean here. How would you like me to > rearrange these patches? Which one is 0009? That should be the 9th that you obsoleted :)
I have rebased the branch (tricky!), but I get Undefined subroutine &C4::Items::TransformKohaToMarc called at /home/vagrant/kohaclone/C4/Items.pm line 1491. when I hit /cgi-bin/koha/catalogue/MARCdetail.pl?biblionumber=42 TransformKohaToMarc is exported by C4::Biblio. And it is what we are trying to avoid... Any ideas?
(In reply to Jonathan Druart from comment #75) > I have rebased the branch (tricky!), but I get > > Undefined subroutine &C4::Items::TransformKohaToMarc called at > /home/vagrant/kohaclone/C4/Items.pm line 1491. > > when I hit /cgi-bin/koha/catalogue/MARCdetail.pl?biblionumber=42 > > TransformKohaToMarc is exported by C4::Biblio. > And it is what we are trying to avoid... > > Any ideas? Circular dependencies. C4::Biblio use C4::Items which use C4::Biblio; If I comment 'use C4::Items ...' in C4::Biblio, MARCdetail.pl works fine. It seems that this 'use' is not even needed. C4::Items is 'require'd each time it's needed in C4::Biblio.
Not sure what's going on here
(In reply to Jonathan Druart from comment #75) > Undefined subroutine &C4::Items::TransformKohaToMarc called at > /home/vagrant/kohaclone/C4/Items.pm line 1491. > > when I hit /cgi-bin/koha/catalogue/MARCdetail.pl?biblionumber=42 Bug 18927 fixes this issue.
Where did the patches go?
(In reply to Kyle M Hall from comment #79) > Where did the patches go? See comment 73: Here is a remote branch I am not going to rebase every week: https://github.com/joubu/Koha/commits/bug_17600 It is here to get feedback.
Hi Jonathan, could you please rebase your branch on current master? Thx!
(In reply to Katrin Fischer from comment #81) > Hi Jonathan, > could you please rebase your branch on current master? Thx! See my previous comment, I do not plan to maintain this branch up-to-date. As I did not get the expected feedback it is not longer on my priority list.
(In reply to Jonathan Druart from comment #75) > See my previous comment, I do not plan to maintain this branch up-to-date. > As I did not get the expected feedback it is not longer on my priority list. I know this is no longer on your priority list, but I'm happy to provide some feedback now that I know this bug exists.
(In reply to Jonathan Druart from comment #75) > I have rebased the branch (tricky!), but I get > > Undefined subroutine &C4::Items::TransformKohaToMarc called at > /home/vagrant/kohaclone/C4/Items.pm line 1491. > > when I hit /cgi-bin/koha/catalogue/MARCdetail.pl?biblionumber=42 > > TransformKohaToMarc is exported by C4::Biblio. > And it is what we are trying to avoid... > > Any ideas? I don't really understand the question here. Based on https://github.com/joubu/Koha/commit/9bbab48f740f1ebcb4c08549622d750b34fe14c0, I'm guessing that you're explicitly importing "TransformKohaToMarc" from C4::Biblio at the top of C4::Items, but you're still getting "Undefined subroutine &C4::Items::TransformKohaToMarc called at > /home/vagrant/kohaclone/C4/Items.pm line 1491."? I'd need to be able to poke around on the system to know exactly what's going on and I'd have to do some experiments (like dumping %INC and looking at functions by namespace to see what is declared at what time), but... ...wouldn't a solution be just to write C4::Biblio::TransformKohaToMarc? Personally, I think we overuse Exporter.pm. I think moving from EXPORT to EXPORT_OK is a great improvement, but I'm not sure why we export so many functions either. Perhaps because it makes things easier to write, but it makes things harder to read and understand, and it creates issues like these...
After Martin's suggestion to have a look at https://metacpan.org/pod/perlimports I had another go at this one. And it seems to work better than my previous attempt. There is a remote branch: https://gitlab.com/joubu/Koha/-/tree/bug_17600_new I am going to attach the patches here but I won't keep them up-to-date. The remote branch must be tested instead.
Created attachment 121309 [details] [review] Bug 17600: Standardize our EXPORT_OK On bug 17591 we discovered that there was something weird going on with the way we export and use subroutines/modules. This patch tries to standardize our EXPORT to use EXPORT_OK only. That way we will need to explicitely define the subroutine we want to use from a module. There are still a lot of things to do, it would be good to write a script to do what's need to be done.
Created attachment 121310 [details] [review] Bug 17600: After export.pl
Created attachment 121311 [details] [review] Bug 17600: After perlimports find . -name '*.pm' -o -name '*.pl' -exec perl App-perlimports/script/perlimports --inplace-edit --no-preserve-unused --filename {} \;
Created attachment 121312 [details] [review] Bug 17600: Manual changes
Created attachment 121313 [details] [review] Bug 17600: after perlimports pm Was missing the pm find . \( -name "*.pl" -o -name "*.pm" \) -exec perl App-perlimports/script/perlimports --inplace-edit --no-preserve-unused --filename {} \; TODO: Restore permissions
Created attachment 121314 [details] [review] Bug 17600: Other manual changes after second perlimports run
Created attachment 122360 [details] [review] Bug 17600: Standardize our EXPORT_OK On bug 17591 we discovered that there was something weird going on with the way we export and use subroutines/modules. This patch tries to standardize our EXPORT to use EXPORT_OK only. That way we will need to explicitely define the subroutine we want to use from a module. There are still a lot of things to do, it would be good to write a script to do what's need to be done.
Created attachment 122361 [details] [review] Bug 17600: After export.pl
Created attachment 122362 [details] [review] Bug 17600: After perlimports find . -name '*.pm' -o -name '*.pl' -exec perl App-perlimports/script/perlimports --inplace-edit --no-preserve-unused --filename {} \;
Created attachment 122363 [details] [review] Bug 17600: Manual changes
Created attachment 122364 [details] [review] Bug 17600: after perlimports pm Was missing the pm find . \( -name "*.pl" -o -name "*.pm" \) -exec perl App-perlimports/script/perlimports --inplace-edit --no-preserve-unused --filename {} \; TODO: Restore permissions
Created attachment 122365 [details] [review] Bug 17600: Other manual changes after second perlimports run
Please test the remote branch - https://gitlab.com/joubu/Koha/-/tree/bug_17600_new
This remote branch is looking great to me.. it just 'feels cleaner' as a whole. I'm just running the test suit against it locally and playing around a bit to see if I spot any obvious issues now.. Thanks for all the hard work Jonathan!
We have the compile test failing at the moment.. I'll try to find a moment to dig further into this.. but I'm not sure when that will happen :(
(In reply to Martin Renvoize from comment #100) > We have the compile test failing at the moment.. I'll try to find a moment > to dig further into this.. but I'm not sure when that will happen :( Fixed! Remote branch updated.
Joubu> Asking again for the Xth times "Do we need bug 17600 (Standardize the EXPORT) or not?" ashimema> I like it ashimema> and have tested... Joubu> It's now or never, basically ashimema> I'd go for it ashimema> personally Joubu> I don't need stamps on it, only a "go for it" is enough :) Joubu> "I will push it at the beginning of July, regardless of its status, if there is no objection before." Joubu> this is what I actually sent to the QA team a couple of weeks ago Joubu> but I am feeling a bit ashamed to push such big changes with a NSO status Joubu> marcelr, kidclamp, khall, jajm, tcohen, you all agree with that, right? ^ marcelr> Joubu you promised it marcelr> wait and begin of July takes another year khall> go for it marcelr> we have 4 months for cleanup haha Joubu> Thanks, I didn't need more. A short "go" is better than silence ;) jajm> Joubu, go go go! Joubu> I will quote you all on the bug :D ashimema> go :) ashimema> I did do a chunk of testing so take my SO tcohen> Joubu add my stamp tcohen> I tested it in the early ages tcohen> and am sure we can fix whatever arises
Created attachment 122869 [details] [review] Bug 17600: Standardize our EXPORT_OK On bug 17591 we discovered that there was something weird going on with the way we export and use subroutines/modules. This patch tries to standardize our EXPORT to use EXPORT_OK only. That way we will need to explicitely define the subroutine we want to use from a module. This patch is a squashed version of: Bug 17600: After export.pl Bug 17600: After perlimport Bug 17600: Manual changes Bug 17600: Other manual changes after second perlimports run Bug 17600: Fix tests And a lot of other manual changes. export.pl is a dirty script that can be found on bug 17600. "perlimport" is: git clone https://github.com/oalders/App-perlimports.git cd App-perlimports/ cpanm --installdeps . export PERL5LIB="$PERL5LIB:/kohadevbox/koha/App-perlimports/lib" find . \( -name "*.pl" -o -name "*.pm" \) -exec perl App-perlimports/script/perlimports --inplace-edit --no-preserve-unused --filename {} \; The ideas of this patch are to: * use EXPORT_OK instead of EXPORT * perltidy the EXPORT_OK list * remove '&' before the subroutine names * remove some uneeded use statements * explicitely import the subroutines we need within the controllers or modules Note that the private subroutines (starting with _) should not be exported (and not used from outside of the module except from tests). EXPORT vs EXPORT_OK (from https://www.thegeekstuff.com/2010/06/perl-exporter-examples/) """ Export allows to export the functions and variables of modules to user’s namespace using the standard import method. This way, we don’t need to create the objects for the modules to access it’s members. @EXPORT and @EXPORT_OK are the two main variables used during export operation. @EXPORT contains list of symbols (subroutines and variables) of the module to be exported into the caller namespace. @EXPORT_OK does export of symbols on demand basis. """ If this patch caused a conflict with a patch you wrote prior to its push: * Make sure you are not reintroducing a "use" statement that has been removed * "$subroutine" is not exported by the C4::$MODULE module means that you need to add the subroutine to the @EXPORT_OK list * Bareword "$subroutine" not allowed while "strict subs" means that you didn't imported the subroutine from the module: - use $MODULE qw( $subroutine list ); You can also use the fully qualified namespace: C4::$MODULE::$subroutine Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Patch pushed to master for 21.11.00
Created attachment 122884 [details] [review] Bug 17600: Fix tests No idea why UpdateStats in C4::Circulation needs the fully qualified namespace! I kept getting Undefined subroutine &C4::Circulation::UpdateStats called at /kohadevbox/koha/C4/Circulation.pm line 1643. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 122945 [details] [review] Bug 17600: Fix batchMod.pl Undefined subroutine &CGI::Compile::ROOT::kohadevbox_koha_tools_batchMod_2epl::haspermission called at /kohadevbox/koha/tools/batchMod.pl line 89 Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Follow-ups have been pushed to master.
Just wondering about the implications of this as I did some easy rebases today. Should we ask for new use statements always having the methods used added with qw? Should there be a coding guideline/QA test?
[WARN] Prototype mismatch: sub Koha::I18N::LC_MESSAGES: none vs () at /usr/share/koha/Koha/I18N.pm line 36. Locale::Messages exports a sub LC_MESSAGES without prototype. And after we do 'use POSIX;' with a default import that probably includes LC_MESSAGES from locale_h ?
Created attachment 123194 [details] [review] Bug 17600: Fix other occurrences in svc
(In reply to Jonathan Druart from comment #110) > Created attachment 123194 [details] [review] [review] > Bug 17600: Fix other occurrences in svc Patch pushed to master.
Created attachment 123195 [details] [review] Bug 17600: Fix wrong import in Koha::I18N - LC_MESSAGES $ perl -wc Koha/I18N.pm Subroutine Koha::I18N::LC_MESSAGES redefined at Koha/I18N.pm line 36. Prototype mismatch: sub Koha::I18N::LC_MESSAGES: none vs () at Koha/I18N.pm line 36. Koha/I18N.pm syntax OK
(In reply to Jonathan Druart from comment #112) > Created attachment 123195 [details] [review] [review] > Bug 17600: Fix wrong import in Koha::I18N - LC_MESSAGES > > $ perl -wc Koha/I18N.pm > Subroutine Koha::I18N::LC_MESSAGES redefined at Koha/I18N.pm line 36. > Prototype mismatch: sub Koha::I18N::LC_MESSAGES: none vs () at Koha/I18N.pm > line 36. > Koha/I18N.pm syntax OK Patch pushed to master.
(In reply to Katrin Fischer from comment #108) > Just wondering about the implications of this as I did some easy rebases > today. Should we ask for new use statements always having the methods used > added with qw? Should there be a coding guideline/QA test? I've tried to explain in the commit message. Basically you need to 1. Use EXPORT_OK instead of EXPORT in modules 2. either use Module qw( method_1 method_2 ); method_1(); or use Module; Module::method_1(); Ideally we should pick one version and stick to that. But there is still a mix of both everywhere in the codebase. We don't really need a guideline because: - We must not add new modules to C4 - The EXPORT_OK should be there now, so you have to write the imports/calls correctly of the execution will explode. (In reply to Marcel de Rooy from comment #109) > [WARN] Prototype mismatch: sub Koha::I18N::LC_MESSAGES: none vs () at > /usr/share/koha/Koha/I18N.pm line 36. > > Locale::Messages exports a sub LC_MESSAGES without prototype. > And after we do 'use POSIX;' with a default import that probably includes > LC_MESSAGES from locale_h ? Last patch fixes that.
(In reply to Jonathan Druart from comment #113) > (In reply to Jonathan Druart from comment #112) > > Created attachment 123195 [details] [review] [review] [review] > > Bug 17600: Fix wrong import in Koha::I18N - LC_MESSAGES > > > > $ perl -wc Koha/I18N.pm > > Subroutine Koha::I18N::LC_MESSAGES redefined at Koha/I18N.pm line 36. > > Prototype mismatch: sub Koha::I18N::LC_MESSAGES: none vs () at Koha/I18N.pm > > line 36. > > Koha/I18N.pm syntax OK > > Patch pushed to master. This is totally wrong, we do want to import LC_MESSAGES from Locale::Messages.
Created attachment 123196 [details] [review] Bug 17600: (follow-up) Fix wrong import in Koha::I18N - LC_MESSAGES
(In reply to Jonathan Druart from comment #116) > Created attachment 123196 [details] [review] [review] > Bug 17600: (follow-up) Fix wrong import in Koha::I18N - LC_MESSAGES Can I get a review on that patch please, before I push it and continue to mess up with things. I think this fixes a problem we had prior to this patch (to confirm!). Both Locale::Messages and POSIX are exporting LC_MESSAGES. As we explicitly imported LC_MESSAGES from Locale::Messages I guess it's what we wanted to do, but POSIX exported it as well and it's the one we got (it's after Locale::Messages in the use sequence).
Created attachment 123200 [details] [review] Bug 17600: Fix t/db_dependent/Members.t Was failing after "Bug 17600: Fix other occurrences in svc"
(In reply to Jonathan Druart from comment #114) > (In reply to Katrin Fischer from comment #108) > > Just wondering about the implications of this as I did some easy rebases > > today. Should we ask for new use statements always having the methods used > > added with qw? Should there be a coding guideline/QA test? > > I've tried to explain in the commit message. > Basically you need to > 1. Use EXPORT_OK instead of EXPORT in modules > 2. either > use Module qw( method_1 method_2 ); > method_1(); > > or > use Module; > Module::method_1(); > Probably you still need to differentiate between Koha modules and external CPAN stuff? The use Module statement does still try to import names. If you do 'use Module ()' with empty list, you do NOT import anymore. Actually, you only 'require'.
(In reply to Jonathan Druart from comment #117) > (In reply to Jonathan Druart from comment #116) > > Created attachment 123196 [details] [review] [review] [review] > > Bug 17600: (follow-up) Fix wrong import in Koha::I18N - LC_MESSAGES > > Can I get a review on that patch please, before I push it and continue to > mess up with things. > > I think this fixes a problem we had prior to this patch (to confirm!). Both > Locale::Messages and POSIX are exporting LC_MESSAGES. As we explicitly > imported LC_MESSAGES from Locale::Messages I guess it's what we wanted to > do, but POSIX exported it as well and it's the one we got (it's after > Locale::Messages in the use sequence). I will try.
Created attachment 123210 [details] [review] Bug 17600: (follow-up) Fix wrong import in Koha::I18N - LC_MESSAGES Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Jonathan Druart from comment #117) > (In reply to Jonathan Druart from comment #116) > > Created attachment 123196 [details] [review] [review] [review] > > Bug 17600: (follow-up) Fix wrong import in Koha::I18N - LC_MESSAGES > > Can I get a review on that patch please, before I push it and continue to > mess up with things. > > I think this fixes a problem we had prior to this patch (to confirm!). Both > Locale::Messages and POSIX are exporting LC_MESSAGES. As we explicitly > imported LC_MESSAGES from Locale::Messages I guess it's what we wanted to > do, but POSIX exported it as well and it's the one we got (it's after > Locale::Messages in the use sequence). Looks good to me now. Have the impression that both constants return the same for me.. But when you look at Locale::Messages, this is the way it should be.
(In reply to Marcel de Rooy from comment #121) > Created attachment 123210 [details] [review] [review] > Bug 17600: (follow-up) Fix wrong import in Koha::I18N - LC_MESSAGES > > Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Patch pushed to master. Thanks, Marcel!
Created attachment 123281 [details] [review] Bug 17600: Fix POSIX imports it fixes xgettext and (maybe) friends [12:22:29] Error: Command failed: misc/translator/xgettext.pl --charset=UTF-8 -s -o /tmp/koha-Jaa9rf/Koha-marc-NORMARC.pot -f /tmp/koha-Jaa9rf/files /tmp/koha-Jaa9rf/Koha-marc-NORMARC.pot at misc/translator/xgettext.pl line 387. Use of uninitialized value in subroutine entry at misc/translator/xgettext.pl line 388. Argument ">:encoding(utf-8)" isn't numeric in subroutine entry at misc/translator/xgettext.pl line 388. Argument "/tmp/koha-Jaa9rf/Koha-marc-NORMARC.pot" isn't numeric in subroutine entry at misc/translator/xgettext.pl line 388.
Last patch pushed to master.
Created attachment 123283 [details] [review] Bug 17600: (follow-up) Import GetAuthority in rebuild_zebra
Created attachment 123284 [details] [review] Bug 17600: Fix missing C4::AuthoritiesMARC import
Last 2 patches pushed to master.
Created attachment 123325 [details] [review] Bug 17600: (follow-up) Fully qualify routine in cancel_expired_holds.pl
Patch pushed to master, thanks Nick!
Created attachment 123417 [details] [review] Bug 17600: Fix GetTagsLabels imports
Created attachment 123418 [details] [review] Bug 17600: Add missing imports in authorities/
Created attachment 123419 [details] [review] Bug 17600: Add missing imports in authorities/
Created attachment 123420 [details] [review] Bug 17600: Add missing imports in authorities/ Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Nick Clemens from comment #135) > Created attachment 123420 [details] [review] [review] > Bug 17600: Add missing imports in authorities/ > > Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Pushed to master.
Created attachment 123565 [details] [review] Bug 17600: Fix missing imports from mappings.pl Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Jonathan Druart from comment #137) > Created attachment 123565 [details] [review] [review] > Bug 17600: Fix missing imports from mappings.pl > > Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Pushed to master.
Created attachment 124866 [details] [review] Bug 17600: Fix opac/svc/overdrive
(In reply to Jonathan Druart from comment #139) > Created attachment 124866 [details] [review] [review] > Bug 17600: Fix opac/svc/overdrive Pushed to master.
Created attachment 125059 [details] [review] Bug 17600: Remove wrong C4::Context imports
(In reply to Jonathan Druart from comment #141) > Created attachment 125059 [details] [review] [review] > Bug 17600: Remove wrong C4::Context imports Pushed to master.
Created attachment 125063 [details] [review] Bug 17600: (follow-up) Fix overdrive proxy
Created attachment 125064 [details] [review] Bug 17600: (follow-up) Fix overdrive_proxy recordedbooks
Created attachment 125066 [details] [review] Bug 17600: Fix overdrive_proxy and recordedbooks
Created attachment 126640 [details] [review] Bug 17600: Fix imports for svc/letters/preview
Last patches pushed to master.
Created attachment 127056 [details] [review] Bug 17600: Fix imports for GetClassSources in guided_reports.pl This will fix the broken option to use cn_source as a runtime parameter, e.g. <<Call no. source|cn_source>>
(In reply to Owen Leonard from comment #148) > Created attachment 127056 [details] [review] [review] > Bug 17600: Fix imports for GetClassSources in guided_reports.pl > > This will fix the broken option to use cn_source as a runtime > parameter, e.g. <<Call no. source|cn_source>> Pushed to master, thanks Owen!
Created attachment 127918 [details] [review] Bug 17600: Fix opac/unapi
(In reply to Jonathan Druart from comment #150) > Created attachment 127918 [details] [review] [review] > Bug 17600: Fix opac/unapi Pushed to master.