feat: comprehensive Docker PHPT test infrastructure and BCMath improvements by nanasess · Pull Request #38 · nanasess/bcmath-polyfill

nanasess · 2025-09-11T07:01:17Z

Summary

This PR introduces a comprehensive Docker-based PHPT test infrastructure and implements several critical bug fixes and enhancements to the BCMath polyfill.

🐳 Docker PHPT Test Infrastructure

Add Docker-based test runner for PHP's official bcmath PHPT tests
Implement automated test environment setup and configuration
Integrate PHPT testing into CI/CD pipeline with GitHub Actions
Support multiple PHP versions (8.1, 8.2, 8.3, 8.4) in testing matrix

🔧 BCMath Bug Fixes and Enhancements

Critical Fixes:

HalfTowardsZero Rounding: Fix PHP_ROUND_HALF_DOWN implementation for negative numbers with exactly 0.5
Modulo by Zero: Correct error message for bcmod() to match PHP's expected "Modulo by zero"
Negative Power Handling: Improve bcpow() behavior for negative bases and zero powers
Sign Handling: Fix various edge cases with negative zero and sign preservation

Enhancements:

Complete RoundingMode Support: Add PositiveInfinity to enum polyfill for full PHP 8.4 compatibility
Improved Error Handling: Better error messages and argument validation
Enhanced Precision: Better decimal handling and scale management

📋 Test Coverage Improvements

Add comprehensive test cases for HalfTowardsZero rounding mode
Expand edge case coverage for various BCMath functions
Integration with PHP's official test suite via PHPT framework

🚀 CI/CD Improvements

Streamlined GitHub Actions workflow
Automated PHPT test execution with appropriate skip lists
Multi-version PHP testing support

Test Plan

All existing unit tests pass
New PHPT tests pass where expected (unsupported modes correctly throw exceptions)
HalfTowardsZero rounding works correctly for all cases
No regressions in existing functionality
CI pipeline executes successfully across all PHP versions

🤖 Generated with Claude Code

Ensure that inputs like '-0' or '-0.0' are correctly normalized to '0'. This fixes inconsistencies in handling special cases for zero values.

…rison The decimal parts of numbers are now padded to the specified scale length with zeros to ensure consistent comparison.

