After bug 19096, we will need some cleanup. This patch set does only remove obsoleted parameters, so essentially it is a no-op.
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.
Looks like bug 30813 did most of the work now.
(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.
(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. ?
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>
(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.
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>
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>
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>
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>
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>
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>
Created attachment 158249 [details] [review] Bug 19097: (follow-up) Fix typo 'altough' Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Thx Nick. All tough :)
/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.
(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.
(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?
Hum indeed, it passes after a reset_all. Weird I have not played with the mapping this morning.
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>
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>
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>
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>
Another nice piece of overdue cleanup, thanks Marcel. All working well, no regressions spotted and tests working and updated. Passing QA
Pushed for 24.05! Well done everyone, thank you!
Not backported to 23.11.x
I have reviewed. As far as I can tell, there is nothing to document for the manual.
This bug makes Koha_Main_My8 fails: t/db_dependent/Biblio.t .. 1/24 # Failed test 'We expect two hits' # at t/db_dependent/Biblio.t line 147. # got: '1' # expected: '2' # Failed test 'Testing a random column for existence in 2nd hash' # at t/db_dependent/Biblio.t line 149. # Looks like you failed 2 tests of 27. # Failed test 'GetMarcSubfieldStructureFromKohaField' # at t/db_dependent/Biblio.t line 155. t/db_dependent/Biblio.t .. 11/24 # Looks like you failed 1 test of 24. t/db_dependent/Biblio.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/24 subtests Test Summary Report ------------------- t/db_dependent/Biblio.t (Wstat: 256 (exited 1) Tests: 24 Failed: 1) Failed test: 3 Non-zero exit status: 1 Files=1, Tests=24, 8 wallclock secs ( 0.05 usr 0.01 sys + 4.84 cusr 1.81 csys = 6.71 CPU) Result: FAIL This is because jenkins's config for this job has `export TEST_DB_UPGRADE=yes`, which means the test suite will be run after we run the update DB process (and not after a "regular" installation simulated using the installer/onboarding selenium tests) You can easily recreate (not necessarily using My8): 1 koha-mysql kohadev -e "DROP DATABASE koha_kohadev" || true 2 mysql -h db -u koha_kohadev -ppassword -e"CREATE DATABASE koha_kohadev" 3 flush_memcached 4 sudo service apache2 restart 5 sudo service koha-common restart 6 koha-mysql kohadev < /kohadevbox/misc4dev/data/sql/marc21/dump_kohadev_v19.11.00.sql 7 sudo koha-shell kohadev -p -c 'perl /kohadevbox/koha/installer/data/mysql/updatedatabase.pl' Then prove t/db_dependent/Biblio.t
so I guess you need to make sure the mapping exists in DB (and not assuming it's there already)
(In reply to Jonathan Druart from comment #29) > so I guess you need to make sure the mapping exists in DB (and not assuming > it's there already) Well, I can imagine that more tests will fail for that reason? Will take a look
See bug 38513