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

Error handling #64

Merged
merged 2 commits into from May 22, 2013
Merged

Error handling #64

merged 2 commits into from May 22, 2013

Conversation

@cboden
Copy link
Contributor

@cboden cboden commented May 19, 2013

In the event a class' constructor throws an exception I think it would be better if that exception was thrown, not a DependencyException.

The code using the Container can more specifically target handling exceptions.

Most importantly this gives a more "honest" stack trace.

cboden added 2 commits May 19, 2013
Let the class constructor exception propagate.
@domu1de
Copy link
Contributor

@domu1de domu1de commented May 20, 2013

Inner exceptions would be a way, too.

@mnapoli mnapoli mentioned this pull request May 21, 2013
@mnapoli
Copy link
Member

@mnapoli mnapoli commented May 21, 2013

I see what you mean, I have merged locally your changes and proposed an alternative solution on #65.

Basically, I agree with you, except when it's a specific PHP-DI exception. Look at those 2 error message a user can get when getting an object from the container:

No entry or class found for 'nonExistentEntry'

or:

Error while injecting dependencies into IntegrationTests\DI\Fixtures\ConstructorInjectionTest\Buggy2: No entry or class found for 'nonExistentEntry'

I find the first message not explicit enough.

So feel free to give your opinion on the new PR.

(btw if that's what you meant @domu1de currently exceptions are "chained", see line 77)

@domu1de
Copy link
Contributor

@domu1de domu1de commented May 22, 2013

@mnapoli Oh, I missed that part. ;)

@mnapoli mnapoli merged commit 2c4b950 into PHP-DI:master May 22, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.