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

Disallow orphan removal attribute on many-to-one #6772

Merged

Conversation

greg0ire
Copy link
Member

It only makes sense for collections, and there is no collection here.
Plus the docs do not say it is supported.
See
http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/working-with-associations.html#orphan-removal

It only makes sense for collections, and there is no collection here.
Plus the docs do not say it is supported.
See
http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/working-with-associations.html#orphan-removal
@Ocramius
Copy link
Member

@greg0ire please write a test verifying that it DOESN'T work

@greg0ire
Copy link
Member Author

greg0ire commented Oct 13, 2017

@Ocramius just to be sure, you want me to create an XML mapping with that attribute and test that it doesn't validate against the XSD?

@greg0ire
Copy link
Member Author

It might be my lazyness speaking, but I don't see the point of this test:

  1. if someone wants to add that back, you are probably going to ask them to provide a test ensuring it works, which would make a lot more sense, and they will remove mine. A safeguard against someone sneakily adding it seems overkill.
  2. on a semi-trolling note, if I add a test for orphan-removal, why not add it for all the other possible combinations of english words?

@greg0ire
Copy link
Member Author

If you mean I should test that the expected behavior does not work, the expected behavior is not defined.

@Ocramius
Copy link
Member

@greg0ire no, I'd want a test that verifies that the orphan removal doesn't kick in :-)

@greg0ire
Copy link
Member Author

So:

  1. create an invalid mapping with that, which makes Validate XML mapping against XSD file #6728 harder :/
  2. set the reference to the "one" side to null
  3. persist
  4. clear
  5. assert findOne still returns something

@gerryvdm
Copy link

How should one write a test for this? With a many-to-one there is no collection to manipulate and subsequently trigger this behavior.

@ciaranmcnulty
Copy link

The set of 'things that don't happen' is quite large, surely?

@Ocramius
Copy link
Member

Ocramius commented Oct 13, 2017 via email

@greg0ire
Copy link
Member Author

Working on it.

greg0ire added a commit to greg0ire/doctrine-orm that referenced this pull request Oct 13, 2017
@greg0ire greg0ire force-pushed the disallow_many_to_one_orphan_removal branch from 8cf9448 to c7522a3 Compare October 13, 2017 20:58
greg0ire added a commit to greg0ire/doctrine-orm that referenced this pull request Oct 13, 2017
@greg0ire greg0ire force-pushed the disallow_many_to_one_orphan_removal branch 3 times, most recently from 170046a to 72c2d91 Compare October 13, 2017 21:12
@greg0ire
Copy link
Member Author

greg0ire commented Oct 13, 2017

And it only passes with sqlite but not database that ensure integrity. What do I do now @Ocramius ? setup a null cascade? Or do I remove this commit now that we have proof?

@greg0ire
Copy link
Member Author

greg0ire commented Oct 14, 2017

I went ahead and setup a set null cascade, now the test pass with mysql, but not pg:

SELECT UUID_GENERATE_V4()

               ^

HINT:  No function matches the given name and argument types. You might need to add explicit type casts.

How do I do this: https://stackoverflow.com/questions/12505158/generating-a-uuid-in-postgres-for-insert-statement#12505220 ?

EDIT: let's just not do it and set uuids manually

@greg0ire greg0ire force-pushed the disallow_many_to_one_orphan_removal branch 4 times, most recently from a137d7d to 2a86517 Compare October 14, 2017 10:40
@greg0ire
Copy link
Member Author

greg0ire commented Oct 14, 2017

I moved the proof to #6774 so that you can decide whether to merge both or just this one more easily, and in the order you see fit.

@guilhermeblanco guilhermeblanco merged commit e279dfa into doctrine:master Oct 16, 2017
@greg0ire greg0ire deleted the disallow_many_to_one_orphan_removal branch October 16, 2017 11:53
@Majkl578 Majkl578 added this to the 2.6.0 milestone Dec 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants