[Bug 42663] New: Koha::Context must be object-oriented designed
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 Bug ID: 42663 Summary: Koha::Context must be object-oriented designed Initiative type: --- Sponsorship --- status: Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart@gmail.com Reporter: jonathan.druart@gmail.com QA Contact: testopia@bugs.koha-community.org Discussion from bug 41892 suggested that the new module Koha::Context should have an Object-oriented interface. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au, | |julian.maurice@biblibre.com | |, tomascohen@gmail.com Status|NEW |ASSIGNED Blocks| |42662 Depends on| |41892, 41893, 41894, 41896, | |41897, 41974, 41975 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41892 [Bug 41892] Remove C4::Context from C4::Auth's BEGIN block https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41893 [Bug 41893] Remove C4::Context from Koha::Plugins's BEGIN block https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41894 [Bug 41894] Bareword "C4::Context::only_my_library" not allowed https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41896 [Bug 41896] Remove C4::Context from Koha::Cache https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41897 [Bug 41897] Remove C4::Context from Koha::Objects::Limit::Library https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41974 [Bug 41974] Remove C4::Context from Koha::Object https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41975 [Bug 41975] Remove C4::Context from C4::Log https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42662 [Bug 42662] [OMNIBUS] Remove C4::Context circular dependencies -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #1 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 199372 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=199372&action=edit Bug 42663: Rewrite Koha::Context in OOP Patch from commit 5e73faf -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #2 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 199373 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=199373&action=edit Bug 42663: Adjust previous changes Patch from commit c9cde74 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |In Discussion --- Comment #3 from Jonathan Druart <jonathan.druart@gmail.com> --- I think there are several good things in this approach: 1. Code is elegant and easy to read! my $context = Koha::Context->current; my $preferences = $context->preferences; my $value = $preferences->get_value("MyPref"); my $config = $context->config; my $value = $config->get("entry"); my $userenv = $context->userenv; my $number = $userenv->get("number"); 2. Remove circular deps and lazy-load "components" (Koha::Context) 3. Need only one middleware (Koha::Middleware::Context replaces Koha::Context::UserEnv, but could also replace RealIP and SetEnv) 4. We set the env from the Plack's middleware and retrieve it from Koha::Context. This might feel weird at first but I think it's a good solution for our hybride PSGI situation (Koha::Context->current). Koha/Context.pm 70 # Try to get from Plack environment first 71 my $env = $class->is_psgi_env; 72 if ($env) { 73 74 # Some modules pre-loaded in plack.psgi access the context before we set it in the middleware 75 # FIXME Is it an (existing) security issue? 76 my $existing_context = $Koha::Middleware::Context::psgi_env->{'koha.context'}; 77 return $existing_context || $class->new; 78 } Koha/Middleware/Context.pm 50 # Store context in environment 51 $env->{'koha.context'} = $context; 52 53 # Initialize remote address is done in Koha::Middleware::RealIP 54 # (for non-Plack CGI scripts this is done in BEGIN block) 55 56 $context->set_current($context); 57 58 # Initialize userenv through context 59 my $userenv = $context->userenv; 60 $userenv->attach( {} ); 61 62 local $psgi_env = $env; 63 return $self->app->($env); We can move this to memory cache if not considered nice/robust enough. 5. Bad code is highlighted here however, some preloaded modules (from plack.psgi) uses the context at compile time (mostly from BEGIN blocks), which is obviously not set yet. IMO this should be remove ASAP anyway, and as follow-up of this bug. That's why `use C4::XSLT` is (temporarily) removed from plack.psgi. The whole tree is available on my gitlab, branch bug_42663 https://gitlab.com/joubu/Koha/-/commits/bug_42663 Next steps: * Apply to C4::Context * Koha::Context->Preferences should have use_syspref_cache defaults to true (currently false) * Write missing POD and tests * Remove `use C4::Context` where it's used only for config, syspref, userenv. [...] * Kill C4::Context This is either awesome and we need to move forward, or I am missing something silly. Please provide feedback, I would be more than happy to continue working on this! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #4 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #0)
Discussion from bug 41892 suggested that the new module Koha::Context should have an Object-oriented interface.
What you've written is mostly what I had in mind. 1) Does this: ```perl $Koha::Middleware::Context::psgi_env->{'koha.context'}; ``` live for a request lifecycle? Is that how it works? 2) I expected to see some `state` to help reusing the instantiated context object on CGI [1] Nice job. This is the way! [1] https://perldoc.perl.org/functions/state -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #5 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #4)
(In reply to Jonathan Druart from comment #0)
Discussion from bug 41892 suggested that the new module Koha::Context should have an Object-oriented interface.
What you've written is mostly what I had in mind.
1) Does this:
```perl $Koha::Middleware::Context::psgi_env->{'koha.context'}; ```
live for a request lifecycle? Is that how it works?
Yes, it's reset in the middleware: 62 local $psgi_env = $env; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- So I'm a bit confused... you've created a new circular dependency in Koha::Middleware::Context and Koha::Context where they depend on each other. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=20813 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #7 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #3)
I think there are several good things in this approach:
1. Code is elegant and easy to read! my $context = Koha::Context->current; my $preferences = $context->preferences; my $value = $preferences->get_value("MyPref");
my $config = $context->config; my $value = $config->get("entry");
my $userenv = $context->userenv; my $number = $userenv->get("number");
In theory, I think that this sounds good, as it's similar to what you see in MVC frameworks. https://metacpan.org/pod/Catalyst#%24c-%3Econfig https://metacpan.org/pod/Catalyst::Component https://metacpan.org/pod/Catalyst::Plugin::Session In fact, looking at Koha/Context.pm, it looks similar to how Catalyst works. But the problem is Koha isn't an MVC web app like Catalyst, and Catlayst also only applies to web apps. It doesn't cover things like CLI tools or cronjobs. (That's why you end up with modules like https://metacpan.org/pod/Config::JFDI which provide Catlayst config outside of a web context.) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #8 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #7)
But the problem is Koha isn't an MVC web app like Catalyst, and Catlayst also only applies to web apps. It doesn't cover things like CLI tools or cronjobs. (That's why you end up with modules like https://metacpan.org/pod/Config::JFDI which provide Catlayst config outside of a web context.)
I mean I say it's a problem... I suppose we'd have those other components that can be used in the CLI tools and cronjobs so that's good... that should take care of those situations. Maybe the only problem is that Koha::Context->current(). 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #9 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #4)
2) I expected to see some `state` to help reusing the instantiated context object on CGI [1]
Nice job. This is the way!
That shouldn't be necessary. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #10 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 199532 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=199532&action=edit Bug 42663: Remove use Koha::Middleware::Context Remove the circ deps at compile time, $Koha::Middleware::Context::psgi_env will be available at runtime because the middleware loads Koha::Context -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #11 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to David Cook from comment #6)
So I'm a bit confused... you've created a new circular dependency in Koha::Middleware::Context and Koha::Context where they depend on each other.
Sorted in the last patch. Not sure you will like it :D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #12 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to David Cook from comment #8)
(In reply to David Cook from comment #7)
But the problem is Koha isn't an MVC web app like Catalyst, and Catlayst also only applies to web apps. It doesn't cover things like CLI tools or cronjobs. (That's why you end up with modules like https://metacpan.org/pod/Config::JFDI which provide Catlayst config outside of a web context.)
I mean I say it's a problem... I suppose we'd have those other components that can be used in the CLI tools and cronjobs so that's good... that should take care of those situations.
Maybe the only problem is that Koha::Context->current().
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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #13 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #12)
(In reply to David Cook from comment #8)
(In reply to David Cook from comment #7)
But the problem is Koha isn't an MVC web app like Catalyst, and Catlayst also only applies to web apps. It doesn't cover things like CLI tools or cronjobs. (That's why you end up with modules like https://metacpan.org/pod/Config::JFDI which provide Catlayst config outside of a web context.)
I mean I say it's a problem... I suppose we'd have those other components that can be used in the CLI tools and cronjobs so that's good... that should take care of those situations.
Maybe the only problem is that Koha::Context->current().
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.
That's a good point about ENV. That's what caused that Business::ISBN issue recently... But now that I think about it - why use an environmental variable at all? It can just exist within a variable in Koha::Context. You can just my $instance at the file level in Koha::Context and it's all good. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #14 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to David Cook from comment #13)
But now that I think about it - why use an environmental variable at all? It can just exist within a variable in Koha::Context. You can just my $instance at the file level in Koha::Context and it's all good.
Nope, Julian found a bug on bug 41972. We don't want it to persist between 2 requests. We can store it in the L1 cache, but this trick felt more Mojo-oriented and more inline with our "ultimate" goal. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #15 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #14)
(In reply to David Cook from comment #13)
But now that I think about it - why use an environmental variable at all? It can just exist within a variable in Koha::Context. You can just my $instance at the file level in Koha::Context and it's all good.
Nope, Julian found a bug on bug 41972. We don't want it to persist between 2 requests. We can store it in the L1 cache, but this trick felt more Mojo-oriented and more inline with our "ultimate" goal.
I see what you mean. I was thinking more about globals like Koha::Context->config() which should be fine to be persisted, but a request-scoped $context->userenv would not be. That could be a problem for a app level singleton (unless you reset the request-scoped data but yeah better to use the environmental variables). You've inspired me a bit with your Mojo reference... When I think about frameworks like Mojolicious or Catalyst, I think about their $c controller context object. And in those cases you'd have $c->session instead of $context->userenv. So if I look at: https://metacpan.org/dist/Plack-Middleware-Session/source/lib/Plack/Middlewa.... I see that they do pass a Plack::Session object through, just like we're passing a Koha::Context object in $env->{'koha.context'}. Looking at that Plack Middleware, we might want to consider using a key like "psgix.koha.context". (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? 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #16 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #15)
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?
Or is it because of Plack::App::CGIBin? That should also be fine. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #17 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to David Cook from comment #15)
Looking at that Plack Middleware, we might want to consider using a key like "psgix.koha.context".
Agreed.
(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?
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. 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. -- You are receiving this mail because: You are watching all bug changes.
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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #19 from David Cook <dcook@prosentient.com.au> --- Actually, I think part of the problem is I'm trying to review it in Bugzilla. I'll look at it in git quick... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #20 from David Cook <dcook@prosentient.com.au> --- Error: Apply failed: Git command (git am -3 /tmp/65T3f7nUOu/0001-199372.patch) failed: error: sha1 information is lacking or useless (C4/Auth.pm). error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #21 from David Cook <dcook@prosentient.com.au> --- I keep notice this same missing commit for C4/Auth.pm. I'm guessing RM or RM Assistant rebased some older commits and erased a state that a bunch of people developed against... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #22 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to David Cook from comment #20)
Error: Apply failed: Git command (git am -3 /tmp/65T3f7nUOu/0001-199372.patch) failed: error: sha1 information is lacking or useless (C4/Auth.pm). error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch
The whole tree still applies for me, I've update the remote branch https://gitlab.com/joubu/Koha/-/commits/bug_42663 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #23 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #22)
(In reply to David Cook from comment #20)
Error: Apply failed: Git command (git am -3 /tmp/65T3f7nUOu/0001-199372.patch) failed: error: sha1 information is lacking or useless (C4/Auth.pm). error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch
The whole tree still applies for me, I've update the remote branch https://gitlab.com/joubu/Koha/-/commits/bug_42663
I just fetched https://gitlab.com/joubu/Koha and e2ef34cb580 does represent a C4/Auth.pm state there, and it looks like the problem was I must not have applied the dependencies. I don't recall git-bz prompting to apply them. Hmm... I don't really understand the dependency list here to be honest. For instance, bug 41896 is listed as a dependency, but it doesn't appear to be relevant here? Since bug 41896 is the only one that is really moving along, wouldn't it make more sense to remove the dependencies and then make them depend on bug 42663? I know it's a PITA from a coding perspective, but from a testing perspective it would make way more sense. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #24 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to David Cook from comment #23)
(In reply to Jonathan Druart from comment #22)
(In reply to David Cook from comment #20)
Error: Apply failed: Git command (git am -3 /tmp/65T3f7nUOu/0001-199372.patch) failed: error: sha1 information is lacking or useless (C4/Auth.pm). error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch
The whole tree still applies for me, I've update the remote branch https://gitlab.com/joubu/Koha/-/commits/bug_42663
I just fetched https://gitlab.com/joubu/Koha and e2ef34cb580 does represent a C4/Auth.pm state there, and it looks like the problem was I must not have applied the dependencies. I don't recall git-bz prompting to apply them. Hmm...
perl-git-bz apply patches from bug reports "in discussion" only since a couple of weeks. You need to pull.
I don't really understand the dependency list here to be honest. For instance, bug 41896 is listed as a dependency, but it doesn't appear to be relevant here?
Since bug 41896 is the only one that is really moving along, wouldn't it make more sense to remove the dependencies and then make them depend on bug 42663? I know it's a PITA from a coding perspective, but from a testing perspective it would make way more sense.
Indeed we don't directly need bug 41896 here. However I don't understand what you are suggesting. Bug 42663 is supposed to be the next step (move to OOP) once the first one (remove circ deps) is pushed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #25 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #24)
perl-git-bz apply patches from bug reports "in discussion" only since a couple of weeks. You need to pull.
Sounds good. I'll do that.
I don't really understand the dependency list here to be honest. For instance, bug 41896 is listed as a dependency, but it doesn't appear to be relevant here?
Since bug 41896 is the only one that is really moving along, wouldn't it make more sense to remove the dependencies and then make them depend on bug 42663? I know it's a PITA from a coding perspective, but from a testing perspective it would make way more sense.
Indeed we don't directly need bug 41896 here. However I don't understand what you are suggesting. Bug 42663 is supposed to be the next step (move to OOP) once the first one (remove circ deps) is pushed.
The next step after bugs 41892 41893 41894 41897 41974 41975 ? But wouldn't it make more sense for those bug reports to depend on this one? They all would need to be updated to use the OOP version, but it just seems more logical? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #26 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #25)
The next step after bugs 41892 41893 41894 41897 41974 41975 ? But wouldn't it make more sense for those bug reports to depend on this one? They all would need to be updated to use the OOP version, but it just seems more logical?
In a way, it's a bit of a chicken and egg problem, but if we have Koha::Context as OOP first, then there's less double-handling and all the circular deps can be removed on separate timelines - rather than needing to be done before this one can move forward. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #27 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #26)
(In reply to David Cook from comment #25)
The next step after bugs 41892 41893 41894 41897 41974 41975 ? But wouldn't it make more sense for those bug reports to depend on this one? They all would need to be updated to use the OOP version, but it just seems more logical?
In a way, it's a bit of a chicken and egg problem, but if we have Koha::Context as OOP first, then there's less double-handling and all the circular deps can be removed on separate timelines - rather than needing to be done before this one can move forward.
Because realistically all of those other bugs would need to be re-tested after the change anyway... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42663 --- Comment #28 from Jonathan Druart <jonathan.druart@gmail.com> --- I first implemented the circ deps removal, then asked to rewrite using OOP. This is done in this patch, no other changes will be needed (all packages undef Koha::Context are OOP now, see the first patch "Rewrite oha::Context in OOP"). But yes, ideally the whole tree should be tested, reviewed and pushed at the same time. I'd say that the circ deps removal are bug fixes, and the OOP rewrite is a new enhancement. The others could be backported, while this one should not. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org