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.