https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #18 from David Cook <dcook@prosentient.com.au> ---
(In reply to Jonathan Druart from comment #12)
Koha::Context shouldn't depend on Koha::Middleware::Context. In fact, I don't think that we really need Koha::Middleware::Context. All Koha::Context->current() needs to do is return a singleton. So check for the $ENV{"koha.context"}. If it's set, return its contents. If it's not set, set it. That's it.
We cannot store structures in ENV, only strings. That's why I need to reuse the psgi_env var from the middleware.
I don't understand this. We are already storing structures in a PSGI environment. Why would we need to store a structure in an "ENV" environmental variable? Is it because of the CGI scripts?
What do we store already there?
In your patch: 47 # Create request-scoped context 48 my $context = Koha::Context->new; 49 50 # Store context in environment 51 $env->{'koha.context'} = $context;
Note that https://metacpan.org/pod/Plack::Component#OBJECT-LIFECYCLE warns against saving per-request data in the object (and that would be especially true for an "our" variable as well).
--
Overall, I think that we're mixing up a number of things here still, and it's probably because of the CGI scripts. If/when the app is 100% a Plack app or 100% a Mojo app, then we'll be able to easily handle things at the beginning and ending of the request cycle.
Obviously we need to mix and workaround, as usual. But we don't need to wait to switch to a 100% mojo app to make this kind of code improvements.
But where would we use Koha::Middleware::Context::psgi_env? I don't understand that part. I'm not seeing where it's used in the code?
IMO the solution is quite good and elegant. I would be happy to receive follow-up patches for any further improvements you can find. Certainly a good area to iterate together.
I think it's pretty good overall. There's just a couple of fine points I'm not sure about. -- I'm also not sure about the dependency graph? Is the idea that we complete Bug 42663 and then redo the patches for the removal of C4::Context? Or are those separate? -- You are receiving this mail because: You are watching all bug changes.