Bug 31468 - Koha::Logger should prefix interface with 'plack'
Summary: Koha::Logger should prefix interface with 'plack'
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 29744
Blocks: 22678
  Show dependency treegraph
 
Reported: 2022-08-26 09:25 UTC by Marcel de Rooy
Modified: 2023-06-08 22:26 UTC (History)
3 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:
22.11.00


Attachments
Bug 31468: Changes to C4::Context::is_psgi_or_plack (2.25 KB, patch)
2022-08-26 09:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31468: Change interface in Koha::Logger (1.11 KB, patch)
2022-08-26 09:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31468: Change interface in Koha::Logger (1.13 KB, patch)
2022-09-16 08:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31468: Change interface in Koha::Logger (1.15 KB, patch)
2022-09-16 09:39 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2022-08-26 09:25:28 UTC
Koha::Logger->get( interface => 'opac' ) should write to plack-opac under plack.

We still need a few tiny tweaks in Context and Logger.
Comment 1 Marcel de Rooy 2022-08-26 09:34:03 UTC
Created attachment 139821 [details] [review]
Bug 31468: Changes to C4::Context::is_psgi_or_plack

When running API, we need to look for $ENV{PLACK_ENV}, but the
underscore is not included in the Context sub.

Test plan:
Run t/Context.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2022-08-26 09:43:31 UTC
Created attachment 139822 [details] [review]
Bug 31468: Change interface in Koha::Logger

Test plan:
Add a line in mainpage.pl:
  require Koha::Logger; Koha::Logger->get->warn("intranet L51");
Restart. Hit intranet mainpage. Verify that line is in plack=intranet.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2022-08-26 09:44:33 UTC
This is trivial and architectural. Going to SO rightaway. Need this for 22678 too.
Comment 4 Marcel de Rooy 2022-08-29 08:42:29 UTC
Could you have a look, Martin ?
Comment 5 Martin Renvoize 2022-09-12 14:00:29 UTC
I have recollections of trying to improve the Context method before and getting caught by all sorts of weird side effects.. do you happen to remember the details David?
Comment 6 Martin Renvoize 2022-09-12 14:06:17 UTC
Lots of discussion around our various implimentations of 'Is this a plack/psgi session' in bug 29420
Comment 7 Martin Renvoize 2022-09-12 14:20:09 UTC
My gut says we should finally test and push bug 29744 before this.. harmonizing the plack detection and then use it here too
Comment 8 David Cook 2022-09-12 23:18:24 UTC
(In reply to Martin Renvoize from comment #5)
> I have recollections of trying to improve the Context method before and
> getting caught by all sorts of weird side effects.. do you happen to
> remember the details David?

Not really but I think bug 29744 probably took care of it?
Comment 9 Marcel de Rooy 2022-09-13 11:12:36 UTC
(In reply to Martin Renvoize from comment #7)
> My gut says we should finally test and push bug 29744 before this..
> harmonizing the plack detection and then use it here too

Sure, I will have a look at the other one. Although it is pretty straightforward to harmonize if we would go the other way around.

Still something to discuss here?
Comment 10 Marcel de Rooy 2022-09-16 07:56:37 UTC
Comment on attachment 139821 [details] [review]
Bug 31468: Changes to C4::Context::is_psgi_or_plack

This has been moved and adjusted on bug 29744
Comment 11 Marcel de Rooy 2022-09-16 08:04:10 UTC
Created attachment 140693 [details] [review]
Bug 31468: Change interface in Koha::Logger

Test plan:
Add a line in mainpage.pl:
  require Koha::Logger; Koha::Logger->get->warn("intranet L51");
Restart. Hit intranet mainpage. Verify that line is in plack=intranet.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Retested with L49 now ;)
Comment 12 Martin Renvoize 2022-09-16 09:39:52 UTC
Created attachment 140697 [details] [review]
Bug 31468: Change interface in Koha::Logger

Test plan:
Add a line in mainpage.pl:
  require Koha::Logger; Koha::Logger->get->warn("intranet L51");
Restart. Hit intranet mainpage. Verify that line is in plack=intranet.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Retested with L49 now ;)
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2022-09-16 09:40:21 UTC
All working and thanks for cleaning up 29744 more for us.

Passing QA
Comment 14 Tomás Cohen Arazi 2022-09-23 12:32:35 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 15 Lucas Gass 2022-11-01 22:09:58 UTC
Missing dependencies for 22.05.x, no backport