Skip to content

Fixed bug #70469 SoapClient's fatal error in error_get_last() #2899

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
wants to merge 1 commit into from
Closed

Fixed bug #70469 SoapClient's fatal error in error_get_last() #2899

wants to merge 1 commit into from

Conversation

artmnv
Copy link
Contributor

@artmnv artmnv commented Nov 4, 2017

Hello! Fixed bug https://bugs.php.net/bug.php?id=70469 when you can get fatal error from error_get_last() even if SoapClient is in exception mode.

This issue first appeared in commit a830b0f . The issue was about server side error logging but patch changed client side too. I think we don't want silent errors in SoapClient in exception mode. Please correct me if I'm wrong.

@artmnv
Copy link
Contributor Author

artmnv commented Nov 4, 2017

Travis failed in the same tests as PHP-7.0 branch. I'll wait for PHP-7.0 to come green.

@krakjoe krakjoe added the Bug label Nov 6, 2017
@nikic
Copy link
Member

nikic commented Nov 22, 2017

I've merged this patch into master as 1dfcd1a. I'm not applying this to any release branches, as this may have BC impact -- after all there was explicit code to throw this error, so I'm apprehensive about simply dropping it. Maybe people were using this to automatically log errors ... or something? I can be convinced to land this in 7.1+ though, if someone has more background about why the code was there in the first place.

@nikic nikic closed this Nov 22, 2017
@nikic
Copy link
Member

nikic commented Nov 22, 2017

I'm also wondering why the "exceptions"=>false mode is even a thing. When would it ever be a good idea to generate fatal errors rather than exceptions? If we were talking about warnings vs. exceptions okay, but fatal errors seem strictly worse.

@nikic
Copy link
Member

nikic commented Mar 23, 2018

Just for the record, this got applied to 7.1.14 and 7.2.2 at a later point as well.

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.

3 participants