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

Utilise $owns for recursive duplication #28

Closed
2 tasks done
pitchandtone opened this issue Jul 13, 2017 · 13 comments
Closed
2 tasks done

Utilise $owns for recursive duplication #28

pitchandtone opened this issue Jul 13, 2017 · 13 comments

Comments

@pitchandtone
Copy link

pitchandtone commented Jul 13, 2017

Since $owns handles versioning cascade, and should handle unpublish, we should also be able to imply duplication (similar to publish). The logic should be similar to the recursive publish method.

Acceptance Criterea:

  • Any duplicated object will include all owned objects (or duplicates as necessary) from the original
  • Developers can customise the list of objects that are duplicated automatically
  • Documentation describes how this works
  • Tests cover any new features

PRs

@pitchandtone
Copy link
Author

I have supplied an example of how we've handled this on Elemental here: #25 (comment)

@pitchandtone
Copy link
Author

@sminnee, @chillu, @tractorcow have also made similar comments here:
#23 (comment)
#23 (comment)
#23 (comment)

@tractorcow
Copy link
Contributor

At the moment we have the following method signature for duplicate:

public function duplicate($doWrite = true, $manyMany = 'many_many')

What we could do is change it to

public function duplicate($doWrite = true, $withOwned = true)

So that it allows you to duplicate owned objects, rather than trying to infer it from the relationship type.

Note that many_many currently only duplicates the mapping table.

We could use the following behaviour:

  • $withOwned = true, duplicates owned many_many mapping table, duplicate actual object for has_one and has_many.
  • $withOwned = false, duplicate only this object and not related.

@pitchandtone would love to hear your feelings. :)

@pitchandtone
Copy link
Author

@tractorcow wouldn't matching the publish api signature be better? duplicateRecursive and duplicateSingle? and then deprecating duplicate for v5, but calling Recursive if $withOwned = true

@sminnee
Copy link
Member

sminnee commented Jul 14, 2017

+1 for API consistency.

@chillu
Copy link
Member

chillu commented Jul 14, 2017

The ticket title reads "Utilise $owns for recursive duplication and deletion", but the comments only discuss duplication. On #23 the consensus seems to be that we can't infer delete/unpublish from ownership. Should we limit this ticket to duplication?

@tractorcow tractorcow changed the title Utilise $owns for recursive duplication and deletion Utilise $owns for recursive duplication Jul 14, 2017
@tractorcow
Copy link
Contributor

Renamed.

@tractorcow
Copy link
Contributor

@tractorcow wouldn't matching the publish api signature be better? duplicateRecursive and duplicateSingle? and then deprecating duplicate for v5, but calling Recursive if $withOwned = true

This is fine, since duplicate() is dataobject, but duplicateRecursive() / duplicateSingle() can be versioned specific.

This will make it opt-in though.

@tractorcow tractorcow added this to the Recipe 4.0.0-beta2 milestone Jul 14, 2017
@sminnee
Copy link
Member

sminnee commented Jul 14, 2017

My view is:

  • Publication & duplication: inferred from owns
  • Deletion & unpublishing: inferred from cascade_deletes

"Owns" kind of means "cascade_creates"

Make sense?

@tractorcow
Copy link
Contributor

Yes I think so.

You agree that with owned many_many, we should only need to duplicate the mapping though, correct?

@chillu
Copy link
Member

chillu commented Jan 18, 2018

Moving this to impact/high so it stays on our radar, since it's a co-fund commitment.

@tractorcow tractorcow self-assigned this Jan 26, 2018
@tractorcow
Copy link
Contributor

WIP branches:

I'm going to have a cascade_duplications that will let you override owns so you have a degree of control.

@chillu chillu self-assigned this Feb 1, 2018
@flamerohr
Copy link
Contributor

API has been merged, recursive duplication is now available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants