Preserve sign and honor currency exponents in money formatting#232
Open
roncodes wants to merge 1 commit into
Open
Preserve sign and honor currency exponents in money formatting#232roncodes wants to merge 1 commit into
roncodes wants to merge 1 commit into
Conversation
Utils::numbersOnly() stripped the leading minus sign via
preg_replace('/[^0-9]/', ...), so negative monetary amounts lost their
sign (e.g. moneyFormat(-500, 'USD') rendered as $5.00 instead of
-$5.00). numbersOnly() now preserves a minus that appears before the
first digit, while still ignoring hyphens that occur after digits
(e.g. phone numbers like "276-7156"), and collapses empty/non-numeric
input to 0.
moneyFormat() treats its amount as an integer number of the currency's
smallest (minor) unit; the money adapter derives decimal placement from
each currency's ISO-4217 exponent, so zero-decimal (JPY/KRW) and
three-decimal (BHD/KWD) currencies format correctly rather than assuming
two places. Docblocks updated to document the contract.
Tests now exercise the real Cknow\Money\Money adapter (added
cknow/laravel-money to require-dev) instead of a stub, covering
negative/zero/JPY/KRW/BHD/KWD/non-numeric/null cases, plus dedicated
numbersOnly sign-preservation coverage.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Problem
Utils::numbersOnly()normalized input withpreg_replace('/[^0-9]/', '', ...), which stripped the leading minus sign. BecauseUtils::moneyFormat()(and theMoneycast) route their input through it, negative monetary amounts silently lost their sign — e.g.moneyFormat(-500, 'USD')rendered as$5.00instead of-$5.00. This is a real bug for refunds and adjustments.While investigating, the money-format path was also checked against currencies whose minor-unit exponent isn't 2 — zero-decimal (JPY/KRW) and three-decimal (BHD/KWD).
Changes
numbersOnly()now preserves a minus sign that appears before the first digit, so signed values keep their sign. A hyphen occurring after digits (e.g. in a phone number like276-7156) is deliberately not treated as a sign, keeping existing non-monetary usage intact. Empty / non-numeric /nullstill collapse to0.moneyFormat()documents and relies on the minor-units contract: the amount is an integer number of the currency's smallest unit, and the money adapter derives decimal placement from each currency's ISO-4217 exponent. No 2-decimal assumption — JPY/KRW format with no decimals, BHD/KWD with three (500fils →0.500).Moneycast follows automatically: it will now store negative amounts instead of flipping them positive.Tests
cknow/laravel-moneytorequire-devso the money tests run against the realCknow\Money\Moneyadapter instead of a stub.UtilsMoneyFormatTestto cover positive / zero / negative USD, formatted-string input, JPY/KRW (0-decimal), BHD/KWD (3-decimal), and non-numeric /null. Assertions target the numeric core of the output so they stay stable across ICU symbol/spacing differences.numbersOnlysign-preservation coverage inUtilsTest.ReportQueryExporterTest, which had a cross-file dependency on the old stub, to assert the real adapter output.Verification
🤖 Generated with Claude Code