We are supposed to be able to override syspref's values using the OVERRIDE_SYSPREF trick, but it's not working for the REST API. To recreate, open /etc/koha/apache-shared-intranet-plack.conf and add RequestHeader add X-Koha-SetEnv "OVERRIDE_SYSPREF_Foo Bar" Add some warn, mainpage.pl and Koha/REST/V1/Patrons.pm (sub list) for instance warn C4::Context->preference('Foo'); Hit the mainpage, look at the log, you see "Bar". Search for patrons: "Use of uninitialized value in warn" => The syspref hasn't been overridden.
The only thing I see is that we are using Plack::App::CGIBin for the intranet and Mojo::Server::PSGI for the API. Could that be a lead?
That's very interesting. I thought maybe the problem was with RequestHeader/RewriteRule not mixing, but the issue exists for both http://localhost:8081/api/v1/patrons and http://localhost:8081/api/v1/app.pl/api/v1/patrons When I dump out the variables from Koha/Middleware/SetEnv.pm I see the data coming through for both the mainpage.pl and the API: 'HTTP_X_KOHA_SETENV' => 'OVERRIDE_SYSPREF_Foo Bar', When I dump %ENV for mainpage.pl, I see lots of HTTP_X headers including: 'HTTP_X_KOHA_SETENV' => 'OVERRIDE_SYSPREF_Foo Bar', When I dump %ENV for API, I don't see any HTTP_X headers... which look like they're just the environmental variables Starman started with... Yep... you need to access the Plack/PSGI-level environmental variables in Mojo::Server::PSGI using $c->req->env. Plack::App::CGIBin uses Plack::App::WrapCGI which resets the %ENV variables at this line: https://metacpan.org/dist/Plack/source/lib/Plack/App/WrapCGI.pm#L48
So... that's interesting. Since %ENV is a global variable, anything we do to it will affect the whole life of the Starman worker process. It looks like we're setting $ENV{REMOTE_ADDR} in Koha/Middleware/RealIP.pm so we're already meddling with that global variable. Since REMOTE_ADDR gets set by every request, I suppose there's no real problem... I suppose we could update Koha/Middleware/SetEnv.pm or Koha/REST/V1.pm, to update %ENV... and folk would need to know they'd need to restart Starman if they change their RequestHeader directives in Apache... -- (As an aside, are we actually flushing L1 caches with the REST API? I don't see anything for it at a glance...) --
I'm looking at custom code right now but there's a little over 100 instances of $ENV in the code I'm looking at. For CGI-style .pl scripts, that should be fine. For Perl modules, it's more of a problem since they can be used by Mojolicious code and create bugs as a result. Like we're seeing now. -- I suppose one option would be to move $ENV calls to C4::Context->env(...) which could check %ENV and/or a $req_env variable which could be set per request (using various mechanisms). That seems like a bit of a hack though. The shortest path is just to pollute %ENV like we're doing with REMOTE_ADDR. -- Another idea for this very particular use case would be to create a cache in C4::Context and then use before_dispatch hook in Koha/REST/V1.pm to set that C4::Context cache from the $env variable. (That works while we're using synchronous requests, but if we used Mojo asynchronously then that would probably be vulnerable to a race condition.) --
Over time, we'll find global variables and singletons like C4::Context are going to cause more and more headaches. Yet, we've deeply embedded C4::Context->preference throughout all our code... For example, Koha::Patron->store() is riddled with syspref calls. Over time, it would be an idea to switch from C4::Context->preference() to $c->syspref() and to pass preferences through function/method calls. (That would help support multitenancy and non-blocking programming in the future too.)
And I realized that just using %ENV could be a problem since the intranet, opac, and API all are served by the same workers. Unless care is taken to clean the %ENV each time then even the shortest path is fraught...
You know what... I wonder if this is still a problem. I discovered a problem with Koha/Middleware/SetEnv.pm a while back and fixed it in bug 33967. See my comment at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23068#c54 That said, it might not be related... something to investigate...
(In reply to David Cook from comment #7) > You know what... I wonder if this is still a problem. Yes it is, I still get "Use of uninitialized value in warn at /kohadevbox/koha/Koha/REST/V1/Patrons.pm" when redoing comment 0.
(In reply to Jonathan Druart from comment #0) > We are supposed to be able to override syspref's values using the > OVERRIDE_SYSPREF trick, but it's not working for the REST API. > > To recreate, open /etc/koha/apache-shared-intranet-plack.conf and add > RequestHeader add X-Koha-SetEnv "OVERRIDE_SYSPREF_Foo Bar" > > Add some warn, mainpage.pl and Koha/REST/V1/Patrons.pm (sub list) for > instance > warn C4::Context->preference('Foo'); > > Hit the mainpage, look at the log, you see "Bar". Search for patrons: "Use > of uninitialized value in warn" > => The syspref hasn't been overridden. tail -f /var/log/koha/kohadev/plack-intranet-error.log [2023/09/21 02:10:38] [WARN] Bar at /kohadevbox/koha/mainpage.pl line 42. tail -f /var/log/koha/kohadev/plack-api-error.log [2023/09/21 02:11:27] [WARN] Use of uninitialized value in warn at /kohadevbox/koha/Koha/REST/V1/Patrons.pm line 46. [2023/09/21 02:11:27] [WARN] Warning: something's wrong at /kohadevbox/koha/Koha/REST/V1/Patrons.pm line 46.
So options... 1. Like Koha/Middleware/RealIP.pm we could update Koha/Middleware/SetEnv.pm to work with the %ENV and not just the $env. That's probably the shortest path. 2. Add code to Koha/REST/V1.pm to update %ENV like https://metacpan.org/dist/Plack/source/lib/Plack/App/WrapCGI.pm#L48 or another data structure that could be used by C4::Context->preference This is probably more logical and not too much longer -- But really this shows how problematic our use of C4::Context->preference really is. (It's also why in other bugs I've said that I don't think it's a great idea for us to embed C4::Context as deeply in Koha modules as we do.)