Add a non-numeric value handler for arithmetic operators - #146
Open
javiermarinros wants to merge 3 commits into
Open
Add a non-numeric value handler for arithmetic operators#146javiermarinros wants to merge 3 commits into
javiermarinros wants to merge 3 commits into
Conversation
Arithmetic and ordering operators hand their operands straight to PHP, so
a value that is not a number leaks a raw \TypeError out of the library
("Unsupported operand types: string / string") instead of one of its own
NXP\Exception\* types, and there is no supported way to say what such a
value should mean short of re-registering every arithmetic operator.
setNonNumericHandler() takes a callable($value, $operator) whose return
value is used in place of the original operand. It is applied to the
operators that require a number (+, -, *, /, %, ^, uNeg, uPos, >, >=, <
and <=), including the ones redefined by setDivisionByZeroIsZero() and
useBCMath(). Operators with defined string or boolean semantics (==, !=,
&&, || and !) are left untouched, as are numeric, null and boolean values.
Without a handler the behaviour is unchanged, which the existing suite and
the new testNonNumericWithoutHandler cases assert.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018EWvvUoaHC1SaNQ23nVrCk
Two cases where installing a handler changed results that were already meaningful without one, breaking the promise that the handler only affects values that have no sensible numeric reading: Arrays are a supported variable type, accepted by defaultVarValidation and used by avg(), min() and max(), and PHP gives + a defined meaning for them. "[1, 2] + [3, 4]" returned the array union but 0 once any handler was set, and the handler was invoked with an array in a parameter documented as a scalar. Arrays now short circuit in normalizeOperand along with numeric, null and boolean values. The ordering operators normalized both operands unconditionally, so "'apple' < 'banana'" flipped from true to false once a handler was set, while == and != deliberately keep their strcmp semantics. Comparing two non-numeric values is now left alone, and the handler only applies when the other side is a number, which is the case this feature is about: PHP would otherwise compare that number as a string, so "rating > 1" still resolves through the handler. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018EWvvUoaHC1SaNQ23nVrCk
Modulo by zero leaked PHP's raw \DivisionByZeroError out of the library instead of NXP\Exception\DivisionByZeroException, and setDivisionByZeroIsZero() did not cover %, so "10 % 0" threw a PHP Error that callers catching MathExecutorException could not catch and that setDivisionByZeroIsZero() could not turn off. useBCMath()'s bcmod() had the same behaviour. % now mirrors / in all three places: it throws the library's exception, setDivisionByZeroIsZero() registers it alongside /, and the BCMath variant checks the divisor before calling bcmod(). This is the one behaviour change in this branch for callers that set no non-numeric handler: "10 % 0" throws DivisionByZeroException where it previously threw \DivisionByZeroError. Operands that are non-numeric without a handler are unaffected, because 0 == 'N/A' is false on PHP 8, so they still reach the operator and raise the same \TypeError as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018EWvvUoaHC1SaNQ23nVrCk
Collaborator
|
Javier,
Thanks for submitting this. Unfortunately I am out of town for a while.
Alex may step in, but I should be able to get to this before the end of the
month.
Bruce
…On Mon, Sep 7, 2026, 4:59 AM Javier Marín ***@***.***> wrote:
The bug
Arithmetic operators hand their operands straight to PHP, so a value that
is not a number leaks a raw \TypeError out of the library instead of one
of its own NXP\Exception\* types:
TypeError: Unsupported operand types: string / string
at NXP\MathExecutor::{closure:NXP\MathExecutor::defaultOperators():371}()
at NXP\Classes\Operator->execute()
at NXP\Classes\Calculator->calculate()
useBCMath() leaks a ValueError from bcdiv() the same way. A caller cannot
catch library errors reliably, because the library's own API throws types
that are not part of it. tests/MathTest.php::testUnsupportedOperands
already pins this behaviour today.
This is not an exotic case. Real data has non-numeric values in otherwise
numeric columns: a rating stored as 'N/A' when nothing was rated, a ''
for a missing measurement, a category code where a number is expected.
is_numeric('') is false, so the empty string hits it too.
And there is currently no supported way to say what a non-numeric operand
*should* mean. The only workaround is to re-register every arithmetic
operator with addOperator(), duplicating the library's own definitions —
including the DivisionByZeroException behaviour and the BCMath variants.
The fix
A handler, in the spirit of the existing setVarNotFoundHandler() /
setVarValidationHandler() / setDivisionByZeroIsZero() opt-ins — not a
boolean, so the caller decides what a non-numeric value means (zero, a
domain-specific mapping, a logged warning, or an exception of their own):
public function setNonNumericHandler(?callable $handler) : self
$executor->setNonNumericHandler(fn($value, string $operator) => 0);$executor->setVar('rating', 'N/A');echo $executor->execute('rating / 2'); // 0
The handler receives the value and the operator name ('+', '/', 'uNeg',
...), so it can react differently per operator, and its return value is
used in place of the operand. Throwing from it converts the \TypeError
into an error of the caller's own type:
$executor->setNonNumericHandler(function ($value, string $operator) {
throw new MathExecutorException("Value ({$value}) is not a number, required by operator ({$operator})");
});
Scope, deliberately narrow:
- *Applied to* the operators that require a number — +, -, *, /, %, ^,
uNeg, uPos, >, >=, <, <= — in defaultOperators(), and to the same
operators as re-registered by setDivisionByZeroIsZero() and useBCMath()
.
- *Not applied to* ==, !=, &&, ||, !. Those already have meaningful
string/boolean semantics (== and != do a strcmp when either side is a
string) and changing them would break existing users.
- *Never called* for values that are numeric ('3' included), null,
boolean or array. Arrays are a supported variable type —
defaultVarValidation() accepts them and avg()/min()/max() consume them
— and PHP gives + a defined meaning for them, so [1, 2] + [3, 4] keeps
returning the array union.
- *Not applied when an ordering operator compares two non-numeric
values.* That is a string comparison, which is meaningful and
consistent with ==/!=, so 'apple' < 'banana' is still true with a
handler installed. The handler applies to >/>=/</<= only when the
other side is a number — the case this feature is about, where PHP would
otherwise compare that number as a string and give 'N/A' > 1 === true.
Backwards compatibility
*No behaviour changes when no handler is set*, which is the default.
normalizeOperand() and normalizeComparisonOperands() both return their
operands untouched as their first check when $onNonNumeric is null, so
every operator receives exactly what it received before.
This was verified differentially rather than asserted: 218 expression/mode
combinations (arithmetic, ordering, equality, logical operators, functions,
string and array and null and boolean variables, under plain /
setDivisionByZeroIsZero() / useBCMath()) were evaluated against master
and against this branch with no handler set. The output is identical except
for the three 10 % 0 cases below.
Implementation notes that follow from the existing design:
- Operator infers arity by reflection on the closure, so the
replacement closures keep exactly the same parameter count and stay untyped
(Calculator pushes raw values through call_user_func_array).
- The affected default closures stop being static so they can read
$this->onNonNumeric *lazily* at call time. That means the handler can
be set at any point — before or after useBCMath() /
setDivisionByZeroIsZero() — and it survives __clone() (which re-runs
addDefaults()).
- normalizeOperand() is protected, not private, so a subclass that
overrides defaultOperators() — the extension point the README
documents — can still use it.
One deliberate behaviour change: modulo by zero
% leaked PHP's raw \DivisionByZeroError out of the library instead of
NXP\Exception\DivisionByZeroException, setDivisionByZeroIsZero() did not
cover it, and useBCMath()'s bcmod() did the same. This is a pre-existing
bug on master — 10 % 0 throws the PHP Error there today — but it is the
*same* bug this PR is about, and it became visible through the handler:
with the README's own => 0 handler, 2 / rating resolved through
DivisionByZeroException while 2 % rating threw a PHP Error that catch
(MathExecutorException) cannot catch.
% now mirrors / in all three places. The change is confined to a zero
divisor:
master this branch
10 % 0 \DivisionByZeroError NXP\Exception\DivisionByZeroException
10 % 0 after setDivisionByZeroIsZero() \DivisionByZeroError 0
10 % 0 under useBCMath() \DivisionByZeroError
NXP\Exception\DivisionByZeroException
Non-numeric operands with no handler are unaffected, because 0 == 'N/A'
is false on PHP 8 — they still reach the operator and raise the same
\TypeError as before.
This is the last commit on the branch and is self-contained, so it can be
dropped if you would rather keep it to a separate PR.
Tests
tests/MathTest.php gains 18 test methods / 26 cases:
- testNonNumericWithoutHandler (data provider, 8 arithmetic
expressions) and testNonNumericComparisonWithoutHandler — assert the
*current* behaviour is untouched with no handler: \TypeError for the
arithmetic operators, string comparison for the ordering ones, 'N/A'
returned by unary +.
- testNonNumericHandler — every affected operator, including '' + 1
=== 1.
- testNonNumericHandlerReceivesTheOperator — asserts the exact
operator names ['+', '-', '*', '/', '%', '^', 'uNeg', 'uPos', '>',
'>=', '<', '<='].
- testNonNumericHandlerCanReturnAnyValue,
testNonNumericHandlerException (the exception propagates),
testNonNumericHandlerCanBeRemoved, testNonNumericHandlerSurvivesClone.
- testNonNumericHandlerIgnoresNumbers — '3' * 2 === 6, 3 + '2.5' ===
5.5, null + 1 === 1, true + 1 === 2, and the handler is never invoked.
- testNonNumericHandlerDoesNotAffectStringOperators — ==, !=, &&, ||, !
unchanged.
- testNonNumericHandlerDoesNotAffectStringOrdering — all four ordering
operators on two strings, with the handler installed, and the handler is
never invoked.
- testNonNumericHandlerDoesNotAffectArrays — the array union survives
and the handler is never invoked.
- testNonNumericHandlerWithDivisionByZeroIsZero,
testNonNumericHandlerWithBCMath,
testNonNumericHandlerWithBCMathDivisionByNonNumeric,
testNonNumericHandlerModuloByNonNumeric,
testNonNumericHandlerModuloByNonNumericIsZero.
- For the modulo fix: testZeroModuloException,
testZeroModuloExceptionWithBCMath, and testZeroDivision extended with 10
% 0.
Suite goes from 455 tests / 567 assertions to 481 tests / 637 assertions,
all green. PHPStan level 6 and php-cs-fixer are clean with no new ignores
or baseline entries.
README gains a "Non-Numeric Value Support" section next to the Division By
Zero one, a bullet in the feature list, and a note that
setDivisionByZeroIsZero() covers % as well.
------------------------------
You can view, comment on, or merge this pull request online at:
#146
Commit Summary
- 78a6060
<78a6060>
Add a non-numeric value handler for arithmetic operators
- eebb83d
<eebb83d>
Keep arrays and string ordering out of the non-numeric handler
- 27c1f42
<27c1f42>
Throw DivisionByZeroException on modulo by zero
File Changes
(3 files <https://github.com/neonxp/MathExecutor/pull/146/files>)
- *M* README.md
<https://github.com/neonxp/MathExecutor/pull/146/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5>
(47)
- *M* src/NXP/MathExecutor.php
<https://github.com/neonxp/MathExecutor/pull/146/files#diff-7fdf8868ac1a9f58238d96b56ad0b0f01e0fcf52ca588a865e2b5279c62a00ca>
(197)
- *M* tests/MathTest.php
<https://github.com/neonxp/MathExecutor/pull/146/files#diff-36553c418ec67ba28afb46327ae83553b5418d38a9594125b16ef8af1b143ed0>
(277)
Patch Links:
- https://github.com/neonxp/MathExecutor/pull/146.patch
- https://github.com/neonxp/MathExecutor/pull/146.diff
—
Reply to this email directly, view it on GitHub
<#146?email_source=notifications&email_token=ABYW6S6JCNK2JB6G62ZE2MT5NZ2I3A5CNFSNUABEM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UF42DINRSGIYDANZRGOTHEZLBONXW5KTTOVRHGY3SNFRGKZFFMV3GK3TUVRTG633UMVZF6Y3MNFRWW>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABYW6S3MZCV2KHOQ3QOIDK35NZ2I3AVCNFSNUABDKJSXA33TNF2G64TZHM4DONRUGYZTSO2JONZXKZJ3GUZTOMRUGEYTOMJYUF3AE>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Contributor
Author
|
Thanks for the heads-up, Bruce! Safe travels and zero rush at all, we're running on our fork in the meantime, so take all the time you need. Looking forward to catching up whenever you're back. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Arithmetic operators hand their operands straight to PHP, so a value that is not a number leaks a raw
\TypeErrorout of the library instead of one of its ownNXP\Exception\*types:useBCMath()leaks aValueErrorfrombcdiv()the same way. A caller cannot catch library errors reliably, because the library's own API throws types that are not part of it.tests/MathTest.php::testUnsupportedOperandsalready pins this behaviour today.This is not an exotic case. Real data has non-numeric values in otherwise numeric columns: a rating stored as
'N/A'when nothing was rated, a''for a missing measurement, a category code where a number is expected.is_numeric('')isfalse, so the empty string hits it too.And there is currently no supported way to say what a non-numeric operand should mean. The only workaround is to re-register every arithmetic operator with
addOperator(), duplicating the library's own definitions — including theDivisionByZeroExceptionbehaviour and the BCMath variants.The fix
A handler, in the spirit of the existing
setVarNotFoundHandler()/setVarValidationHandler()/setDivisionByZeroIsZero()opt-ins — not a boolean, so the caller decides what a non-numeric value means (zero, a domain-specific mapping, a logged warning, or an exception of their own):The handler receives the value and the operator name (
'+','/','uNeg', ...), so it can react differently per operator, and its return value is used in place of the operand. Throwing from it converts the\TypeErrorinto an error of the caller's own type:Scope, deliberately narrow:
+,-,*,/,%,^,uNeg,uPos,>,>=,<,<=— indefaultOperators(), and to the same operators as re-registered bysetDivisionByZeroIsZero()anduseBCMath().==,!=,&&,||,!. Those already have meaningful string/boolean semantics (==and!=do astrcmpwhen either side is a string) and changing them would break existing users.'3'included),null, boolean or array. Arrays are a supported variable type —defaultVarValidation()accepts them andavg()/min()/max()consume them — and PHP gives+a defined meaning for them, so[1, 2] + [3, 4]keeps returning the array union.==/!=, so'apple' < 'banana'is stilltruewith a handler installed. The handler applies to>/>=/</<=only when the other side is a number — the case this feature is about, where PHP would otherwise compare that number as a string and give'N/A' > 1 === true.Backwards compatibility
No behaviour changes when no handler is set, which is the default.
normalizeOperand()andnormalizeComparisonOperands()both return their operands untouched as their first check when$onNonNumericis null, so every operator receives exactly what it received before.This was verified differentially rather than asserted: 218 expression/mode combinations (arithmetic, ordering, equality, logical operators, functions, string and array and null and boolean variables, under plain /
setDivisionByZeroIsZero()/useBCMath()) were evaluated againstmasterand against this branch with no handler set. The output is identical except for the three10 % 0cases below.Implementation notes that follow from the existing design:
Operatorinfers arity by reflection on the closure, so the replacement closures keep exactly the same parameter count and stay untyped (Calculatorpushes raw values throughcall_user_func_array).staticso they can read$this->onNonNumericlazily at call time. That means the handler can be set at any point — before or afteruseBCMath()/setDivisionByZeroIsZero()— and it survives__clone()(which re-runsaddDefaults()).normalizeOperand()isprotected, not private, so a subclass that overridesdefaultOperators()— the extension point the README documents — can still use it.One deliberate behaviour change: modulo by zero
%leaked PHP's raw\DivisionByZeroErrorout of the library instead ofNXP\Exception\DivisionByZeroException,setDivisionByZeroIsZero()did not cover it, anduseBCMath()'sbcmod()did the same. This is a pre-existing bug onmaster—10 % 0throws the PHPErrorthere today — but it is the same bug this PR is about, and it became visible through the handler: with the README's own=> 0handler,2 / ratingresolved throughDivisionByZeroExceptionwhile2 % ratingthrew a PHPErrorthatcatch (MathExecutorException)cannot catch.%now mirrors/in all three places. The change is confined to a zero divisor:master10 % 0\DivisionByZeroErrorNXP\Exception\DivisionByZeroException10 % 0aftersetDivisionByZeroIsZero()\DivisionByZeroError010 % 0underuseBCMath()\DivisionByZeroErrorNXP\Exception\DivisionByZeroExceptionNon-numeric operands with no handler are unaffected, because
0 == 'N/A'isfalseon PHP 8 — they still reach the operator and raise the same\TypeErroras before.This is the last commit on the branch and is self-contained, so it can be dropped if you would rather keep it to a separate PR.
Tests
tests/MathTest.phpgains 18 test methods / 26 cases:testNonNumericWithoutHandler(data provider, 8 arithmetic expressions) andtestNonNumericComparisonWithoutHandler— assert the current behaviour is untouched with no handler:\TypeErrorfor the arithmetic operators, string comparison for the ordering ones,'N/A'returned by unary+.testNonNumericHandler— every affected operator, including'' + 1 === 1.testNonNumericHandlerReceivesTheOperator— asserts the exact operator names['+', '-', '*', '/', '%', '^', 'uNeg', 'uPos', '>', '>=', '<', '<='].testNonNumericHandlerCanReturnAnyValue,testNonNumericHandlerException(the exception propagates),testNonNumericHandlerCanBeRemoved,testNonNumericHandlerSurvivesClone.testNonNumericHandlerIgnoresNumbers—'3' * 2 === 6,3 + '2.5' === 5.5,null + 1 === 1,true + 1 === 2, and the handler is never invoked.testNonNumericHandlerDoesNotAffectStringOperators—==,!=,&&,||,!unchanged.testNonNumericHandlerDoesNotAffectStringOrdering— all four ordering operators on two strings, with the handler installed, and the handler is never invoked.testNonNumericHandlerDoesNotAffectArrays— the array union survives and the handler is never invoked.testNonNumericHandlerWithDivisionByZeroIsZero,testNonNumericHandlerWithBCMath,testNonNumericHandlerWithBCMathDivisionByNonNumeric,testNonNumericHandlerModuloByNonNumeric,testNonNumericHandlerModuloByNonNumericIsZero.testZeroModuloException,testZeroModuloExceptionWithBCMath, andtestZeroDivisionextended with10 % 0.Suite goes from 455 tests / 567 assertions to 481 tests / 637 assertions, all green. PHPStan level 6 and php-cs-fixer are clean with no new ignores or baseline entries.
README gains a "Non-Numeric Value Support" section next to the Division By Zero one, a bullet in the feature list, and a note that
setDivisionByZeroIsZero()covers%as well.