-
Notifications
You must be signed in to change notification settings - Fork 7.9k
SPL use standard Error instead of custom exception (again) #6115
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
SPL use standard Error instead of custom exception (again) #6115
Conversation
@kocsismate and @nikic can I get your opinion on this once again as this is a trimmed down version of #4992? |
edc69ac
to
167c32c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm okay with these changes. They're pretty conservative and align exception usage with other extensions. E.g. the "An iterator cannot be used with foreach by reference" case is also present in many other exts and always uses Error
.
ext/spl/spl_directory.c
Outdated
zend_throw_exception_ex(spl_ce_LogicException, 0, | ||
"The parent constructor was not called: the object is in an " | ||
"invalid state "); | ||
zend_throw_error(NULL, "The parent constructor was not called: the object is in an invalid state "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zend_throw_error(NULL, "The parent constructor was not called: the object is in an invalid state "); | |
zend_throw_error(NULL, "The parent constructor was not called: the object is in an invalid state"); |
@kocsismate Thoughts? |
@nikic I'm also in favour of these changes, and I also believe they should be safe enough to do. :) |
167c32c
to
9192e2d
Compare
Slightly more conservative then last time as those are programming errors and therefore should probably use Errors instead of Exceptions.