Bug 33649

Summary: Fix use of cronlogaction
Product: Koha Reporter: Magnus Enger <magnus>
Component: NoticesAssignee: Magnus Enger <magnus>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: jonathan.druart, lucas
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.06
Circulation function:
Bug Depends on: 31203    
Bug Blocks:    
Attachments: Bug 33649: Fix use of cronlogaction
Bug 33649: Fix use of cronlogaction
Bug 33649: Fix use of cronlogaction

Description Magnus Enger 2023-05-02 13:14:28 UTC
process_message_queue.pl has this code: 

  try {
      $script_handler->lock_exec;
  }
  catch {
      my $message = "Skipping execution of $0 ($_)";
      print STDERR "$message\n"
          if $verbose;
      cronlogaction( $message );
      exit;
  };

If lock_exec fails for some reason, we get this error: 

$ sudo koha-foreach --chdir --enabled --email /usr/share/koha/bin/cronjobs/process_message_queue.pl
Can't use string ("Skipping execution of /usr/share"...) as a HASH ref while "strict refs" in use at /usr/share/koha/lib/C4/Log.pm line 135.
mykoha: 13 status returned by "/usr/share/koha/bin/cronjobs/process_message_queue.pl"

This is because cronlogaction should be called like this: 

cronlogaction({ info => $message })
Comment 1 Jonathan Druart 2023-05-03 06:43:41 UTC
There is an other one in misc/cronjobs/fines.pl.
Comment 2 Magnus Enger 2023-05-03 06:48:52 UTC
(In reply to Jonathan Druart from comment #1)
> There is an other one in misc/cronjobs/fines.pl.

Yeah, just spotted it. :-)
Comment 3 Magnus Enger 2023-05-03 07:04:22 UTC
Created attachment 150563 [details] [review]
Bug 33649: Fix use of cronlogaction

C4::Log::cronlogaction() takes a hashref as argument, with "info"
and possibly "action" as keys. But there are a couple of places
where it is called with just a string as argument, and that does
not work. Both places need lock_exec to fail to trigger the error.
I have seen this on a production server, but not been able to
reproduce in ktd.

To test:
- Run this on the Koha repo: grep -r "cronlogaction(" *
- Verify that fines.pl and process_message_queue.pl are the only
  scripts that call cronlogaction without a hashref as argument,
  but do it like this: cronlogaction( $message );
- Apply this patch
- Run the grep again and verify that all calls to cronlogaction
  now take a hashref as argument
Comment 4 Nick Clemens (kidclamp) 2023-05-03 11:23:01 UTC
Created attachment 150573 [details] [review]
Bug 33649: Fix use of cronlogaction

C4::Log::cronlogaction() takes a hashref as argument, with "info"
and possibly "action" as keys. But there are a couple of places
where it is called with just a string as argument, and that does
not work. Both places need lock_exec to fail to trigger the error.
I have seen this on a production server, but not been able to
reproduce in ktd.

To test:
- Run this on the Koha repo: grep -r "cronlogaction(" *
- Verify that fines.pl and process_message_queue.pl are the only
  scripts that call cronlogaction without a hashref as argument,
  but do it like this: cronlogaction( $message );
- Apply this patch
- Run the grep again and verify that all calls to cronlogaction
  now take a hashref as argument

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Jonathan Druart 2023-05-03 11:48:09 UTC
Created attachment 150577 [details] [review]
Bug 33649: Fix use of cronlogaction

C4::Log::cronlogaction() takes a hashref as argument, with "info"
and possibly "action" as keys. But there are a couple of places
where it is called with just a string as argument, and that does
not work. Both places need lock_exec to fail to trigger the error.
I have seen this on a production server, but not been able to
reproduce in ktd.

To test:
- Run this on the Koha repo: grep -r "cronlogaction(" *
- Verify that fines.pl and process_message_queue.pl are the only
  scripts that call cronlogaction without a hashref as argument,
  but do it like this: cronlogaction( $message );
- Apply this patch
- Run the grep again and verify that all calls to cronlogaction
  now take a hashref as argument

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 6 Jonathan Druart 2023-05-03 11:48:36 UTC
Can be tested easily adding a sleep right after the lock.
Comment 7 Tomás Cohen Arazi 2023-05-05 13:15:47 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 8 Martin Renvoize 2023-05-12 14:05:51 UTC
Many hands makes light work, thankyou everyone!

Pushed to 22.11.x for the next release
Comment 9 Lucas Gass 2023-06-13 15:48:28 UTC
Missing dependencies for 22.05.x, no backport.