https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41892 --- Comment #1 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 193509 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193509&action=edit Bug 41892: Remove C4::Context from C4::Auth's BEGIN block Patch from commit 0722d27 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41892 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com, | |martin.renvoize@openfifth.c | |o.uk, tomascohen@gmail.com --- Comment #2 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Good points: * Logic is moved to its own package. * Initialization is more clear * The package is a wrapper for nicely setting $ENV{REMOTE_ADDR} Bad points: * It is not OO * It uses `require`? I tend to agree here, but I also agree on my use in the Availability bug. We should discuss when it is worth and when it isn't. * `->set()` is weird. I'd expect the constructor `new()` to set thing, and return an object we keep within a top-level object (a.k.a. `Koha::Context` class instance) * The package is a wrapper for nicely setting $ENV{REMOTE_ADDR}, but we still have code querying $ENV{REMOTE_ADDR}, when it should really use the global context object to access it: e.g. $context->config*->remote_addr with some better encapsulation: ``` C4/InstallAuth.pm: # $ENV{'REMOTE_ADDR'}, $time; C4/Output.pm: my $IPAddress = $ENV{'REMOTE_ADDR'}; Koha/App/Controller/CGI.pm: REMOTE_ADDR => $c->tx->remote_address, Koha/Template/Plugin/Branches.pm: my $ip = $ENV{'REMOTE_ADDR'}; opac/ilsdi.pl: if ( $netmask && $netmask->match( $ENV{REMOTE_ADDR} ) ) opac/ilsdi.pl: $out->{'message'} = "Unauthorized IP address: $ENV{REMOTE_ADDR}."; opac/ilsdi.pl: $out->{'message'} = "Unauthorized IP address: $ENV{REMOTE_ADDR}."; opac/opac-restrictedpage.pl:my $userIP = $ENV{'REMOTE_ADDR'}; opac/opac-search.pl: my $IPAddress = $ENV{'REMOTE_ADDR'}; ``` it is not the end of the world, but I think the idea of having a stash with the current request context and accessing it uniformly through the codebase would be great and this is the chance to do it. Some other headers might be worth keeping on the RemoteAddress object, not sure. * It feels the `use Koha::Config` will be our next `C4::Context` in terms of circular dependencies Following the way I've been coding and the patterns I try to implement (documented on the koha-handbook project), I suggest: * We create an OO `Koha::Context` class * `Koha::Context` should be the only one including the other packages (e.g. `Koha::Config`, `Koha::Context::RemoteAddress`, etc). Should some code need access to the config, then $context->config should return the already instantiated `koha::Config` object. * The `Koha::Context` constructor should initialize the related classes, the way `C4::Context` does now, just better. * The `Koha::Context::*` packages should be OO too. * Ideally we should implement `Koha::Context` as a singleton, at least in the scope of a request. Not sure how that would be implemented, but I'd expect that we initialize the context in `C4::Auth` (or whatever replaces it), it internally initializes the other classes, and then every caller can access it. * If everything was API-driven (which it should) we would solve the singleton using the `stash`. We need to think that way. So maybe the legacy code would do something manual and API endpoints would have it in the stash. Need to think a bit more about it. * Reorganizing the code like this is part of the task, but I believe we should go OO. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41892 --- Comment #4 from Jonathan Druart <jonathan.druart@gmail.com> --- Thanks for the feedback. IMO the first step is to remove the existing circular dependencies. Then we can redesign on top. The idea was to have less disruptive changes as possible, to make it backportable if needed. This (bug 41890) is mostly bug fixes actually instead of enh. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41892 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #3)

Yeah, we need to check how this is to be used as `Koha::Middleware::RealIP` does the same thing.

It's supposed to be used for non-Plack CGI scripts, because they're not covered by the Koha::Middleware::RealIP middleware. That said, I don't know why we have "require CGI" and "CGI->http" as we could just use $ENV{HTTP_X_FORWARDED_FOR}. I think it executes for Plack scripts too but wouldn't do anything as the REMOTE_ADDR would've already been rewritten. We could check for PSGI env variables though. All Jonathan is doing here is moving C4::Context->set_remote_address to Koha::Context::RemoteAddress->set, and updating some preference lookups. I think it's a reasonable change as is. -- Overall, I think $ENV{REMOTE_ADDR} is the right place to check. (Note that instead of Koha::Middleware::RealIP, a person could just use mod_remoteip with Apache.) I reckon creating a Koha::Context singleton would likely bloat over time and just get us back to where we are now. The hard part when we have 1 object doing many things is that it ends up having many dependencies which then leads to circular dependencies. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41892 --- Comment #8 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to David Cook from comment #6)

I reckon creating a Koha::Context singleton would likely bloat over time and just get us back to where we are now.

The hard part when we have 1 object doing many things is that it ends up having many dependencies which then leads to circular dependencies.

The whole point of introducing object orientation here is that consumers will get an object and will no longer need to import anything. They'll have a factory class that we control circ deps at, and provides the instantiated classes. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41892 --- Comment #10 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #8)

(In reply to David Cook from comment #6)

...

I reckon creating a Koha::Context singleton would likely bloat over time and just get us back to where we are now.

The hard part when we have 1 object doing many things is that it ends up having many dependencies which then leads to circular dependencies.

The whole point of introducing object orientation here is that consumers will get an object and will no longer need to import anything. They'll have a factory class that we control circ deps at, and provides the instantiated classes.

Except that by loading the factory class they'll have to load the dependencies of the factory class too, so they'll still be running the import() method on those dependencies. Depending on what you have in mind, you could move those "require <class>" into methods instead of using "use <class>" at the top, but that just disguises the dependencies and makes them harder to find. You could add a few more layers of abstraction on top of what Jonathan has done, but I don't think it would add any value. But happy to see a POC. Maybe I'm misunderstanding what you're saying. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41892 --- Comment #11 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 199285 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=199285&action=edit Bug 41892: Remove C4::Context from C4::Auth's BEGIN block Patch from commit 2fa591b -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41892 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #199285|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41892 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42663 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 [Bug 42663] Koha::Context must be object-oriented designed -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41892 --- Comment #14 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #9)

Clicked too fast. I'll try to provide my own POC.

I am going to suggest something on bug 42663. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41892 --- Comment #15 from Jonathan Druart <jonathan.druart@gmail.com> --- Patches are available on the remote branch of the parent bug report. -- You are receiving this mail because: You are watching all bug changes.