Bug 33649 - Fix use of cronlogaction
Summary: Fix use of cronlogaction
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Magnus Enger
QA Contact: Testopia
URL:
Keywords:
Depends on: 31203
Blocks:
  Show dependency treegraph
 
Reported: 2023-05-02 13:14 UTC by Magnus Enger
Modified: 2024-07-04 20:37 UTC (History)
2 users (show)

See Also:
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:


Attachments
Bug 33649: Fix use of cronlogaction (1.84 KB, patch)
2023-05-03 07:04 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 33649: Fix use of cronlogaction (1.90 KB, patch)
2023-05-03 11:23 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33649: Fix use of cronlogaction (1.97 KB, patch)
2023-05-03 11:48 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.