Bug 19097

Summary: Koha to MARC mappings (Part 3): Correct remaining GetMarcFromKohaField calls
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Marcel de Rooy <m.de.rooy>
Status: Passed QA --- QA Contact: Martin Renvoize <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, martin.renvoize, nick
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10306
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 19096, 22721    
Bug Blocks:    
Attachments: Bug 19097: GetMarcSubfieldStructureFromKohaField - remove wantarray
Bug 19097: Remove wantarray from GetMarcSubfieldStructureFromKohaField
Bug 19097: Remove wantarray from GetMarcFromKohaField
Bug 19097: Adjust some GetMarcFromKohaField calls
Bug 19097: Remove wantarray from GetMarcSubfieldStructureFromKohaField
Bug 19097: Remove wantarray from GetMarcFromKohaField
Bug 19097: Adjust some GetMarcFromKohaField calls
Bug 19097: (follow-up) Fix typo 'altough'
Bug 19097: Remove wantarray from GetMarcSubfieldStructureFromKohaField
Bug 19097: Remove wantarray from GetMarcFromKohaField
Bug 19097: Adjust some GetMarcFromKohaField calls
Bug 19097: (follow-up) Fix typo 'altough'

Description Marcel de Rooy 2017-08-14 07:45:05 UTC

    
Comment 1 Marcel de Rooy 2017-08-14 11:38:41 UTC
After bug 19096, we will need some cleanup. This patch set does only remove obsoleted parameters, so essentially it is a no-op.
Comment 2 Marcel de Rooy 2017-10-30 10:59:14 UTC
Moving this QA comment on bug 19096 about sub GetMarcFromKohaField here also:

