-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Convert SPL Exception usage to normal Error #4992
Conversation
If there are no objections I'll merge this in a week. |
@cmb69 @kocsismate Any thoughts on the general idea here? Changing existing exception types seems somewhat more problematic from a BC perspective to me, and not sure it really buys us much. |
At the first sight, this change seemed too risky for me, but I am wondering if it is really the case: As far as I saw, these are programming errors. Thus, I would be surprised if people caught the thrown exceptions "directly". What it means for me is that it might not matter much for the end users what kind of exception is actually thrown... And as people already have to have a general error handler (catching |
d402fb3
to
9869eb9
Compare
ext/spl/php_spl.c
Outdated
} | ||
if (error) { | ||
efree(error); | ||
} | ||
zend_string_release_ex(func_name, 0); | ||
RETURN_FALSE; | ||
return; |
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.
All these return
s should be RETURN_THROWS()
ideally. See #5036
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.
Huh, I though that you went through them that's why I needed to manually fix the merge conflicts. Will do the remaining ones
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.
Ah, no, please ignore. I didn't add RETURN_THROWS()
previously myself here, because it's not sure that an exception is thrown. But then what is the purpose of changing the return type?
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 didn't look at the specific case until I opened my editor. But yeah now I'm not sure why I changed the return type ...
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.
Maybe
if (do_throw) {
RETURN_THROWS();
} else {
RETURN_FALSE;
}
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've covered all the case, please let me know if there are some that I missed.
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.
For me, the PR itself looks fine (apart from the few issues with RETURN_THROWS()
), but we should try to address Nikita's concern. :)
9869eb9
to
47844c4
Compare
47844c4
to
46ef395
Compare
Should I move forwards with this, post on internals, or drop this change? |
46ef395
to
b4983a1
Compare
Any other opinions? |
b4983a1
to
fe54531
Compare
fe54531
to
19e9a15
Compare
Thinking about this, it seems to me that it's more of a problem that we still support some of these exceptions not to be thrown at all ( |
SPL is truly an interesting extension, I didn't even know you could do that. Are there only a couple of function which toggle this behaviour, and if so is there a list? (could always read the docs to get an idea of haw many there are ...) |
|
19e9a15
to
73273dc
Compare
73273dc
to
fb77ee0
Compare
fb77ee0
to
2e27775
Compare
I've made some conversions too while I was updating the errors to the new format, don't know if I should have split or not but the easiest would be to split per Class if needed. @kocsismate can you have a look at the error messages? Also still need to do iterators and observers classes. |
@Girgias I'll have a look for sure! However, I think we should first decide if we can really change the exceptions to errors. @nikic? @cmb69? I for one would be in favour, because this is what we use everywhere (with the exception of sodium, as far as I can tell). However, I don't know much about the BC implications... My guess is that the change wouldn't be very problematic, since I would expect that people generally don't catch |
Well, we also have to consider existing test suites; I'm not sure that changing the exception types is a good idea for now. |
Yes, that makes sense. In this case, some of the |
2e27775
to
2816653
Compare
Some conversions, some trying to wrap my brain around, maybe a bugfix also
3061f5c
to
27e55df
Compare
Closing because this is old and rebasing it is a bit of a PITA. Might reopen a similar one just converting Uninitialized/already initialized objects to a standard Error. |
It seems to be me that it is wiser to convert those Exceptions to (Type|ArgumentCount|Value)Error then to leave the SPL Exceptions.
Thoughts?