Skip to content

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

Closed

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Sep 12, 2020

Slightly more conservative then last time as those are programming errors and therefore should probably use Errors instead of Exceptions.

@Girgias
Copy link
Member Author

Girgias commented Sep 13, 2020

@kocsismate and @nikic can I get your opinion on this once again as this is a trimmed down version of #4992?

@Girgias Girgias force-pushed the sqpl-use-error-instead-custom-exceptio-v2 branch from edc69ac to 167c32c Compare September 13, 2020 18:56
Copy link
Member

@nikic nikic left a 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.

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 ");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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");

@nikic
Copy link
Member

nikic commented Sep 14, 2020

@kocsismate Thoughts?

@kocsismate
Copy link
Member

@nikic I'm also in favour of these changes, and I also believe they should be safe enough to do. :)

@Girgias Girgias force-pushed the sqpl-use-error-instead-custom-exceptio-v2 branch from 167c32c to 9192e2d Compare September 14, 2020 15:40
@Girgias
Copy link
Member Author

Girgias commented Sep 15, 2020

Merged as 9affbef, b620733, and 063fdd9

@Girgias Girgias closed this Sep 15, 2020
@Girgias Girgias deleted the sqpl-use-error-instead-custom-exceptio-v2 branch September 15, 2020 10:52
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.

3 participants