return wantarray ? @retval : ( @retval ? $retval[0] : undef ); 
This is not necessary, do not do that please.
Caller can do
my ( $v ) = the_sub();
Not blocker.
Comment 3 Marcel de Rooy 2022-06-07 13:05:51 UTC
Looks like bug 30813 did most of the work now.
Comment 4 Marcel de Rooy 2022-06-07 13:32:48 UTC
(In reply to Marcel de Rooy from comment #3)
> Looks like bug 30813 did most of the work now.

But not all. Will make it more concrete.
Comment 5 Katrin Fischer 2023-09-30 19:41:26 UTC
(In reply to Marcel de Rooy from comment #4)
> (In reply to Marcel de Rooy from comment #3)
> > Looks like bug 30813 did most of the work now.
> 
> But not all. Will make it more concrete.

?
Comment 6 Marcel de Rooy 2023-10-30 13:34:01 UTC
Created attachment 158063 [details] [review]
Bug 19097: GetMarcSubfieldStructureFromKohaField - remove wantarray

Replacing wantarray by a list parameter.
Adding comment to calls where simply one return is assumed.

Test plan:
Look at results of git grep GetMarcSubfieldStructureFromKohaField
Run t/db_dependent/Biblio.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2023-10-30 13:35:03 UTC
(In reply to Katrin Fischer from comment #5)
> (In reply to Marcel de Rooy from comment #4)
> > (In reply to Marcel de Rooy from comment #3)
> > > Looks like bug 30813 did most of the work now.
> > 
> > But not all. Will make it more concrete.
> 
> ?

First patch addresses sub GetMarcSubfieldStructureFromKohaField.
Still another follow-up needed for GetMarcFromKohaField.
Comment 8 Marcel de Rooy 2023-11-02 07:44:20 UTC
Created attachment 158231 [details] [review]
Bug 19097: Remove wantarray from GetMarcSubfieldStructureFromKohaField

Replacing wantarray by always returning all mappings.
In a few cases only we expect multiple ones.
Changing two calls to pick the first hit, and add comment about
the implicit assumption being made (as before, no behavior change).

Test plan:
Look at results of git grep GetMarcSubfieldStructureFromKohaField
Run t/db_dependent/Biblio.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2023-11-02 07:44:22 UTC
Created attachment 158232 [details] [review]
Bug 19097: Remove wantarray from GetMarcFromKohaField

The routine should be called in list context now.
Warns also about use of obsoleted framework parameter. This
is the case for several years already btw. But may help us
catch a forgotten occurrence? Can be removed later.

In a follow-up we will check for second parameters and
we will update calls in scalar context.

Test plan:
Prove t/db_dependent/Biblio.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2023-11-02 07:44:24 UTC
Created attachment 158233 [details] [review]
Bug 19097: Adjust some GetMarcFromKohaField calls

We need to:
[1] Remove obsolete framework parameters if any (yes there are).
[2] Check scalar context calls if any (most probably not).

Ad 1:
How to find them? Git grep and look at each hit. You will need the -C
parameter to add a few context lines around the call even.

Found one in C4/Biblio.pm and six in Suggestions.t with ''.
Found one in C4/Items on a second line. Similar one in Koha/EDI.
Note: sub receipt_items in EDI has no unit tests yet.

Also did a few git searches like:
git grep GetMarcFromKohaField | grep -v -P "GetMarcFromKohaField.*;"| grep -P "GetMarcFromKohaField\s*\("
Note: Seeing still three lines; they are all fine. Last line comes from
testing to trigger the warn.

Ad 2:
Have been looking at each git grep line and did not spot any
scalar context call. Before this report, we had an example in
POD and the Biblio test only.

Try to find them by performing smarter git searches?
Did look again at results for:
git grep GetMarcFromKohaField | grep -v -P "\)\s*=\s*&?(C4::Biblio::)?GetMarcFromKohaField" | grep -v -P "use "
The tests from this grep are included in the last test plan line.

Test plan:
Read the patch.
Run t/db_dependent/Biblio/TransformHtmlToMarc.t
Run t/db_dependent/Items.t
Run t/db_dependent/Suggestions.t
Run prove t/Biblio2.t t/db_dependent/Biblio.t t/db_dependent/Filter_MARC_ViewPolicy.t t/db_dependent/Koha/Authorities.t t/db_dependent/Koha/Biblio/Metadata.t t/db_dependent/Koha/Filter/EmbedItemsAvailability.t t/db_dependent/Koha/SearchEngine/Search.t t/db_dependent/XISBN.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Nick Clemens 2023-11-02 11:17:07 UTC
Created attachment 158246 [details] [review]
Bug 19097: Remove wantarray from GetMarcSubfieldStructureFromKohaField

Replacing wantarray by always returning all mappings.
In a few cases only we expect multiple ones.
Changing two calls to pick the first hit, and add comment about
the implicit assumption being made (as before, no behavior change).

Test plan:
Look at results of git grep GetMarcSubfieldStructureFromKohaField
Run t/db_dependent/Biblio.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens 2023-11-02 11:17:09 UTC
Created attachment 158247 [details] [review]
Bug 19097: Remove wantarray from GetMarcFromKohaField

The routine should be called in list context now.
Warns also about use of obsoleted framework parameter. This
is the case for several years already btw. But may help us
catch a forgotten occurrence? Can be removed later.

In a follow-up we will check for second parameters and
we will update calls in scalar context.

Test plan:
Prove t/db_dependent/Biblio.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Nick Clemens 2023-11-02 11:17:11 UTC
Created attachment 158248 [details] [review]
Bug 19097: Adjust some GetMarcFromKohaField calls

We need to:
[1] Remove obsolete framework parameters if any (yes there are).
[2] Check scalar context calls if any (most probably not).

Ad 1:
How to find them? Git grep and look at each hit. You will need the -C
parameter to add a few context lines around the call even.

Found one in C4/Biblio.pm and six in Suggestions.t with ''.
Found one in C4/Items on a second line. Similar one in Koha/EDI.
Note: sub receipt_items in EDI has no unit tests yet.

Also did a few git searches like:
git grep GetMarcFromKohaField | grep -v -P "GetMarcFromKohaField.*;"| grep -P "GetMarcFromKohaField\s*\("
Note: Seeing still three lines; they are all fine. Last line comes from
testing to trigger the warn.

Ad 2:
Have been looking at each git grep line and did not spot any
scalar context call. Before this report, we had an example in
POD and the Biblio test only.

Try to find them by performing smarter git searches?
Did look again at results for:
git grep GetMarcFromKohaField | grep -v -P "\)\s*=\s*&?(C4::Biblio::)?GetMarcFromKohaField" | grep -v -P "use "
The tests from this grep are included in the last test plan line.

Test plan:
Read the patch.
Run t/db_dependent/Biblio/TransformHtmlToMarc.t
Run t/db_dependent/Items.t
Run t/db_dependent/Suggestions.t
Run prove t/Biblio2.t t/db_dependent/Biblio.t t/db_dependent/Filter_MARC_ViewPolicy.t t/db_dependent/Koha/Authorities.t t/db_dependent/Koha/Biblio/Metadata.t t/db_dependent/Koha/Filter/EmbedItemsAvailability.t t/db_dependent/Koha/SearchEngine/Search.t t/db_dependent/XISBN.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens 2023-11-02 11:17:13 UTC
Created attachment 158249 [details] [review]
Bug 19097: (follow-up) Fix typo 'altough'

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Marcel de Rooy 2023-11-02 12:36:22 UTC
Thx Nick. All tough :)
Comment 16 Jonathan Druart 2024-01-25 11:07:49 UTC
/kohadevbox/koha/t/db_dependent/Biblio.t .. 2/20 
    #   Failed test 'We expect two hits'
    #   at /kohadevbox/koha/t/db_dependent/Biblio.t line 127.
    #          got: '1'
    #     expected: '2'

    #   Failed test 'Testing a random column for existence in 2nd hash'
    #   at /kohadevbox/koha/t/db_dependent/Biblio.t line 129.
    # Looks like you failed 2 tests of 27.

#   Failed test 'GetMarcSubfieldStructureFromKohaField'
#   at /kohadevbox/koha/t/db_dependent/Biblio.t line 135.
Comment 17 Marcel de Rooy 2024-01-25 12:34:39 UTC
(In reply to Jonathan Druart from comment #16)
> /kohadevbox/koha/t/db_dependent/Biblio.t .. 2/20 
>     #   Failed test 'We expect two hits'
>     #   at /kohadevbox/koha/t/db_dependent/Biblio.t line 127.
>     #          got: '1'
>     #     expected: '2'
> 
>     #   Failed test 'Testing a random column for existence in 2nd hash'
>     #   at /kohadevbox/koha/t/db_dependent/Biblio.t line 129.
>     # Looks like you failed 2 tests of 27.
> 
> #   Failed test 'GetMarcSubfieldStructureFromKohaField'
> #   at /kohadevbox/koha/t/db_dependent/Biblio.t line 135.

Hmm. Passes with me.
Comment 18 Marcel de Rooy 2024-01-25 12:36:48 UTC
(In reply to Marcel de Rooy from comment #17)
> (In reply to Jonathan Druart from comment #16)
> > /kohadevbox/koha/t/db_dependent/Biblio.t .. 2/20 
> >     #   Failed test 'We expect two hits'
> >     #   at /kohadevbox/koha/t/db_dependent/Biblio.t line 127.
> >     #          got: '1'
> >     #     expected: '2'
> > 
> >     #   Failed test 'Testing a random column for existence in 2nd hash'
> >     #   at /kohadevbox/koha/t/db_dependent/Biblio.t line 129.
> >     # Looks like you failed 2 tests of 27.
> > 
> > #   Failed test 'GetMarcSubfieldStructureFromKohaField'
> > #   at /kohadevbox/koha/t/db_dependent/Biblio.t line 135.
> 
> Hmm. Passes with me.

This test assumes multiple mappings for kohafield biblio.copyrightdate (in 260, 264) as present in default install.
Could you check your data?
Obviously, the test could add them too?
Comment 19 Jonathan Druart 2024-01-25 12:49:35 UTC
Hum indeed, it passes after a reset_all. Weird I have not played with the mapping this morning.
Comment 20 Martin Renvoize 2024-04-18 13:43:37 UTC
Created attachment 165103 [details] [review]
Bug 19097: Remove wantarray from GetMarcSubfieldStructureFromKohaField

Replacing wantarray by always returning all mappings.
In a few cases only we expect multiple ones.
Changing two calls to pick the first hit, and add comment about
the implicit assumption being made (as before, no behavior change).

Test plan:
Look at results of git grep GetMarcSubfieldStructureFromKohaField
Run t/db_dependent/Biblio.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2024-04-18 13:43:40 UTC
Created attachment 165104 [details] [review]
Bug 19097: Remove wantarray from GetMarcFromKohaField

The routine should be called in list context now.
Warns also about use of obsoleted framework parameter. This
is the case for several years already btw. But may help us
catch a forgotten occurrence? Can be removed later.

In a follow-up we will check for second parameters and
we will update calls in scalar context.

Test plan:
Prove t/db_dependent/Biblio.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2024-04-18 13:43:42 UTC
Created attachment 165105 [details] [review]
Bug 19097: Adjust some GetMarcFromKohaField calls

We need to:
[1] Remove obsolete framework parameters if any (yes there are).
[2] Check scalar context calls if any (most probably not).

Ad 1:
How to find them? Git grep and look at each hit. You will need the -C
parameter to add a few context lines around the call even.

Found one in C4/Biblio.pm and six in Suggestions.t with ''.
Found one in C4/Items on a second line. Similar one in Koha/EDI.
Note: sub receipt_items in EDI has no unit tests yet.

Also did a few git searches like:
git grep GetMarcFromKohaField | grep -v -P "GetMarcFromKohaField.*;"| grep -P "GetMarcFromKohaField\s*\("
Note: Seeing still three lines; they are all fine. Last line comes from
testing to trigger the warn.

Ad 2:
Have been looking at each git grep line and did not spot any
scalar context call. Before this report, we had an example in
POD and the Biblio test only.

Try to find them by performing smarter git searches?
Did look again at results for:
git grep GetMarcFromKohaField | grep -v -P "\)\s*=\s*&?(C4::Biblio::)?GetMarcFromKohaField" | grep -v -P "use "
The tests from this grep are included in the last test plan line.

Test plan:
Read the patch.
Run t/db_dependent/Biblio/TransformHtmlToMarc.t
Run t/db_dependent/Items.t
Run t/db_dependent/Suggestions.t
Run prove t/Biblio2.t t/db_dependent/Biblio.t t/db_dependent/Filter_MARC_ViewPolicy.t t/db_dependent/Koha/Authorities.t t/db_dependent/Koha/Biblio/Metadata.t t/db_dependent/Koha/Filter/EmbedItemsAvailability.t t/db_dependent/Koha/SearchEngine/Search.t t/db_dependent/XISBN.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2024-04-18 13:43:44 UTC
Created attachment 165106 [details] [review]
Bug 19097: (follow-up) Fix typo 'altough'

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2024-04-18 13:46:17 UTC
Another nice piece of overdue cleanup, thanks Marcel.

All working well, no regressions spotted and tests working and updated.

Passing QA