Created attachment 53656 [details] [review] Bug 16965: Add Koha::Objects->search_related In order to search on relations, we need this new method. Test plan: Confirm that the changes in Objects.t make sense and that the tests pass.
Shouldn't you wrap them like search does ?
(In reply to Marcel de Rooy from comment #2) > Shouldn't you wrap them like search does ? I don't think so: "In list context, ->all() is called implicitly on the resultset, thus returning a list of result objects instead." https://metacpan.org/pod/DBIx::Class::ResultSet#search_related
(In reply to Jonathan Druart from comment #3) > (In reply to Marcel de Rooy from comment #2) > > Shouldn't you wrap them like search does ? > > I don't think so: > "In list context, ->all() is called implicitly on the resultset, thus > returning a list of result objects instead." > https://metacpan.org/pod/DBIx::Class::ResultSet#search_related I mean the other search: sub search { my ( $self, $params, $attributes ) = @_; if (wantarray) { my @dbic_rows = $self->_resultset()->search($params, $attributes); return $self->_wrap(@dbic_rows); etc.
Created attachment 53673 [details] [review] Bug 16965: search_related returns an instanciated Koha::Objects-based object Koha::Objects->search_related should return a Koha::Objects-based object. This search_related method should follow the same rules as the search method, i.e. take into account what the caller want (scalar or list). The problem here is that we do not know (in Koha::Objects) what is the kind of objects we want to instanciate. To know it, this patch adds a get_object_class, it will return the class of the object and the resultset Koha::Object-based object. The drawback of this method is that we will have to keep it up-to-date every time we add a new Koha::Object class.
Created attachment 53777 [details] [review] Bug 16965: [Follow-up] Adjust get_object_class In many cases it should be possible to derive the Koha::Objects class name from the DBIx result class name in a trivial manner. If not, the DBIx result class should have a class method called koha_objects_class providing that non-trivial name, such as Libraries for Branch. Note: We are only interested in the plural form here (Koha::Objects). Test plan: Run t/db_dependent/Koha/Objects.t
Jonathan: What do you think of the approach in the third patch?
(In reply to Marcel de Rooy from comment #7) > Jonathan: What do you think of the approach in the third patch? I liked the idea to have the mappings in one place, it will be easier to know which classes are already mapped. Why did you make this change: - return $object_class->_wrap(@dbic_rows); + return _wrap( $object_class, @dbic_rows ); ?
Ccing Tomas, Kyle and Martin to get their opinions on this last patch.
(In reply to Jonathan Druart from comment #8) > Why did you make this change: > - return $object_class->_wrap(@dbic_rows); > + return _wrap( $object_class, @dbic_rows ); > ? Hmm. Somewhere in the process of testing. But we can redo it? We should not expect a Koha Object class to override a private underscored routine. So this should be exactly the same.
(In reply to Marcel de Rooy from comment #10) > (In reply to Jonathan Druart from comment #8) > > Why did you make this change: > > - return $object_class->_wrap(@dbic_rows); > > + return _wrap( $object_class, @dbic_rows ); > > ? > > Hmm. Somewhere in the process of testing. But we can redo it? We should not > expect a Koha Object class to override a private underscored routine. So > this should be exactly the same. Yes, that's the same, that's why I have asked why :)
Hm, I think Marcel is offline for the next 2 weeks (according to his last goodbye on IRC) - can we move on with this somewhow? It's the next bug needed to unlock others in the dependency tree.
To be frank, I think someone needs to go back to basics with me and explain why/what advantages Koha::Objects actually bring us above and beyond DBIx::Class.. It's feeling more and more like we're just wrapping DBIx::Class for the sake of wrapping it and not really winning anything. As such, I can't really comment here.. I use ->search_related allot within a DBIx::Class based app I've written and believe it's certainly a useful method. Beyond that, I can't comment.
(In reply to Martin Renvoize from comment #13) > To be frank, I think someone needs to go back to basics with me and explain > why/what advantages Koha::Objects actually bring us above and beyond > DBIx::Class.. It's feeling more and more like we're just wrapping > DBIx::Class for the sake of wrapping it and not really winning anything. See Bug 17091 - Add AUTOLOAD to Koha::Objects and Bug 17226 - Improve AUTOLOAD of Koha::Object
Created attachment 56009 [details] [review] Bug 16965: Add Koha::Objects->search_related In order to search on relations, we need this new method. Test plan: Confirm that the changes in Objects.t make sense and that the tests pass. Tested all 3 patches together, followed test plan, result OK Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 56010 [details] [review] Bug 16965: search_related returns an instanciated Koha::Objects-based object Koha::Objects->search_related should return a Koha::Objects-based object. This search_related method should follow the same rules as the search method, i.e. take into account what the caller want (scalar or list). The problem here is that we do not know (in Koha::Objects) what is the kind of objects we want to instanciate. To know it, this patch adds a get_object_class, it will return the class of the object and the resultset Koha::Object-based object. The drawback of this method is that we will have to keep it up-to-date every time we add a new Koha::Object class. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 56011 [details] [review] Bug 16965: [Follow-up] Adjust get_object_class In many cases it should be possible to derive the Koha::Objects class name from the DBIx result class name in a trivial manner. If not, the DBIx result class should have a class method called koha_objects_class providing that non-trivial name, such as Libraries for Branch. Note: We are only interested in the plural form here (Koha::Objects). Test plan: Run t/db_dependent/Koha/Objects.t Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 56238 [details] [review] Bug 16965: Add Koha::Objects->search_related In order to search on relations, we need this new method. Test plan: Confirm that the changes in Objects.t make sense and that the tests pass. Tested all 3 patches together, followed test plan, result OK Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 56239 [details] [review] Bug 16965: search_related returns an instanciated Koha::Objects-based object Koha::Objects->search_related should return a Koha::Objects-based object. This search_related method should follow the same rules as the search method, i.e. take into account what the caller want (scalar or list). The problem here is that we do not know (in Koha::Objects) what is the kind of objects we want to instanciate. To know it, this patch adds a get_object_class, it will return the class of the object and the resultset Koha::Object-based object. The drawback of this method is that we will have to keep it up-to-date every time we add a new Koha::Object class. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 56240 [details] [review] Bug 16965: [Follow-up] Adjust get_object_class In many cases it should be possible to derive the Koha::Objects class name from the DBIx result class name in a trivial manner. If not, the DBIx result class should have a class method called koha_objects_class providing that non-trivial name, such as Libraries for Branch. Note: We are only interested in the plural form here (Koha::Objects). Test plan: Run t/db_dependent/Koha/Objects.t Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 56372 [details] [review] Bug 16965: Add Koha::Objects->search_related In order to search on relations, we need this new method. Test plan: Confirm that the changes in Objects.t make sense and that the tests pass. Tested all 3 patches together, followed test plan, result OK Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 56373 [details] [review] Bug 16965: search_related returns an instanciated Koha::Objects-based object Koha::Objects->search_related should return a Koha::Objects-based object. This search_related method should follow the same rules as the search method, i.e. take into account what the caller want (scalar or list). The problem here is that we do not know (in Koha::Objects) what is the kind of objects we want to instanciate. To know it, this patch adds a get_object_class, it will return the class of the object and the resultset Koha::Object-based object. The drawback of this method is that we will have to keep it up-to-date every time we add a new Koha::Object class. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 56374 [details] [review] Bug 16965: [Follow-up] Adjust get_object_class In many cases it should be possible to derive the Koha::Objects class name from the DBIx result class name in a trivial manner. If not, the DBIx result class should have a class method called koha_objects_class providing that non-trivial name, such as Libraries for Branch. Note: We are only interested in the plural form here (Koha::Objects). Test plan: Run t/db_dependent/Koha/Objects.t Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 56375 [details] [review] Bug 16965: Allow Koha::Objects->result_class
So.. I'm mostly OK with these patches.. but I have a related question: What's the history around the continual regeneration of dbic result classes? My general understanding of dbic over the past couple of years is that 'dbicdump' is really about allowing a migration path from a non-dbic system to a system with dbic classes. It's a one time thing, and then you should really maintain the classes and instead generate the db changes from them as opposed to the other way around. If we did this, we could happily rename our resultset and result classes and remove some of the code here that requires mapping from ::Object to ::Result classes. Just something to bare in mind. I could happily write a proof of concept for such a trivial dbic ::Result object example should people be interested in such.
(In reply to Martin Renvoize from comment #25) > So.. I'm mostly OK with these patches.. but I have a related question: > > What's the history around the continual regeneration of dbic result classes? > > > My general understanding of dbic over the past couple of years is that > 'dbicdump' is really about allowing a migration path from a non-dbic system > to a system with dbic classes. It's a one time thing, and then you should > really maintain the classes and instead generate the db changes from them as > opposed to the other way around. If we did this, we could happily rename > our resultset and result classes and remove some of the code here that > requires mapping from ::Object to ::Result classes. > > Just something to bare in mind. I could happily write a proof of concept > for such a trivial dbic ::Result object example should people be interested > in such. Yes, I think we should move forward too and obsolete kohastructure.sql etc. iirc there were problems with deploying (instead of running kohastructure for a new install), perhaps on some specific systems? Running the sql file probably was considered safer. Furthermore we would need to address how to deploy for existing installs in updatedatabase.
This is probably a conversation for elsewhere.. I'm going to pass qa on these for now to prevent a holdup in the queue and in the knowledge that we can fix the dbic usage later fairly trivially and just clean out this code again when we have. For deployment, I actually use DBIC::DeploymentHandler myself in other apps.. it's a great tool, and would happily cope with a move from kohastructure.sql to a versioned schema scheme I believe.. It is however very dependency heavy so we may need to brew our own :(
Created attachment 56650 [details] [review] [PASSED QA] Bug 16965: Add Koha::Objects->search_related In order to search on relations, we need this new method. Test plan: Confirm that the changes in Objects.t make sense and that the tests pass. Tested all 3 patches together, followed test plan, result OK Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 56651 [details] [review] [PASSED QA] Bug 16965: search_related returns an instanciated Koha::Objects-based object Koha::Objects->search_related should return a Koha::Objects-based object. This search_related method should follow the same rules as the search method, i.e. take into account what the caller want (scalar or list). The problem here is that we do not know (in Koha::Objects) what is the kind of objects we want to instanciate. To know it, this patch adds a get_object_class, it will return the class of the object and the resultset Koha::Object-based object. The drawback of this method is that we will have to keep it up-to-date every time we add a new Koha::Object class. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 56652 [details] [review] [PASSED QA] Bug 16965: [Follow-up] Adjust get_object_class In many cases it should be possible to derive the Koha::Objects class name from the DBIx result class name in a trivial manner. If not, the DBIx result class should have a class method called koha_objects_class providing that non-trivial name, such as Libraries for Branch. Note: We are only interested in the plural form here (Koha::Objects). Test plan: Run t/db_dependent/Koha/Objects.t Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 56653 [details] [review] [PASSED QA] Bug 16965: Allow Koha::Objects->result_class Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
So much as I'm an opponent of Koha::Objects (They still just feel like boilerplate for the point of boilerplate to me), I can't find anything desperately wrong with this code. Passing QA
Pushed to master for 16.11, thanks Jonathan, Marcel!