-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce inheritance, fix #91 #103
Conversation
having helpers inherit from Row, ResultSet &c causes problems when loading multiple components: some base classes appear before components in C3 resolution, and component methods get skipped See for example issue frioux#91
the problem happens when iheriting from a class that `load_components` I suspect the root of the problem is that the derived class doesn't use C3
Yep, confirmed, adding |
@@ -5,9 +5,7 @@ package DBIx::Class::Helper::ResultSet::IgnoreWantarray; | |||
use strict; | |||
use warnings; | |||
|
|||
use parent 'DBIx::Class::ResultSet'; | |||
|
|||
sub search :DBIC_method_is_indirect_sugar{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove this attribute? Is this no longer needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I grepped for it in the source code of the latest released DBIC, and couldn't find it, so my tests failed. I don't know how they pass for you and CI, I suspect I'm missing something crucial.
Unless you can show a concrete improvement from this patchset I'm inclined to close it. This is what DBIx::Class::Helper::Schema::Verifier::C3 was built to automate: people must use c3, one way or another, with DBIC. |
Very fair! Maybe add a line of documentation in https://github.com/frioux/DBIx-Class-Helpers/blob/master/lib/DBIx/Class/Helper/ResultSet.pm and to a Result helper or doc-only package, explaining that? One of the problems is that Schema::Loader puts a |
yeah they either need to use mro 'c3' or I think calling load_components does that for you as well. |
I reported the suggestion to Schema::Loader https://rt.cpan.org/Ticket/Display.html?id=132035 |
Cool. Gonna close this one now. Thanks! |
I have removed most
use parent
, and replaced the necessary ones with the smallest needed class.I've also added a test showing the problem reported in issue #91. I think the root of the problem is that derived classes that don't call
load_components
don't get the C3 mro.All tests still pass on my machine.