…nding` Consolidated duplicate logic for handling rounding operations in `floor` and `ceil` methods into a new private method `normalizeZeroForRounding`. This improves code maintainability and reduces redundancy.

…wers of zero Introduce support for recognizing `-0` and `-0.0` as valid exponents in `bcpow`. Added behavior to throw a `DivisionByZeroError` for negative powers of zero, aligning with PHP 8.4+ standards. Improved handling of negative base calculations by applying a negative sign only for odd integer exponents.

…puts into powmod Remove the validateIntegerInputs method that was incorrectly stripping negative signs from valid negative numbers, causing bcpowmod to fail with negative bases. The method was only used in powmod and created unnecessary abstraction with a complex constraint system. By inlining the logic directly: - Fix sign removal bug that affected negative base calculations - Preserve security measures (ltrim for whitespace handling) - Add proper negative base handling for bcmath-compatible modulus operations - Simplify code by removing 54 lines of unnecessary abstraction - Improve maintainability with direct, readable validation logic This resolves bcpowmod test failures where negative bases were incorrectly processed as positive values (e.g., bcpowmod('-2', '5', '7') now correctly returns '-4' instead of '4'). 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

test(BCMathTest): update data provider and test method names for clarity

The scale_ini test is skipped because the bcmath.scale INI setting is ignored without the native extension. docs: clarify bcmath.scale INI setting limitations in README Updated the README to explain that the bcmath.scale INI setting is ignored when the native bcmath extension is not loaded. Added a workaround to use `bcscale()` instead. fix: handle bcmath.scale INI setting fallback in polyfill Updated `resolveScale` method to handle cases where `ini_get('bcmath.scale')` returns false due to the absence of the native bcmath extension. Default scale is set to 0 in such cases.

Add a dedicated error message for modulo by zero and implement a `checkModuloByZero` method to validate divisor inputs. Replace division by zero check with modulo by zero check in `bcmod`.

- Fix PHP_ROUND_HALF_DOWN implementation to properly handle "towards zero" rounding - For exactly 0.5: positive numbers round down, negative numbers round up (towards zero) - For > 0.5: always round away from zero regardless of sign - Add comprehensive test cases for HalfTowardsZero rounding mode - Update CI to enable bcround_half_down test (now passes) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

Added PositiveInfinity to the RoundingMode enum polyfill for PHP 8.1-8.3. Updated BCMath to throw a ValueError for unsupported PositiveInfinity mode.

claude · 2025-09-11T07:02:58Z

Pull Request Review

This is a comprehensive PR that introduces significant improvements to the BCMath polyfill. I've reviewed the code quality, security, performance, and test coverage. Here's my detailed feedback:

✅ Strengths

Excellent Docker PHPT Integration: The Docker-based test infrastructure is well-implemented and provides robust validation against PHP's official test suite.
Critical Bug Fixes: The fixes for HalfTowardsZero rounding, modulo by zero error messages, and negative power handling address important compatibility issues.
Improved Error Handling: Better alignment with PHP's native bcmath error messages and behavior.
Code Documentation: Good inline documentation explaining limitations with bcmath.scale INI settings.

🔍 Code Quality & Best Practices

Good:

Consistent with existing code style and PSR-2 standards
Proper use of constants for error messages (MODULO_BY_ZERO_MESSAGE)
Well-structured 5-phase processing pattern maintained
Appropriate use of early returns for performance

Areas for Improvement:

The normalizeZeroForRounding() method could benefit from more detailed PHPDoc explaining its callback pattern
Some complex conditional logic in bcpow() could be extracted into helper methods for better readability

⚡ Performance Considerations

Good:

Efficient early returns for special cases (zero powers, etc.)
Appropriate use of BigInteger operations

Potential Concerns:

The new negative base handling in bcpowmod() uses a less efficient approach (calculating full power then modulus) instead of the optimized powMod() method. This could impact performance for large numbers.
String manipulation operations in rounding functions could be optimized with fewer substring operations.

🔒 Security

No security concerns identified. The changes maintain proper input validation and don't introduce any new attack vectors.

🧪 Test Coverage

Excellent:

Comprehensive test cases for HalfTowardsZero rounding mode
Good edge case coverage for negative powers of zero
PHPT integration provides extensive validation against official test suite
Proper use of PHPUnit data providers

Suggestions:

Consider adding property-based tests for rounding operations to catch edge cases
Add more test cases for the bcpowmod negative base behavior to ensure correctness

🚨 Potential Issues

Performance Regression in bcpowmod: Lines 758-773 use an inefficient approach for negative bases that could be slow for large numbers. Consider if there's a way to maintain bcmath compatibility while preserving the performance benefits of powMod().
Complex Conditional Logic: The bcpow() method has become quite complex with multiple nested conditions. Consider refactoring for better maintainability.

📋 Recommendations

Add performance benchmarks for bcpowmod with negative bases to quantify the impact
Consider extracting helper methods from complex functions like bcpow() for better readability
Add inline examples in PHPDoc for the new rounding behaviors
Consider adding a CHANGELOG entry to document the behavior changes

🎯 Overall Assessment

This is a high-quality PR that significantly improves the polyfill's compatibility with PHP's native bcmath. The Docker PHPT integration is particularly valuable. While there are some performance and maintainability considerations, the benefits far outweigh the concerns.

Recommendation: Approve with minor suggestions addressed.

Great work on this comprehensive improvement! 🚀

🤖 Generated with Claude Code

claude · 2025-09-11T07:44:35Z

🔍 Code Review - BCMath Polyfill PR #38

Thank you for this comprehensive improvement to the BCMath polyfill! This is an excellent contribution with significant enhancements to functionality, testing infrastructure, and compatibility. Below is my detailed analysis:

✅ Strengths

1. Docker PHPT Test Infrastructure

Outstanding addition: The Docker-based PHPT test integration is a major improvement that brings this polyfill closer to real-world bcmath compatibility
Multi-version testing: Supporting PHP 8.1-8.4 in CI demonstrates commitment to broad compatibility
Proper skip list management: The selective test skipping with clear documentation is well-implemented

2. Critical Bug Fixes

HalfTowardsZero rounding fix (src/BCMath.php:1125-1126): The negative number handling for exactly 0.5 cases is now correct
Modulo error message (src/BCMath.php:50): Proper "Modulo by zero" vs "Division by zero" distinction improves PHP compatibility
Negative power of zero (src/BCMath.php:657-659): Adding DivisionByZeroError for 0^(-n) matches PHP 8.4+ behavior
Enhanced decimal padding (src/BCMath.php:345-346): Fixes comparison precision issues

3. Code Quality Improvements

Modular error handling: Separate checkModuloByZero() method improves code organization
Comprehensive test coverage: New test cases for edge cases significantly improve reliability
Documentation updates: Clear explanation of bcmath.scale INI limitations

🔍 Areas for Consideration

1. Performance Considerations

In src/BCMath.php:782-804 (bcpowmod negative base handling), computing the full power before modulus could cause performance issues for large exponents. Consider implementing a more efficient algorithm or documenting the limitation.

2. Sign Handling Complexity

The sign handling logic in bcpow() is quite complex and could benefit from additional test cases covering edge cases like (-2.0)^3 vs (-2.1)^3.

3. Error Message Consistency

The codebase now has checkDivisionByZero() and checkModuloByZero() methods. Consider consolidating these or creating a more generic checkZeroDivisor(string $operation) method.

🛡️ Security & Reliability

Positive Aspects:

✅ Proper input validation throughout
✅ No credential exposure risks
✅ Appropriate error handling for malformed inputs
✅ Memory-safe BigInteger operations

Edge Case Coverage:

The new test cases significantly improve reliability:

✅ HalfTowardsZero rounding edge cases
✅ Negative zero normalization
✅ Zero power handling
✅ Modulus error conditions

📋 Recommendations

High Priority:

Add performance tests for bcpowmod with large negative exponents to establish safe usage limits
Validate the complex exponent parsing logic in bcpow() with additional test cases

Medium Priority:

Consider refactoring the dual validation methods for zero divisors
Add inline documentation for the complex sign handling logic in bcpow()

Low Priority:

Code formatting: Some long lines could be broken for better readability

🎯 Overall Assessment

This is an excellent contribution that significantly improves the polyfill's compatibility and reliability. The Docker PHPT integration alone is a major achievement. The bug fixes address real compatibility issues that would affect production usage.

Recommendation: ✅ APPROVE - This PR substantially improves the codebase with well-tested fixes and infrastructure improvements.

The areas I've highlighted are suggestions for future improvements rather than blockers for this PR. The core functionality is sound and the testing infrastructure is robust.

Great work! 🚀

Copilot

Pull Request Overview

This PR introduces a comprehensive Docker-based PHPT test infrastructure for validating BCMath compatibility with PHP's official test suite and implements several critical bug fixes and enhancements to the BCMath polyfill.

Add Docker-based test runner for PHP's official bcmath PHPT tests with CI/CD integration
Fix critical BCMath bugs including HalfTowardsZero rounding for negative numbers and modulo by zero error messages
Enhance BCMath functionality with better error handling, improved precision, and complete RoundingMode enum support

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tests/BCMathTest.php	Adds comprehensive test coverage for HalfTowardsZero rounding mode and improves bcpow zero base handling
src/BCMath.php	Implements critical bug fixes for modulo error messages, negative power handling, and HalfTowardsZero rounding
lib/RoundingMode.php	Adds PositiveInfinity case to RoundingMode enum for complete PHP 8.4 compatibility
README.md	Documents bcmath.scale INI setting limitations when native extension is not loaded
.github/workflows/ci.yml	Integrates Docker-based PHPT testing into CI pipeline with appropriate skip lists

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-11T07:45:08Z

src/BCMath.php

        // Convert to BigInteger for calculation
        $baseBig = new BigInteger($baseParts[0].$baseParts[1]);

-        $sign = self::isNegative($baseBig) ? '-' : '';
+        $baseIsNegative = self::isNegative($baseBig);


The variable $baseIsNegative should be determined before converting $baseBig to absolute value to ensure consistent state. Currently, the negative check happens after creating the BigInteger but before taking the absolute value, which works but is less clear.

Copilot · 2025-09-11T07:45:08Z

src/BCMath.php

+                    // Exactly 0.5 and negative: round towards zero (which means don't add anything - truncate)
+                    // Do nothing - let truncation handle it
                }
+                // For exactly 0.5 and positive: round towards zero (which is down, so don't add)


This empty conditional block with only comments should be removed or restructured. Either combine the conditions or make the intent clearer by restructuring the logic to avoid an empty block.

Suggested change

// Exactly 0.5 and negative: round towards zero (which means don't add anything - truncate)

// Do nothing - let truncation handle it

}

// For exactly 0.5 and positive: round towards zero (which is down, so don't add)

// Exactly 0.5 and negative: round towards zero (which means don't add anything - let truncation handle it)

}

// For exactly 0.5 and positive: round towards zero (which is down, so don't add; let truncation handle it)

Copilot · 2025-09-11T07:45:08Z

src/BCMath.php

+        if (self::startsWithNegativeSign($baseInt)) {
+            $baseInt = ltrim($baseInt);
+        }


The ltrim($baseInt) call will remove all leading whitespace and zeros, not just the negative sign. This could incorrectly modify the base value. Use substr($baseInt, 1) instead to remove only the negative sign.

Copilot · 2025-09-11T07:45:09Z

src/BCMath.php

+        $exponentParts = explode('.', $exponent, 2);
+        $exponentInt = $exponentParts[0] === '' ? '0' : $exponentParts[0];
+        if (self::startsWithNegativeSign($exponentInt)) {
+            $exponentInt = ltrim($exponentInt);


Same issue as with $baseInt - ltrim() removes all leading whitespace and zeros, not just the negative sign. Use substr($exponentInt, 1) to remove only the negative sign.

Suggested change

$exponentInt = ltrim($exponentInt);

$exponentInt = substr($exponentInt, 1);

Copilot · 2025-09-11T07:45:09Z

src/BCMath.php

+        if (self::startsWithNegativeSign($modulusInt)) {
+            $modulusInt = ltrim($modulusInt);
+        }


Same issue as above - ltrim() removes all leading whitespace and zeros, not just the negative sign. Use substr($modulusInt, 1) to remove only the negative sign.

nanasess and others added 10 commits September 10, 2025 17:02

fix(BCMath): handle edge cases for '-0' in integer part calculation

6e92665

Ensure that inputs like '-0' or '-0.0' are correctly normalized to '0'. This fixes inconsistencies in handling special cases for zero values.

feat(BCMath): add padding to decimal parts for consistent scale compa…

3132b57

…rison The decimal parts of numbers are now padded to the specified scale length with zeros to ensure consistent comparison.

refactor(BCMath): centralize rounding logic with `normalizeZeroForRou…

