Skip to content

Conversation

@trowski
Copy link
Member

@trowski trowski commented Jun 11, 2016

This PR converts most occurrences in extensions of E_ERROR with thrown Error objects. Not all fatal errors were converted to exceptions. Those dealing with memory allocation or other unrecoverable errors have remained fatal. I also did not covert any fatal errors in the Soap extension. The extension appears to rely heavily on bailout behavior, so fatals could not easily be converted to thrown exceptions. Perhaps someone with more experience with the Soap extension would be able to do so.

This PR supersedes #1388.


RFC: https://wiki.php.net/rfc/throw_error_in_extensions

@trowski trowski force-pushed the throw-error-in-extensions branch from d5313d2 to 771e5cc Compare June 13, 2016 14:02
@trowski trowski force-pushed the throw-error-in-extensions branch from 27a165a to 7d53864 Compare June 14, 2016 18:18
@hoshsadiq
Copy link

Perhaps a good idea to add tests to ensure Throwable and/or Error are caught

@trowski trowski force-pushed the throw-error-in-extensions branch 3 times, most recently from 1001fb9 to 7d53864 Compare June 18, 2016 14:44
@laruence laruence added the RFC label Jun 28, 2016
if (Z_TYPE(CE_STATIC_MEMBERS(intern->ce)[ref->prop.offset]) == IS_UNDEF) {
php_error_docref(NULL, E_ERROR, "Internal error: Could not find the property %s::%s", ZSTR_VAL(intern->ce->name), ZSTR_VAL(ref->prop.name));
zend_throw_error(NULL, "Internal error: Could not find the property %s::%s", ZSTR_VAL(intern->ce->name), ZSTR_VAL(ref->prop.name));
/* Bails out */
Copy link
Member

Choose a reason for hiding this comment

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

should recover and return

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, missed this one.

@php-pulls php-pulls merged commit e9832b5 into php:master Jul 5, 2016
@trowski trowski deleted the throw-error-in-extensions branch April 28, 2021 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants