Bug 17600 - Standardize the EXPORT
Summary: Standardize the EXPORT
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL: https://gitlab.com/joubu/Koha/-/tree/...
Keywords: rel_21_11_candidate, RM_priority
Depends on: 17722 18927
Blocks: 28722 28731 28750 28758 28763
  Show dependency treegraph
 
Reported: 2016-11-09 16:46 UTC by Jonathan Druart
Modified: 2021-08-03 12:56 UTC (History)
11 users (show)

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


Attachments
Bug 17600: Standardize our EXPORT_OK (41.68 KB, patch)
2016-11-09 16:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Standardize our EXPORT_OK (41.52 KB, patch)
2016-12-05 12:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Remove useless extra spaces (2.62 KB, patch)
2016-12-05 12:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: continue replace of EXPORT with EXPORT_OK (22.19 KB, patch)
2016-12-05 12:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Explicitly export the subroutines used (41.73 KB, patch)
2016-12-05 12:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
[DO NOT PUSH] Script to export the subroutines used (3.62 KB, patch)
2016-12-05 12:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Standardize our EXPORT_OK (41.33 KB, patch)
2017-04-10 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Remove useless extra spaces (2.62 KB, patch)
2017-04-10 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: continue replace of EXPORT with EXPORT_OK (21.11 KB, patch)
2017-04-10 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Explicitly export the subroutines used (41.66 KB, patch)
2017-04-10 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Rerun the script to update use statements (7.71 KB, patch)
2017-04-10 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Standardize our EXPORT_OK (41.31 KB, patch)
2017-04-17 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Remove useless extra spaces (2.62 KB, patch)
2017-04-17 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: continue replace of EXPORT with EXPORT_OK (21.11 KB, patch)
2017-04-17 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Explicitly export the subroutines used (41.67 KB, patch)
2017-04-17 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Rerun the script to update use statements (7.71 KB, patch)
2017-04-17 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Standardize our EXPORT_OK (41.38 KB, patch)
2017-05-02 09:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17600: Remove useless extra spaces (2.68 KB, patch)
2017-05-02 09:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17600: continue replace of EXPORT with EXPORT_OK (21.18 KB, patch)
2017-05-02 09:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17600: Explicitly export the subroutines used (41.73 KB, patch)
2017-05-02 09:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17600: Rerun the script to update use statements (7.72 KB, patch)
2017-05-02 14:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: re-Rerun the script to update use statements (4.24 KB, patch)
2017-05-02 14:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Standardize our EXPORT_OK (41.65 KB, patch)
2017-06-16 14:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Remove useless extra spaces (2.74 KB, patch)
2017-06-16 14:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: continue replace of EXPORT with EXPORT_OK (21.38 KB, patch)
2017-06-16 14:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Explicitly export the subroutines used (42.19 KB, patch)
2017-06-16 14:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Rerun the script to update use statements (7.79 KB, patch)
2017-06-16 14:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: re-Rerun the script to update use statements (6.29 KB, patch)
2017-06-16 14:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Standardize our EXPORT_OK (41.75 KB, patch)
2017-06-20 19:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Remove useless extra spaces (2.74 KB, patch)
2017-06-20 19:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: continue replace of EXPORT with EXPORT_OK (21.39 KB, patch)
2017-06-20 19:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Explicitly export the subroutines used (41.98 KB, patch)
2017-06-20 19:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Rerun the script to update use statements (7.79 KB, patch)
2017-06-20 19:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: re-Rerun the script to update use statements (6.29 KB, patch)
2017-06-20 19:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Export subroutine from modules (32.33 KB, patch)
2017-06-20 19:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Update script to export - export for scripts (4.80 KB, patch)
2017-06-20 19:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Explicitly export the subroutines used - scripts (352.12 KB, patch)
2017-06-20 19:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: (follow-up) Explicitly export the subroutines used - scripts (4.53 KB, patch)
2017-06-20 19:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Some more... (3.71 KB, patch)
2017-06-20 19:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Standardize our EXPORT_OK (37.29 KB, patch)
2021-05-24 08:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: After export.pl (362.70 KB, patch)
2021-05-24 08:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: After perlimports (401.70 KB, patch)
2021-05-24 08:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Manual changes (17.08 KB, patch)
2021-05-24 08:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: after perlimports pm (276.51 KB, patch)
2021-05-24 08:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Other manual changes after second perlimports run (3.91 KB, patch)
2021-05-24 08:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Standardize our EXPORT_OK (36.95 KB, patch)
2021-06-24 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: After export.pl (362.71 KB, patch)
2021-06-24 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: After perlimports (397.42 KB, patch)
2021-06-24 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Manual changes (17.36 KB, patch)
2021-06-24 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: after perlimports pm (270.08 KB, patch)
2021-06-24 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Other manual changes after second perlimports run (3.91 KB, patch)
2021-06-24 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Standardize our EXPORT_OK (785.79 KB, patch)
2021-07-16 06:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Fix tests (6.40 KB, patch)
2021-07-16 11:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Fix batchMod.pl (944 bytes, patch)
2021-07-19 14:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Fix other occurrences in svc (3.14 KB, patch)
2021-07-26 14:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Fix wrong import in Koha::I18N - LC_MESSAGES (797 bytes, patch)
2021-07-26 14:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: (follow-up) Fix wrong import in Koha::I18N - LC_MESSAGES (736 bytes, patch)
2021-07-26 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Fix t/db_dependent/Members.t (797 bytes, patch)
2021-07-26 17:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: (follow-up) Fix wrong import in Koha::I18N - LC_MESSAGES (831 bytes, patch)
2021-07-27 12:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17600: Fix POSIX imports (3.96 KB, patch)
2021-07-29 12:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: (follow-up) Import GetAuthority in rebuild_zebra (883 bytes, patch)
2021-07-29 13:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17600: Fix missing C4::AuthoritiesMARC import (4.26 KB, patch)
2021-07-29 13:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: (follow-up) Fully qualify routine in cancel_expired_holds.pl (743 bytes, patch)
2021-07-30 13:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17600: Fix GetTagsLabels imports (4.86 KB, patch)
2021-08-03 12:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Add missing imports in authorities/ (1.54 KB, patch)
2021-08-03 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Add missing imports in authorities/ (2.06 KB, patch)
2021-08-03 12:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17600: Add missing imports in authorities/ (2.11 KB, patch)
2021-08-03 12:54 UTC, Nick Clemens
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-11-09 16:46:23 UTC
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.
Comment 1 Jonathan Druart 2016-11-09 16:48:38 UTC
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.
Comment 2 Kyle M Hall 2016-11-10 11:18:38 UTC
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.
Comment 3 Marcel de Rooy 2016-11-10 11:54:41 UTC
(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.
Comment 4 Jonathan Druart 2016-11-10 12:18:12 UTC
(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.
Comment 5 Martin Renvoize 2016-11-11 07:34:14 UTC
+1 from me, this would be a great move forward for Koha :)
Comment 6 Mark Tompsett 2016-11-21 20:25:49 UTC
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?
Comment 7 Jonathan Druart 2016-11-22 07:12:54 UTC
(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.
Comment 8 Jonathan Druart 2016-12-05 12:13:58 UTC
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.
Comment 9 Jonathan Druart 2016-12-05 12:14:02 UTC
Created attachment 57951 [details] [review]
Bug 17600: Remove useless extra spaces
Comment 10 Jonathan Druart 2016-12-05 12:14:06 UTC
Created attachment 57952 [details] [review]
Bug 17600: continue replace of EXPORT with EXPORT_OK
Comment 11 Jonathan Druart 2016-12-05 12:14:10 UTC
Created attachment 57953 [details] [review]
Bug 17600: Explicitly export the subroutines used

This patch is generated using the export.pl script
Comment 12 Jonathan Druart 2016-12-05 12:14:13 UTC
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
Comment 13 Jonathan Druart 2016-12-05 12:45:57 UTC
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.
Comment 14 Jonathan Druart 2016-12-05 14:15:13 UTC
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?
Comment 15 Mark Tompsett 2017-01-09 14:02:13 UTC
(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.
Comment 16 Jonathan Druart 2017-03-21 19:22:08 UTC
(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();
Comment 17 Jonathan Druart 2017-03-21 19:23:01 UTC
Put in QA queue to get feedbacks from QAer. More than 2 signoffs from QA team members would be great.
Comment 18 Marcel de Rooy 2017-03-22 12:26:53 UTC
(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.
Comment 19 Martin Renvoize 2017-03-24 09:14:33 UTC
This one is also on my list for today :)
Comment 20 Martin Renvoize 2017-04-10 08:56:00 UTC
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?
Comment 21 Jonathan Druart 2017-04-10 12:09:20 UTC
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.
Comment 22 Jonathan Druart 2017-04-10 12:09:24 UTC
Created attachment 61995 [details] [review]
Bug 17600: Remove useless extra spaces
Comment 23 Jonathan Druart 2017-04-10 12:09:28 UTC
Created attachment 61996 [details] [review]
Bug 17600: continue replace of EXPORT with EXPORT_OK
Comment 24 Jonathan Druart 2017-04-10 12:09:33 UTC
Created attachment 61997 [details] [review]
Bug 17600: Explicitly export the subroutines used

This patch is generated using the export.pl script
Comment 25 Jonathan Druart 2017-04-10 12:09:38 UTC
Created attachment 61998 [details] [review]
Bug 17600: Rerun the script to update use statements
Comment 26 Marcel de Rooy 2017-04-14 06:10:01 UTC
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.
Comment 27 Jonathan Druart 2017-04-17 12:39:08 UTC
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.
Comment 28 Jonathan Druart 2017-04-17 12:39:14 UTC
Created attachment 62210 [details] [review]
Bug 17600: Remove useless extra spaces
Comment 29 Jonathan Druart 2017-04-17 12:39:18 UTC
Created attachment 62211 [details] [review]
Bug 17600: continue replace of EXPORT with EXPORT_OK
Comment 30 Jonathan Druart 2017-04-17 12:39:23 UTC
Created attachment 62212 [details] [review]
Bug 17600: Explicitly export the subroutines used

This patch is generated using the export.pl script
Comment 31 Jonathan Druart 2017-04-17 12:39:27 UTC
Created attachment 62213 [details] [review]
Bug 17600: Rerun the script to update use statements
Comment 32 Martin Renvoize 2017-04-28 15:00:06 UTC
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 ;)
Comment 33 Marcel de Rooy 2017-05-01 06:37:34 UTC
(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.
Comment 34 Martin Renvoize 2017-05-02 09:36:06 UTC
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>
Comment 35 Martin Renvoize 2017-05-02 09:36:10 UTC
Created attachment 62941 [details] [review]
Bug 17600: Remove useless extra spaces

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Martin Renvoize 2017-05-02 09:36:13 UTC
Created attachment 62942 [details] [review]
Bug 17600: continue replace of EXPORT with EXPORT_OK

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Martin Renvoize 2017-05-02 09:36:17 UTC
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>
Comment 38 Martin Renvoize 2017-05-02 09:36:40 UTC
I have now ;)
Comment 39 Marcel de Rooy 2017-05-02 12:17:04 UTC
(In reply to Martin Renvoize from comment #38)
> I have now ;)

Great, Martin. Thanks.
Comment 40 Marcel de Rooy 2017-05-02 12:23:16 UTC
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?)
Comment 41 Jonathan Druart 2017-05-02 14:38:23 UTC
Created attachment 62958 [details] [review]
Bug 17600: Rerun the script to update use statements
Comment 42 Jonathan Druart 2017-05-02 14:38:28 UTC
Created attachment 62959 [details] [review]
Bug 17600: re-Rerun the script to update use statements
Comment 43 Jonathan Druart 2017-05-02 14:39:06 UTC
(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.
Comment 44 Marcel de Rooy 2017-05-02 14:57:04 UTC
(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.
Comment 45 Jonathan Druart 2017-05-02 21:37:18 UTC
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.
Comment 46 Marcel de Rooy 2017-05-03 11:18:47 UTC
(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.
Comment 47 Marcel de Rooy 2017-06-14 14:47:56 UTC
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..
Comment 48 Jonathan Druart 2017-06-16 14:16:34 UTC
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>
Comment 49 Jonathan Druart 2017-06-16 14:16:39 UTC
Created attachment 64388 [details] [review]
Bug 17600: Remove useless extra spaces

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 50 Jonathan Druart 2017-06-16 14:16:44 UTC
Created attachment 64389 [details] [review]
Bug 17600: continue replace of EXPORT with EXPORT_OK

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 Jonathan Druart 2017-06-16 14:16:49 UTC
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>
Comment 52 Jonathan Druart 2017-06-16 14:16:55 UTC
Created attachment 64391 [details] [review]
Bug 17600: Rerun the script to update use statements
Comment 53 Jonathan Druart 2017-06-16 14:17:01 UTC
Created attachment 64392 [details] [review]
Bug 17600: re-Rerun the script to update use statements
Comment 54 Jonathan Druart 2017-06-16 14:48:38 UTC
Too many errors "subroutine is not exported by the C4::XXX module"
Comment 55 Jonathan Druart 2017-06-20 19:31:04 UTC
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>
Comment 56 Jonathan Druart 2017-06-20 19:31:20 UTC
Created attachment 64467 [details] [review]
Bug 17600: Remove useless extra spaces

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 57 Jonathan Druart 2017-06-20 19:31:32 UTC
Created attachment 64468 [details] [review]
Bug 17600: continue replace of EXPORT with EXPORT_OK

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 58 Jonathan Druart 2017-06-20 19:31:38 UTC
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>
Comment 59 Jonathan Druart 2017-06-20 19:31:46 UTC
Created attachment 64470 [details] [review]
Bug 17600: Rerun the script to update use statements
Comment 60 Jonathan Druart 2017-06-20 19:31:54 UTC
Created attachment 64471 [details] [review]
Bug 17600: re-Rerun the script to update use statements
Comment 61 Jonathan Druart 2017-06-20 19:32:09 UTC
Created attachment 64472 [details] [review]
Bug 17600: Export subroutine from modules
Comment 62 Jonathan Druart 2017-06-20 19:32:18 UTC
Created attachment 64473 [details] [review]
Update script to export - export for scripts
Comment 63 Jonathan Druart 2017-06-20 19:32:35 UTC
Created attachment 64474 [details] [review]
Bug 17600: Explicitly export the subroutines used - scripts
Comment 64 Jonathan Druart 2017-06-20 19:32:40 UTC
Created attachment 64475 [details] [review]
Bug 17600: (follow-up) Explicitly export the subroutines used - scripts

manual adjustements
Comment 65 Jonathan Druart 2017-06-20 19:40:59 UTC
Created attachment 64476 [details] [review]
Bug 17600: Some more...
Comment 66 Jonathan Druart 2017-06-20 19:42:30 UTC
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.
Comment 67 Marcel de Rooy 2017-06-30 08:14:22 UTC
-   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 ??
Comment 68 Marcel de Rooy 2017-06-30 08:14:53 UTC
Auth_with_ldap too btw
Comment 69 Marcel de Rooy 2017-06-30 08:17:53 UTC
Well, it seems to be git or git bz that mangles those files. Reapplying patch 0008 triggers the same problem with me.
Comment 70 Marcel de Rooy 2017-06-30 08:19:56 UTC
Just noting that these patches are in git 2.11.0 and git 2.1.4 apparently does not like them. Too bad :)
Comment 71 Marcel de Rooy 2017-06-30 08:21:39 UTC
Can we rearrange these patches too?
All patches that should not be pushed together? 0009?
All patches for modules and for scripts or so ?
Comment 72 Jonathan Druart 2017-07-06 19:20:46 UTC
(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?
Comment 73 Jonathan Druart 2017-07-06 19:37:37 UTC
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.
Comment 74 Marcel de Rooy 2017-07-07 06:16:37 UTC
(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 :)
Comment 75 Jonathan Druart 2017-08-30 17:35:56 UTC
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?
Comment 76 Julian Maurice 2017-09-01 14:57:50 UTC
(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.
Comment 77 Kyle M Hall 2017-09-01 16:10:47 UTC
Not sure what's going on here
Comment 78 Jonathan Druart 2017-09-06 15:27:25 UTC
(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.
Comment 79 Kyle M Hall 2017-10-27 14:36:57 UTC
Where did the patches go?
Comment 80 Jonathan Druart 2017-10-27 15:21:48 UTC
(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.
Comment 81 Katrin Fischer 2018-02-17 23:08:00 UTC
Hi Jonathan,
could you please rebase your branch on current master? Thx!
Comment 82 Jonathan Druart 2018-02-18 17:41:51 UTC
(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.
Comment 83 David Cook 2018-10-25 00:40:43 UTC
(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.
Comment 84 David Cook 2018-10-25 00:46:16 UTC
(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...
Comment 85 Jonathan Druart 2021-05-24 08:39:32 UTC
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.
Comment 86 Jonathan Druart 2021-05-24 08:40:11 UTC
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.
Comment 87 Jonathan Druart 2021-05-24 08:40:17 UTC
Created attachment 121310 [details] [review]
Bug 17600: After export.pl
Comment 88 Jonathan Druart 2021-05-24 08:40:25 UTC
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 {} \;
Comment 89 Jonathan Druart 2021-05-24 08:40:30 UTC
Created attachment 121312 [details] [review]
Bug 17600: Manual changes
Comment 90 Jonathan Druart 2021-05-24 08:40:37 UTC
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
Comment 91 Jonathan Druart 2021-05-24 08:40:43 UTC
Created attachment 121314 [details] [review]
Bug 17600: Other manual changes after second perlimports run
Comment 92 Jonathan Druart 2021-06-24 09:00:07 UTC
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.
Comment 93 Jonathan Druart 2021-06-24 09:00:16 UTC
Created attachment 122361 [details] [review]
Bug 17600: After export.pl
Comment 94 Jonathan Druart 2021-06-24 09:00:23 UTC
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 {} \;
Comment 95 Jonathan Druart 2021-06-24 09:00:29 UTC
Created attachment 122363 [details] [review]
Bug 17600: Manual changes
Comment 96 Jonathan Druart 2021-06-24 09:00:37 UTC
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
Comment 97 Jonathan Druart 2021-06-24 09:00:42 UTC
Created attachment 122365 [details] [review]
Bug 17600: Other manual changes after second perlimports run
Comment 98 Jonathan Druart 2021-06-24 09:13:13 UTC
Please test the remote branch - https://gitlab.com/joubu/Koha/-/tree/bug_17600_new
Comment 99 Martin Renvoize 2021-06-25 10:11:09 UTC
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!
Comment 100 Martin Renvoize 2021-06-28 13:35:51 UTC
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 :(
Comment 101 Jonathan Druart 2021-07-05 14:40:53 UTC
(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.
Comment 102 Jonathan Druart 2021-07-08 14:25:00 UTC
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
Comment 103 Jonathan Druart 2021-07-16 06:58:14 UTC
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>
Comment 104 Jonathan Druart 2021-07-16 11:54:59 UTC
Patch pushed to master for 21.11.00
Comment 105 Jonathan Druart 2021-07-16 11:55:46 UTC
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>
Comment 106 Jonathan Druart 2021-07-19 14:40:12 UTC
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>
Comment 107 Jonathan Druart 2021-07-19 14:46:39 UTC
Follow-ups have been pushed to master.
Comment 108 Katrin Fischer 2021-07-25 01:31:53 UTC
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?
Comment 109 Marcel de Rooy 2021-07-26 09:06:45 UTC
[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 ?
Comment 110 Jonathan Druart 2021-07-26 14:43:46 UTC
Created attachment 123194 [details] [review]
Bug 17600: Fix other occurrences in svc
Comment 111 Jonathan Druart 2021-07-26 14:45:43 UTC
(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.
Comment 112 Jonathan Druart 2021-07-26 14:49:59 UTC
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
Comment 113 Jonathan Druart 2021-07-26 14:50:26 UTC
(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.
Comment 114 Jonathan Druart 2021-07-26 14:55:03 UTC
(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.
Comment 115 Jonathan Druart 2021-07-26 15:18:58 UTC
(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.
Comment 116 Jonathan Druart 2021-07-26 15:21:49 UTC
Created attachment 123196 [details] [review]
Bug 17600: (follow-up) Fix wrong import in Koha::I18N - LC_MESSAGES
Comment 117 Jonathan Druart 2021-07-26 15:24:36 UTC
(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).
Comment 118 Jonathan Druart 2021-07-26 17:35:33 UTC
Created attachment 123200 [details] [review]
Bug 17600: Fix t/db_dependent/Members.t

Was failing after "Bug 17600: Fix other occurrences in svc"
Comment 119 Marcel de Rooy 2021-07-27 11:11:15 UTC
(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'.
Comment 120 Marcel de Rooy 2021-07-27 11:11:38 UTC
(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.
Comment 121 Marcel de Rooy 2021-07-27 12:14:38 UTC
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>
Comment 122 Marcel de Rooy 2021-07-27 12:16:44 UTC
(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.
Comment 123 Jonathan Druart 2021-07-27 12:50:05 UTC
(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!
Comment 124 Jonathan Druart 2021-07-29 12:37:08 UTC
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.
Comment 125 Jonathan Druart 2021-07-29 12:38:44 UTC
Last patch pushed to master.
Comment 126 Nick Clemens 2021-07-29 13:15:28 UTC
Created attachment 123283 [details] [review]
Bug 17600: (follow-up) Import GetAuthority in rebuild_zebra
Comment 127 Jonathan Druart 2021-07-29 13:34:42 UTC
Created attachment 123284 [details] [review]
Bug 17600: Fix missing C4::AuthoritiesMARC import
Comment 128 Jonathan Druart 2021-07-29 13:36:06 UTC
Last 2 patches pushed to master.
Comment 129 Nick Clemens 2021-07-30 13:20:20 UTC
Created attachment 123325 [details] [review]
Bug 17600: (follow-up) Fully qualify routine in cancel_expired_holds.pl
Comment 130 Jonathan Druart 2021-07-30 14:02:20 UTC
Patch pushed to master, thanks Nick!
Comment 131 Jonathan Druart 2021-08-03 12:36:31 UTC
Created attachment 123417 [details] [review]
Bug 17600: Fix GetTagsLabels imports
Comment 132 Jonathan Druart 2021-08-03 12:37:02 UTC
Last patch pushed to master.
Comment 133 Jonathan Druart 2021-08-03 12:47:23 UTC
Created attachment 123418 [details] [review]
Bug 17600: Add missing imports in authorities/
Comment 134 Jonathan Druart 2021-08-03 12:51:34 UTC
Created attachment 123419 [details] [review]
Bug 17600: Add missing imports in authorities/
Comment 135 Nick Clemens 2021-08-03 12:54:43 UTC
Created attachment 123420 [details] [review]
Bug 17600: Add missing imports in authorities/

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 136 Jonathan Druart 2021-08-03 12:56:01 UTC
(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.