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 #2 #1248

Closed
wants to merge 10 commits into from

Conversation

flip111
Copy link
Contributor

@flip111 flip111 commented Jan 13, 2015

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.

Previous: #1016

flip111 added 3 commits April 23, 2014 16:50
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-3490

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

$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.

Don't catch a generic exception here: be specific

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how i can track all the possible situations which yield all possible Exceptions

Copy link
Member

Choose a reason for hiding this comment

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

@flip111 writing these tests isn't that problematic if you keep the test asset as simple as possible.

@Ocramius
Copy link
Member

@flip111 also, please provide tests for this enhancement

`@return self` trend break with `@return ORMInvalidArgumentException`
Add unused parameters for `invalidAssociation`
@flip111
Copy link
Contributor Author

flip111 commented Jan 13, 2015

@Ocramius i totally lost track (since 10 months) how i even arrived at this situation where this error would be useful. I hope not to forgot the PR this time though ^^

removed one `)` too many
rename duplicate method
@Ocramius
Copy link
Member

@flip111 the original PR was probably trying to give a better meaning to stuff like:

class Foo
{
    /** @ManyToOne(targetEntity="Bar") */
    public $bar;
}

$foo = new Foo;

$foo->bar = 'baz'; // wrong - exception
$foo->bar = []; // wrong - exception
$foo->bar = new ArrayCollection; // wrong - exception
$foo->bar = new stdClass; // wrong - exception
$foo->bar = new Bar; // correct

remove added `value` to exception
@fprochazka
Copy link
Contributor

I would strongly discourage you from writing code in github editor. Checkout the branch and fix it locally. It's more effective and IDE tells about problems github won't.

@Ocramius Ocramius self-assigned this Jan 17, 2015
@Ocramius Ocramius closed this in 39766e6 Jan 18, 2015
@Ocramius
Copy link
Member

This was manually rebased/tested and merged into master at 39766e6

Thanks @flip111!

@flip111
Copy link
Contributor Author

flip111 commented Jan 18, 2015

Thank you @Ocramius i had already a hard time imagining being able to finish this PR

@Ocramius
Copy link
Member

@flip111 check the merge commit, the test was not that terrible after all :-)

@flip111
Copy link
Contributor Author

flip111 commented Jan 18, 2015

Ye i know it's not a big change. Just that Doctrine internal code is pretty hard to get into. I guess that goes for any ORMs though..

I checked the commit, you added two cases on the 2000+ lines, that's nice. I remember when i was testing i needed that catch around line 710 ... perhaps that has changed by now.

@Ocramius
Copy link
Member

@flip111 yeah, the association exception throwing was enough :-)

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.

5 participants