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

improved error handling for invalid association values #1016

Closed
wants to merge 1 commit into from

Conversation

flip111
Copy link
Contributor

@flip111 flip111 commented Apr 23, 2014

Possibly to do:

  1. Make custom Exception for line 713
  2. Make custom Exception for line 817
  3. Does the object check on line 816 slow down the code too much? Alternatively a try-catch could be put around line 1415 or higher up.

Possibly to do:
1. Make custom Exception for line 713
2. Make custom Exception for line 817
3. Does the object check on line 816 slow down the code too much? Alternatively a try-catch could be put around line 1415 or higher up.
@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3098

We use Jira to track the state of pull requests and the versions they got
included in.

@stof
Copy link
Member

stof commented Apr 23, 2014

IMO, the var_export should be removed from the exception message. IT could make it pretty unreadable

and yes, you should not use the base exception class. Use the ORMInvalidArgumentException here

@flip111
Copy link
Contributor Author

flip111 commented Apr 23, 2014

@stof i considered invalidObject::invalidObject exception, just that this one talks about a parameter, but this is about a property. That made me doubt if ORMInvalidArgumentException is the right class to put this as it's not clear to me if the ArgumentException is about the internal Doctrine arguments or the programmer argument. In case ORMInvalidArgumentException a new method should be created imo.

Do you have a good alternative to var_export? i could only use gettype (like the invalidObject method), but showing the actual value might be more expressive?

Edit:
Stof, are you suggesting to use ORMInvalidArgumentException for both line 713 and 817? The exception on line 817 needs to have a value attached so might be better to use a different class all together.

$this->computeAssociationChanges($assoc, $val);
try {
$this->computeAssociationChanges($assoc, $val);
} catch (\Exception $ex) {
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't catch generic exceptions here.

@Ocramius Ocramius self-assigned this Jan 13, 2015
@Ocramius
Copy link
Member

Closing as incomplete.

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

Successfully merging this pull request may close these issues.

4 participants