6c269d7

…nding` Consolidated duplicate logic for handling rounding operations in `floor` and `ceil` methods into a new private method `normalizeZeroForRounding`. This improves code maintainability and reduces redundancy.

refactor(BCMath): improve exponent handling logic and error messaging

d5cdd1e

test(BCMathTest): update data provider and test method names for clarity

fix(BCMath): handle modulo by zero error

a52fadf

Add a dedicated error message for modulo by zero and implement a `checkModuloByZero` method to validate divisor inputs. Replace division by zero check with modulo by zero check in `bcmod`.

feat(RoundingMode): add PositiveInfinity support to enum polyfill

2f3ec0e

Added PositiveInfinity to the RoundingMode enum polyfill for PHP 8.1-8.3. Updated BCMath to throw a ValueError for unsupported PositiveInfinity mode.

ci(workflows): update skipped tests in Docker-based PHPT configuration

5aa753c

nanasess force-pushed the phpt-tests branch from 73b216f to 5aa753c Compare September 11, 2025 07:42

nanasess requested a review from Copilot September 11, 2025 07:43

Copilot AI reviewed Sep 11, 2025

View reviewed changes

nanasess merged commit a9a5884 into main Sep 11, 2025
47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: comprehensive Docker PHPT test infrastructure and BCMath improvements#38

feat: comprehensive Docker PHPT test infrastructure and BCMath improvements#38
nanasess merged 11 commits intomainfrom
phpt-tests

nanasess commented Sep 11, 2025

Uh oh!

claude bot commented Sep 11, 2025

Uh oh!

claude bot commented Sep 11, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 11, 2025

Uh oh!

Copilot AI Sep 11, 2025

Uh oh!

Copilot AI Sep 11, 2025

Uh oh!

Copilot AI Sep 11, 2025

Uh oh!

Copilot AI Sep 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Comments

	$exponentInt = ltrim($exponentInt);
	$exponentInt = substr($exponentInt, 1);

Conversation

nanasess commented Sep 11, 2025

Summary

🐳 Docker PHPT Test Infrastructure

🔧 BCMath Bug Fixes and Enhancements

Critical Fixes:

Enhancements:

📋 Test Coverage Improvements

🚀 CI/CD Improvements

Test Plan

Uh oh!

claude bot commented Sep 11, 2025

Pull Request Review

✅ Strengths

🔍 Code Quality & Best Practices

⚡ Performance Considerations

🔒 Security

🧪 Test Coverage

🚨 Potential Issues

📋 Recommendations

🎯 Overall Assessment

Uh oh!

claude bot commented Sep 11, 2025

🔍 Code Review - BCMath Polyfill PR #38

✅ Strengths

1. Docker PHPT Test Infrastructure

2. Critical Bug Fixes

3. Code Quality Improvements

🔍 Areas for Consideration

1. Performance Considerations

2. Sign Handling Complexity

3. Error Message Consistency

🛡️ Security & Reliability

Positive Aspects:

Edge Case Coverage:

📋 Recommendations

High Priority:

Medium Priority:

Low Priority:

🎯 Overall Assessment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Comments