Bug 27161

Summary: Base.pm complains about unitialized value
Product: Koha Reporter: Alvaro Cornejo <cornejo.alvaro>
Component: Architecture, internals, and plumbingAssignee: Alvaro Cornejo <cornejo.alvaro>
Status: RESOLVED DUPLICATE QA Contact: Testopia <testopia>
Severity: trivial    
Priority: P5 - low CC: cornejo.alvaro, david, fridolin.somers, jonathan.druart, martin.renvoize
Version: 19.11   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 25790    
Attachments: test patch
Base.pm patch to fix uninitialize variable error

Description Alvaro Cornejo 2020-12-07 05:06:51 UTC
I get several errors on 

 Use of uninitialized value in subroutine entry at /usr/share/koha/lib/Koha/Plugins/Base.pm line 182.

177 sub get_metadata {
178     my ( $self, $args ) = @_;
179
180     #FIXME: Why another encoding issue? For metadata containing non latin characters.
181     my $metadata = $self->{metadata};
182         utf8::decode($metadata->{$_}) for keys %$metadata;
183     return $metadata;
184 }


Verifying that $metadata value is set before applying utf8::decode will avoid that log error

177 sub get_metadata {
178     my ( $self, $args ) = @_;
179
180     #FIXME: Why another encoding issue? For metadata containing non latin characters.
181     my $metadata = $self->{metadata};
182     if ($metadata !~ "") {
183         utf8::decode($metadata->{$_}) for keys %$metadata;
184         }
185     return $metadata;
186 }
Comment 1 Jonathan Druart 2020-12-09 13:18:14 UTC
Could you submit a patch?
Comment 2 Alvaro Cornejo 2020-12-10 03:20:43 UTC
Created attachment 114295 [details] [review]
test patch

Hope is OK.

It does not fully avoid the "uninitialized value in subroutine" error but greatly reduces the number of errors.

Patch against 19.11.11
Comment 3 Alvaro Cornejo 2020-12-10 03:40:30 UTC
Comment on attachment 114295 [details] [review]
test patch

>--- Base.pm.orig	2020-12-09 22:11:25.639834095 -0500
>+++ Base.pm	2020-12-07 19:41:17.903081236 -0500
>@@ -179,7 +179,9 @@
> 
>     #FIXME: Why another encoding issue? For metadata containing non latin characters.
>     my $metadata = $self->{metadata};
>+    if ( $metadata  & $metadata !~ "")  {
> 	utf8::decode($metadata->{$_}) for keys %$metadata;
>+	}
>     return $metadata;
> }
>
Comment 4 Jonathan Druart 2020-12-10 09:30:45 UTC
Hello Alvaro, you patch does not follow our guidelines (not git formatted).

https://wiki.koha-community.org/wiki/Submitting_A_Patch

Could you follow those steps?
Comment 5 Alvaro Cornejo 2020-12-12 04:17:53 UTC
Created attachment 114360 [details] [review]
Base.pm patch to fix uninitialize variable error

From 32bfd2fe945e7aef6f489ddaf1c80be018622663 Mon Sep 17 00:00:00 2001
From: Alvaro Cornejo <cornejo.alvaro@gmail.com>
Date: Fri, 11 Dec 2020 23:02:06 -0500
Subject: [PATCH] Bug 27161: Base.pm complains about unitialized value

This patch does validate that $metadata exists and is set in sub get_metadata

TEST PLAN
Before applying the patch
tail plack error log and see errors like:

Use of uninitialized value in subroutine entry at line 182 in Base.pm

Apply the patch

restart koha-common service

tail plack error log and verify no more erros appear.
Comment 6 Fridolin Somers 2020-12-16 15:35:53 UTC
  if ( $metadata  & $metadata !~ "")

second test is useless I think, 
  if ($metadata) with skip undefined, empty string and 0
Comment 7 Alvaro Cornejo 2020-12-19 03:40:10 UTC
(In reply to Fridolin Somers from comment #6)
>   if ( $metadata  & $metadata !~ "")
> 
> second test is useless I think, 
>   if ($metadata) with skip undefined, empty string and 0

Yes, I thought that but with only one of the tests there are still "undefined" variable errors.

Not sure why, but with both tests error does not show anymore
Comment 8 Martin Renvoize 2021-03-01 10:11:54 UTC
I'm wondering if we ought to instead define a default..

i.e.

my $metadata = $self->{metadata};

vs

my $metadata = $self->{metadata} // {};

?
Comment 9 David Nind 2021-04-10 22:09:55 UTC
Changed status to in discussion... feel free to change back when/if ready for sign off.
Comment 10 Martin Renvoize 2021-04-12 15:02:24 UTC
In fact.. I'm not sure this makes sense at all.. a Plugin requires a metadata structure containing a version.. so even if we add `// {};` as a default, we would just be pushing the error elsewhere.
Comment 11 Fridolin Somers 2021-09-11 08:03:57 UTC

*** This bug has been marked as a duplicate of bug 28228 ***
Comment 12 Fridolin Somers 2021-09-11 08:04:45 UTC
Alvaro Cornejo thanks a lot for your work, its continues on Bug 28228