Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use class name as detail if exception message is not set #7

Merged
merged 1 commit into from
Jun 21, 2016

Conversation

clovnrian
Copy link

Exception message isn't mandatory and if isn't set the exception is thrown. In many cases the exception class name is self explained, so if message is not set use the class name instead.

@@ -135,7 +135,9 @@ public static function fromException(Exception $exception, $title = '', $type =
$type = self::RFC2616;
}

return new self($code, $exception->getMessage(), $title, $type, $additionalDetails);
$detail = empty($exception->getMessage()) ? get_class($exception) : $exception->getMessage();
Copy link
Owner

Choose a reason for hiding this comment

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

I like the idea but I've got some concerns.

First I would not leak the fully qualified namespace. How about only the exception name?

Yet, this brings us to a second case. Still I find it risky because for instance capturing a PDOException and returning the class name reveals it's SQL problem (query, timeout, locked table, etc).

Copy link
Owner

@nilportugues nilportugues Jun 21, 2016

Choose a reason for hiding this comment

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

I've got to agree, the getMessage() approach may lead to the same situation.

@clovnrian let me think about it

@clovnrian
Copy link
Author

Yes, you have right, only class name would be better.

And about the security... In your example the class name will tell you only what is the problem, but message will give you the SQL which is much worse.

@nilportugues
Copy link
Owner

nilportugues commented Jun 21, 2016

@clovnrian get the class name only, instead of the namespaced version, and I'm merging this

@nilportugues nilportugues merged commit 4488828 into nilportugues:master Jun 21, 2016
@nilportugues
Copy link
Owner

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

Successfully merging this pull request may close these issues.

2 participants