Skip to content
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

Ability to override cascade_copy when calling Row->copy #58

Closed
wants to merge 1 commit into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Aug 26, 2014

Simple modification adding an optional argument to the Class::Row->copy method allowing you to force overriding of individual relationship's cascade_copy attribute.

@@ -1119,7 +1119,7 @@ sub set_inflated_columns {

=head2 copy

my $copy = $orig->copy({ change => $to, ... });
my $copy = $orig->copy({ change => $to, ... }, $dont_cascade?);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very limited (and limiting, extensibility-wise) API. I suggest a hash ref with { cascade => { rel_one => 1, rel_two => 0 } }, so you can override both ways (rels not mentioned in the cascade hash maintain the value specified in their definition) and add more options in future.

@ilmari
Copy link
Contributor

ilmari commented Aug 27, 2014

Thanks for the contribution. However, see my comment on the API above. Also, could you please add some tests for the new behaviour?

@ghost
Copy link
Author

ghost commented Aug 27, 2014

Thanks for the feedback. I'll work on implementing the suggested changes when possible. Likely this weekend.

@ilmari
Copy link
Contributor

ilmari commented Sep 2, 2014

After discussing this on IRC with @ribasushi, I realised that to work generally in DBIC, it the data structure would have to be able to specify whether to cascade the copy recursively for all relationships, which gets more complicated. It'd have to be something like this, where the value gets passed on to the related ->copy call if it's a hashref:

{
    cascade => {
        rel_one => {            # implicitly true
            nested_rel => 0,
        },
        rel_two => 0,            # can't nest off false
        rel_three => 1,          # further rels get default
    },
}

If you don't have the time or inclination to do all this work (which is perfectly okay), I suggest implementing just what you need as a component that overrides ->copy and localises the cascade_copy attributes as needed.

@ghost
Copy link
Author

ghost commented Sep 2, 2014

I'll still take a stab at it. Wanted to get to it this weekend but I was recovering from jet lag.

@ribasushi
Copy link
Collaborator

@mikegrb What @ilmari didn't mention was that even with a well though-through API this use-case is too specific to be a part of the DBIC core. That is, while the code in either case is identical, the feature itself should be available in a DBIx::Class:: via an override of copy(). Then a user that needs this would load it in the base Result class via load_components() and be on their way.

Thus I am going to close this PR for the time being. Let me know if you have further question. In particular if you disagree with the above PLEASE don't hesitate to reopen this PR and articulate your point further.

@ribasushi ribasushi closed this Sep 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants