Skip to content
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

Fixed bug #75169 (BCMath errors/warnings bypass error handling) #2745

Merged
merged 1 commit into from Sep 13, 2017

Conversation

@cmb69
Copy link
Contributor

commented Sep 9, 2017

Instead of writing warning messages to stderr, we employ PHP's error
handling to raise E_WARNING even for the single case where
bc_rt_error() has been called, since that did not actually error out.
We choose to call php_error_docref() directly in libbcmath, since
there is no upstream, and since other PHP core functionality is already
used in our bundled libbcmath. Accordingly, we remove rt.c so it will
not be accidentally used in the future.

Besides adapting a few existing tests, we add new tests so that the
warnings are tested at least once. We also get rid of the Windows
specific tests, since the warning behavior is now supposed to be
platform-agnostic.

Instead of writing warning messages to `stderr`, we employ PHP's error
handling to raise `E_WARNING` even for the single case where
`bc_rt_error()` has been called, since that did not actually error out.
We choose to call `php_error_docref()` directly in libbcmath, since
there is no upstream, and since other PHP core functionality is already
used in our bundled libbcmath. Accordingly, we remove `rt.c` so it will
not be accidentally used in the future.

Besides adapting a few existing tests, we add new tests so that the
warnings are tested at least once. We also get rid of the Windows
specific tests, since the warning behavior is now supposed to be
platform-agnostic.
@krakjoe krakjoe added the Bugfix label Sep 11, 2017
@cmb69

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2017

Apparently, this is uncontroversial, so I'm going to merge into master.

@php-pulls php-pulls merged commit fd73a54 into php:master Sep 13, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cmb69 cmb69 deleted the cmb69:bcmath-warnings branch Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.