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.

Sign up for GitHub

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

Jump to bottom

Exception when persisting class unlisted in disciminator map (Issue 867) #1887

Merged
merged 3 commits into from
Nov 30, 2018
Merged

Exception when persisting class unlisted in disciminator map (Issue 867) #1887

merged 3 commits into from
Nov 30, 2018

Conversation

watari
Copy link
Contributor

@watari watari commented Oct 30, 2018 •
edited
Loading

Q A
Type feature/improvement
BC Break yes
Fixed issues #867

Summary

Updated persisting logic to throw exception of class is not listed in discriminator map.

There is no new tests is added since library have a set of old tests that is cover most of the cases.

Right now exception message contain information only about unlisted class without parent class since in the places of the checks I don't find a way of extracting parent class info.

@watari watari force-pushed the issue_867 branch 2 times, most recently from a46ec90 to fb84eea Compare October 30, 2018 19:05
@alcaeus alcaeus self-requested a review November 1, 2018 20:34
malarzm
malarzm requested changes Nov 1, 2018
View reviewed changes
lib/Doctrine/ODM/MongoDB/DocumentManager.php Outdated Show resolved Hide resolved
lib/Doctrine/ODM/MongoDB/DocumentManager.php Outdated Show resolved Hide resolved
lib/Doctrine/ODM/MongoDB/DocumentManager.php Outdated Show resolved Hide resolved
@malarzm
Copy link
Member

malarzm commented Nov 1, 2018

This logic forbid use of empty discriminator map. Now all embedded and referenced documents must have target document or discriminator map.

This answers one of my comments :) I don't think we should go that far, not specifying discriminatorMap is IMO very useful and the only strictness we should introduce is, as per #867, throwing an exception if user has opted in for using a map AND document being persisted is not listed there.

@watari
Copy link
Contributor Author

watari commented Nov 4, 2018

This logic forbid use of empty discriminator map. Now all embedded and referenced documents must have target document or discriminator map.

This answers one of my comments :) I don't think we should go that far, not specifying discriminatorMap is IMO very useful and the only strictness we should introduce is, as per #867, throwing an exception if user has opted in for using a map AND document being persisted is not listed there.

@malarzm , I really gone to far with strictness). I will decrease checks for making discriminatorMap optional. Can you, please, check my answers to your comments?

@malarzm
Copy link
Member

malarzm commented Nov 5, 2018

@watari I think I've answered all questions, but I think that the best course of action would be to create this PR once again and only introducing the exception. It should be easier than reverting all changes that will no longer be needed :)

@watari watari force-pushed the issue_867 branch 2 times, most recently from 804b1c2 to b577752 Compare November 9, 2018 05:56
@watari
Copy link
Contributor Author

watari commented Nov 9, 2018 •
edited
Loading

@malarzm , it was not so hard to revert partially changes and introduce only exception in this PR). Also I updated PR description. Can you, please, check all again?

@watari
Removed additional checks of class existence for targetDocument and d…
a15ec3b 
…iscriminator map from ClassMetadata class. Replaced tests and docs class name literals by `::class` constants. Updated docs according to new behaviour with unregistered classes.
@watari
Copy link
Contributor Author

watari commented Nov 19, 2018

@alcaeus , @malarzm , is there will be any other remarks about PR? All previous remarks is already implemented.

@alcaeus alcaeus mentioned this pull request Nov 26, 2018
Closed
alcaeus
alcaeus reviewed Nov 28, 2018
View reviewed changes
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

Apart from documentation wording, this looks good! Great work @watari!

docs/en/reference/annotations-reference.rst Outdated Show resolved Hide resolved
@alcaeus alcaeus self-assigned this Nov 28, 2018
@alcaeus alcaeus added this to the 2.0.0 milestone Nov 28, 2018
@watari
Copy link
Contributor Author

watari commented Nov 28, 2018

Updated docs according to suggestions

alcaeus
alcaeus approved these changes Nov 28, 2018
View reviewed changes
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

Thanks @watari! 🎉

@alcaeus alcaeus merged commit ae9680d into doctrine:master Nov 30, 2018
@alcaeus alcaeus added the Task label Dec 23, 2018
alcaeus added a commit to alcaeus/mongodb-odm that referenced this pull request Jan 11, 2019
@alcaeus
Add deprecation layer for unlisted classes in discriminator map (doct…
f96f136 
…rine#1887)
alcaeus added a commit to alcaeus/mongodb-odm that referenced this pull request Jan 11, 2019
@alcaeus
Add deprecation layer for unlisted classes in discriminator map (doct…
84eee94 
…rine#1887)
@alcaeus alcaeus mentioned this pull request Jan 11, 2019
Merged
alcaeus added a commit to alcaeus/mongodb-odm that referenced this pull request Jan 14, 2019
@alcaeus
Add deprecation layer for unlisted classes in discriminator map (doct…
57e4228 
…rine#1887)
@alcaeus alcaeus mentioned this pull request Apr 12, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmikola jmikola jmikola left review comments

@malarzm malarzm malarzm approved these changes

@alcaeus alcaeus alcaeus approved these changes

Assignees

@alcaeus alcaeus

Labels
BC Break Task
Projects
None yet
Milestone
2.0.0-Beta1
Development

Successfully merging this pull request may close these issues.
4 participants
@watari @malarzm @jmikola @alcaeus