feat: add support for arrays of REAL and DOUBLE PRECISION
#307
Conversation
WalkthroughThe pull request restructures the PostgreSQL types documentation by expanding the table to include practical usage and internal catalog names, with existing types updated to their array counterparts and new types added. It introduces a new abstract class for handling float array types, along with two concrete implementations defining constraints for DOUBLE PRECISION[] and REAL[] types. Additionally, custom exception classes and extensive tests have been added to validate and ensure proper transformation behavior, with minor renamings in existing tests to reflect domain concerns. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FloatArray as BaseFloatArray
participant TypeHandler as {DoublePrecisionArray / RealArray}
participant ExceptionHandler
Client->>FloatArray: transformArrayItemForPHP(input)
FloatArray->>TypeHandler: Validate (min, max, precision)
alt Valid Value
TypeHandler-->>FloatArray: Validated value
FloatArray-->>Client: Return converted float
else Invalid Value
TypeHandler-->>ExceptionHandler: Identify error type
ExceptionHandler-->>Client: Throw exception with message
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatValueException.php (1)
16-29: Consider adding more descriptive doc-blocks.The usage of
var_export()for detailed output is excellent, and the static constructor approach is consistent. For completeness, consider adding doc-blocks for each method to clarify the intended usage scenarios and parameter constraints.src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php (1)
71-114: Reduce duplicated validation logic.Logic in
transformArrayItemForPHP()partially overlaps withisValidArrayItemForDatabase()(e.g., checking for numeric content, float regex, min/max values). Consider refactoring into a shared helper method to ensure consistency and reduce code duplication.docs/AVAILABLE-TYPES.md (1)
3-13: Expand on precision/edge-case details in documentation.The updated table is clear and comprehensive. To further assist users, consider adding notes about valid float ranges, handling of near-zero values, and behavior for Infinity/NaN if relevant.
tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.php (2)
1-10: Structure looks good, but missing an informative file docblock.The namespace, imports, and file structure follow best practices. However, consider adding a file-level docblock explaining the purpose of this test case class and its role in testing float array types.
+/** + * Abstract test case for float array type implementations. + * Provides common test functionality for REAL[] and DOUBLE PRECISION[] array types. + */ <?php declare(strict_types=1);
50-58: Test passes though the test for transformation is incomplete.The method only tests
transformArrayItemForPHPfor single values, but doesn't test the full array transformation flow. Since this is testing a PostgreSQL array type, it would be beneficial to also test the conversion of a complete array.public function can_transform_to_php_value(float $phpValue, string $postgresValue): void { self::assertEquals($phpValue, $this->fixture->transformArrayItemForPHP($postgresValue)); + + // Test full array transformation + $postgresArray = '{' . $postgresValue . '}'; + $phpArray = $this->fixture->convertToPHPValue($postgresArray, null); + self::assertIsArray($phpArray); + self::assertCount(1, $phpArray); + self::assertEquals($phpValue, $phpArray[0]); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/AVAILABLE-TYPES.md(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArray.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatValueException.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/RealArray.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArrayTestCase.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/RealArrayTestCase.php(1 hunks)
🧰 Additional context used
🧬 Code Definitions (5)
src/MartinGeorgiev/Doctrine/DBAL/Types/RealArray.php (2)
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php (5)
BaseFloatArray(14-115)getMinValue(18-18)getMaxValue(20-20)getMaxPrecision(22-22)getMinAbsoluteValue(24-24)src/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArray.php (4)
getMinValue(19-22)getMaxValue(24-27)getMaxPrecision(29-32)getMinAbsoluteValue(34-37)
src/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArray.php (2)
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php (5)
BaseFloatArray(14-115)getMinValue(18-18)getMaxValue(20-20)getMaxPrecision(22-22)getMinAbsoluteValue(24-24)src/MartinGeorgiev/Doctrine/DBAL/Types/RealArray.php (4)
getMinValue(19-22)getMaxValue(24-27)getMaxPrecision(29-32)getMinAbsoluteValue(34-37)
tests/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArrayTestCase.php (3)
src/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArray.php (1)
DoublePrecisionArray(15-38)src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatValueException.php (1)
InvalidFloatValueException(14-30)src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php (1)
transformArrayItemForPHP(71-114)
tests/MartinGeorgiev/Doctrine/DBAL/Types/RealArrayTestCase.php (4)
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatValueException.php (1)
InvalidFloatValueException(14-30)src/MartinGeorgiev/Doctrine/DBAL/Types/RealArray.php (1)
RealArray(15-38)tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.php (3)
BaseFloatArrayTestCase(11-78)provideInvalidTransformations(28-38)provideValidTransformations(66-66)src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php (1)
transformArrayItemForPHP(71-114)
tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.php (3)
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php (3)
BaseFloatArray(14-115)isValidArrayItemForDatabase(26-69)transformArrayItemForPHP(71-114)tests/MartinGeorgiev/Doctrine/DBAL/Types/RealArrayTestCase.php (2)
provideInvalidTransformations(25-35)provideValidTransformations(40-64)tests/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArrayTestCase.php (2)
provideInvalidTransformations(26-39)provideValidTransformations(44-53)
🔇 Additional comments (12)
src/MartinGeorgiev/Doctrine/DBAL/Types/RealArray.php (1)
1-38: Implementation looks good!The RealArray class implementation correctly defines the appropriate constraints for PostgreSQL's REAL[] type. The min/max values, precision limit, and minimum absolute value align with PostgreSQL's specifications.
src/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArray.php (1)
1-38: Implementation looks good!The DoublePrecisionArray class correctly implements PostgreSQL's DOUBLE PRECISION[] type with appropriate constraints. The class structure follows the same pattern as RealArray, providing a consistent approach across the codebase.
tests/MartinGeorgiev/Doctrine/DBAL/Types/RealArrayTestCase.php (2)
27-34: Comprehensive test cases for invalid valuesGood job providing thorough test cases for invalid transformations, including values that are too large, too small, with too many decimal places, and those that are too close to zero.
40-64: Good coverage of valid transformationsThe test cases for valid transformations cover important scenarios including edge cases close to min/max values, regular decimal values, and zero.
tests/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArrayTestCase.php (4)
27-38: Extensive invalid transformation test casesExcellent coverage of invalid transformation scenarios including boundary values, precision issues, and various malformed inputs. This will help ensure robust validation.
46-52: Good coverage of valid transformationsThe test cases appropriately cover scientific notation, which is important for the DOUBLE PRECISION type, along with regular decimal values and basic positive/negative scenarios.
58-64:✅ Verification successful
Exception message consistency check
The error message test expects "too close to zero for PostgreSQL DOUBLE PRECISION type" which aligns with the current implementation of
InvalidFloatValueException. However, if the exception class is updated to use dynamic type names (to fix the issue in RealArrayTestCase), this test would need to be updated as well.
🏁 Script executed:
#!/bin/bash # Check implementation of InvalidFloatValueException::forValueThatIsTooCloseToZero grep -A 5 "forValueThatIsTooCloseToZero" src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatValueException.phpLength of output: 432
Exception message consistency confirmed
The test in
tests/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArrayTestCase.phpcorrectly expects the exception message"Given value of '1.18E-308' is too close to zero for PostgreSQL DOUBLE PRECISION type"which aligns with the current implementation in
InvalidFloatValueException::forValueThatIsTooCloseToZero(as verified by the grep output). Note that if you later update the exception class to use a dynamic type name—as addressed inRealArrayTestCase—this test will also need to be updated to match the new message.
14-15:✅ Verification successful
Potential parent::setUp() issue
The method calls
parent::setUp()but it's not clear from the provided snippets ifBaseFloatArrayTestCaseimplements asetUpmethod. If it doesn't, this could potentially cause an error.
🏁 Script executed:
#!/bin/bash # Check if BaseFloatArrayTestCase has a setUp method grep -A 10 "function setUp" tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.phpLength of output: 97
I'll run an additional verification to get more context on the parent class. This next script will search for both any
setUpmethod in the file as well as the class inheritance declaration to confirm whetherBaseFloatArrayTestCasemight indirectly define or inheritsetUp().Please run the above script and share the output so we can definitively verify whether the parent class chain provides a
setUpmethod or if there's a risk here.
🏁 Script executed:
#!/bin/bash echo "Searching for setUp method definition in BaseFloatArrayTestCase.php:" rg -C 5 "function\s+setUp\(" tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.php echo "Checking the class declaration for BaseFloatArrayTestCase to see what it extends:" rg "class\s+BaseFloatArrayTestCase" tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.phpLength of output: 579
Parent setUp Method Usage Confirmed
After verifying the code, we found that
BaseFloatArrayTestCase(located attests/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.php) is declared as:abstract class BaseFloatArrayTestCase extends TestCaseAlthough no
setUp()method is explicitly defined inBaseFloatArrayTestCase, it inherits thesetUp()method from its parent class (TestCase), which is provided by PHPUnit. Therefore, the call toparent::setUp()inDoublePrecisionArrayTestCaseis valid and follows the standard PHPUnit pattern.No corrective action is necessary.
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php (1)
94-97: Verify handling of special float values.Infinity or NaN values may bypass current checks or pose edge cases. If the code should handle or reject them, ensure test coverage and explicit handling.
Would you like me to open a new issue to test Infinity and NaN cases?
tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.php (3)
11-14: LGTM! The class declaration is clear and properly abstract.The class correctly extends TestCase and defines the required protected property for the test fixture.
15-23: The test method correctly validates rejection of invalid values.This test ensures that the
isValidArrayItemForDatabase()method properly rejects invalid values that shouldn't be stored in the database.
60-66: LGTM! Well-documented abstract method with proper return type.The PHPDoc accurately describes the expected structure of the data provider and the method is appropriately marked as abstract for subclasses to implement.
| $this->expectException(InvalidFloatValueException::class); | ||
| $this->expectExceptionMessage("Given value of '1.17E-38' is too close to zero for PostgreSQL REAL type"); |
There was a problem hiding this comment.
Exception message inconsistency
The exception message test expects "too close to zero for PostgreSQL REAL type", but the InvalidFloatValueException::forValueThatIsTooCloseToZero method appears to hardcode "DOUBLE PRECISION type" in its message according to the relevant code snippets.
This test will likely fail unless the exception message is customized per type. Consider modifying the exception class to use the correct type name dynamically.
| /** | ||
| * @test | ||
| */ | ||
| public function throws_conversion_exception_when_invalid_array_item_value(): void | ||
| { | ||
| $this->expectException(ConversionException::class); | ||
| $this->expectExceptionMessage("Given value of '1.23e4' content cannot be transformed to valid PHP float."); | ||
|
|
||
| $this->fixture->transformArrayItemForPHP('1.23e4'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The test hardcodes the scientific notation example.
This test expects an exception specifically for the value '1.23e4', but there's inconsistency with how scientific notation is handled in the implementation. The test should be made more generic or accommodate different behaviors for different float array types.
public function throws_conversion_exception_when_invalid_array_item_value(): void
{
$this->expectException(ConversionException::class);
- $this->expectExceptionMessage("Given value of '1.23e4' content cannot be transformed to valid PHP float.");
+ // Get an invalid value from the data provider instead of hardcoding
+ $invalidValue = static::provideInvalidTransformations()[0][0];
+ $this->expectExceptionMessage("cannot be transformed to valid PHP float");
- $this->fixture->transformArrayItemForPHP('1.23e4');
+ $this->fixture->transformArrayItemForPHP($invalidValue);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * @test | |
| */ | |
| public function throws_conversion_exception_when_invalid_array_item_value(): void | |
| { | |
| $this->expectException(ConversionException::class); | |
| $this->expectExceptionMessage("Given value of '1.23e4' content cannot be transformed to valid PHP float."); | |
| $this->fixture->transformArrayItemForPHP('1.23e4'); | |
| } | |
| /** | |
| * @test | |
| */ | |
| public function throws_conversion_exception_when_invalid_array_item_value(): void | |
| { | |
| $this->expectException(ConversionException::class); | |
| - $this->expectExceptionMessage("Given value of '1.23e4' content cannot be transformed to valid PHP float."); | |
| + // Get an invalid value from the data provider instead of hardcoding | |
| + $invalidValue = static::provideInvalidTransformations()[0][0]; | |
| + $this->expectExceptionMessage("cannot be transformed to valid PHP float"); | |
| - $this->fixture->transformArrayItemForPHP('1.23e4'); | |
| + $this->fixture->transformArrayItemForPHP($invalidValue); | |
| } |
| /** | ||
| * @test | ||
| * | ||
| * @dataProvider provideValidTransformations | ||
| */ | ||
| public function can_transform_from_php_value(float $phpValue, string $postgresValue): void | ||
| { | ||
| self::assertTrue($this->fixture->isValidArrayItemForDatabase($phpValue)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The test method name doesn't match the assertion.
The method is named can_transform_from_php_value, but it only tests if the value is valid for the database using isValidArrayItemForDatabase. It doesn't actually test the transformation process itself.
- public function can_transform_from_php_value(float $phpValue, string $postgresValue): void
+ public function can_validate_php_value_for_database(float $phpValue, string $postgresValue): void
{
self::assertTrue($this->fixture->isValidArrayItemForDatabase($phpValue));
}Alternatively, modify the test to actually check the transformation:
public function can_transform_from_php_value(float $phpValue, string $postgresValue): void
{
self::assertTrue($this->fixture->isValidArrayItemForDatabase($phpValue));
+ // Test actual transformation
+ $transformedValue = $this->fixture->convertToDatabaseValue([$phpValue], \Doctrine\DBAL\Platforms\PostgreSQLPlatform::class);
+ self::assertStringContainsString($postgresValue, $transformedValue);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * @test | |
| * | |
| * @dataProvider provideValidTransformations | |
| */ | |
| public function can_transform_from_php_value(float $phpValue, string $postgresValue): void | |
| { | |
| self::assertTrue($this->fixture->isValidArrayItemForDatabase($phpValue)); | |
| } | |
| /** | |
| * @test | |
| * | |
| * @dataProvider provideValidTransformations | |
| */ | |
| - public function can_transform_from_php_value(float $phpValue, string $postgresValue): void | |
| + public function can_validate_php_value_for_database(float $phpValue, string $postgresValue): void | |
| { | |
| self::assertTrue($this->fixture->isValidArrayItemForDatabase($phpValue)); | |
| } |
| /** | |
| * @test | |
| * | |
| * @dataProvider provideValidTransformations | |
| */ | |
| public function can_transform_from_php_value(float $phpValue, string $postgresValue): void | |
| { | |
| self::assertTrue($this->fixture->isValidArrayItemForDatabase($phpValue)); | |
| } | |
| /** | |
| * @test | |
| * | |
| * @dataProvider provideValidTransformations | |
| */ | |
| public function can_transform_from_php_value(float $phpValue, string $postgresValue): void | |
| { | |
| self::assertTrue($this->fixture->isValidArrayItemForDatabase($phpValue)); | |
| + // Test actual transformation | |
| + $transformedValue = $this->fixture->convertToDatabaseValue([$phpValue], \Doctrine\DBAL\Platforms\PostgreSQLPlatform::class); | |
| + self::assertStringContainsString($postgresValue, $transformedValue); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatValueException.php (1)
1-31: 💡 Verification agent❓ Verification inconclusive
Address code style issues flagged by the CI pipeline.
The pipeline failure indicates that
PHP CS Fixerfound code style violations. Please runphp-cs-fixer fix(as suggested by the pipeline logs) to resolve any remaining style inconsistencies before merging.
🏁 Script executed:
#!/bin/bash # Automatically fix code style issues using PHP CS Fixer. composer global require friendsofphp/php-cs-fixer php-cs-fixer fix --allow-risky=yesLength of output: 6155
Action Required: Resolve Code Style Issues with PHP CS Fixer
The CI pipeline still detects code style issues. The automated command failed with a “php-cs-fixer: command not found” error, which means the PHP CS Fixer executable isn’t available in the current PATH. Please verify that the composer global installation directory (typically something like
~/.composer/vendor/binor~/.config/composer/vendor/bin) is added to your PATH, or reference the full path to the executable. Once that’s done, re-run the fixer command (php-cs-fixer fix --allow-risky=yes) to address the code style issues before merging.🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: PHP CS Fixer found issues with code style. Please run 'php-cs-fixer fix' to apply the suggested fixes.
🧹 Nitpick comments (3)
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatValueException.php (1)
16-29: Strengthen test coverage for precision limit exceptions.Your exception methods allow for discrete error messages covering invalid floats, excessively precise floats, or floats too close to zero. Consider adding tests that specifically target
forValueThatExceedsMaximumPrecision()to confirm correct behavior under max precision scenarios.src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php (2)
16-17: Consider allowing trailing decimal notation (e.g., "3.").The current regex (
^-?\d*\.?\d+(?:[eE][-+]?\d+)?$) disallows floats that end with a decimal and have no digits after it. Although this may be intentional, "3." is often considered valid in many contexts. If desired, you could update the pattern to permit trailing decimal notation.- private const FLOAT_REGEX = '/^-?\d*\.?\d+(?:[eE][-+]?\d+)?$/'; + private const FLOAT_REGEX = '/^-?\d+(\.\d+)?([eE][-+]?\d+)?$/';
71-114: Comprehensive transformation with consistent exception handling.The
transformArrayItemForPHP()method ensures invalid values throw descriptive exceptions, fostering clear error-handling paths. Good job differentiating between invalid numeric formats, excessively small magnitudes, and out-of-range values. Consider adding unit tests specifically covering borderline edge cases (e.g. precise exponent boundaries).Would you like help adding tests to increase coverage for these edge conditions?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatValueException.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArrayTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/RealArrayTest.php(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
tests/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArrayTest.php (2)
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatValueException.php (1)
InvalidFloatValueException(14-30)src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php (1)
transformArrayItemForPHP(71-114)
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php (3)
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatValueException.php (4)
InvalidFloatValueException(14-30)forValueThatIsNotAValidPHPFloat(16-19)forValueThatIsTooCloseToZero(21-24)forValueThatExceedsMaximumPrecision(26-29)src/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArray.php (4)
getMinValue(19-22)getMaxValue(24-27)getMaxPrecision(29-32)getMinAbsoluteValue(34-37)src/MartinGeorgiev/Doctrine/DBAL/Types/RealArray.php (4)
getMinValue(19-22)getMaxValue(24-27)getMaxPrecision(29-32)getMinAbsoluteValue(34-37)
🪛 GitHub Actions: CI
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatValueException.php
[error] 1-1: PHP CS Fixer found issues with code style. Please run 'php-cs-fixer fix' to apply the suggested fixes.
🔇 Additional comments (6)
tests/MartinGeorgiev/Doctrine/DBAL/Types/RealArrayTest.php (2)
58-75: Well-defined negative test scenario.The method
throws_conversion_exception_when_value_too_close_to_zeroprovides excellent coverage for edge-case float inputs. It's good to see negative scenarios covered as well.
25-35:❓ Verification inconclusive
Check boundaries for oversized float values.
You've flagged
'3.402823467E+38'and'1.17E-38'as invalid inputs for the real type due to exceeding range or being too close to zero. The margin between valid and invalid can be very tight. Ensure there's a firm reference for the exact boundary of the PostgreSQLrealtype to avoid flaky tests on certain platforms or PHP versions.
🏁 Script executed:
#!/bin/bash # Verify actual float range boundaries. php -r 'echo "Float max: " . PHP_FLOAT_MAX . "\n"; echo "Float min: " . PHP_FLOAT_MIN . "\n";'Length of output: 160
Action Item: Revisit Float Boundary Values for PostgreSQL
The recent output confirms that PHP’s
PHP_FLOAT_MAXandPHP_FLOAT_MINreflect 64‑bit double values—not the boundaries for PostgreSQL’s 4‑byterealtype. PostgreSQL’s valid float range is much more constrained (approximately 3.4028235E+38 for the maximum and 1.17549435E-38 for the minimum normalized value). Given that the tests currently invalidate inputs like'3.402823467E+38'and'1.17E-38', please ensure these thresholds precisely match the PostgreSQL documentation. This verification will help avoid flaky tests across different platforms and PHP versions.tests/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArrayTest.php (2)
46-53: Good variety of valid transformations.Your test cases for valid transformations cover typical numeric patterns, decimal, and scientific notation. This helps ensure thorough coverage for double precision arrays.
28-38:❓ Verification inconclusive
Consider borderline double precision values carefully.
The chosen test data (
'1.7976931348623157E+309'and'2.2250738585072014E-309') are indeed outside the typical IEEE-754 double range. Keep in mind that certain platforms may handle subnormal or overflow differently. Confirm that these values are consistently throwing the exception in different environments.
🏁 Script executed:
#!/bin/bash # Verify consistent behavior across environments. php -r 'var_dump((float) "1.7976931348623157E+309"); var_dump((float) "2.2250738585072014E-309");'Length of output: 141
Borderline Double Precision Value Handling in Invalid Transformations Test
Our local test confirms that converting
"1.7976931348623157E+309"results inINFand"2.2250738585072014E-309"produces approximately2.225073858507203E-309. These outcomes align with IEEE-754 expectations in our current PHP environment. However, since different platforms may treat subnormal numbers or overflows differently, please ensure that the transformation logic reliably throws the expected exception for these border cases across all target environments.
- File:
tests/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArrayTest.php(Lines 28–38)- Verify that invalid transformations using these values consistently trigger an exception regardless of the execution environment.
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php (2)
1-14: Well-structured class definition and documentation.The class declaration and docblocks are well-organized, making it clear this file provides a foundational, abstract float array type with robust validation rules. Good introduction of abstract methods to enforce constraints in child classes.
26-69: Solid validation logic with thorough checks for numeric range and precision.The
isValidArrayItemForDatabase()method effectively filters out invalid or out-of-range values. The approach of expanding scientific notation to standard form and splitting on the decimal point is clear and reduces the likelihood of silent precision loss.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/MartinGeorgiev/Doctrine/DBAL/Types/RealArrayTest.php (1)
25-35: Consider adding PHPDoc with return type annotation.The
provideInvalidTransformationsmethod is missing PHPDoc with a return type annotation similar to the one inprovideValidTransformations. Adding it would improve consistency and code documentation.+/** + * @return array<int, array{0: string}> + */ public static function provideInvalidTransformations(): array { return \array_merge(parent::provideInvalidTransformations(), [ ['3.402823467E+38'], // Too large ['-3.402823467E+38'], // Too small ['1.1234567'], // Too many decimal places (>6) ['1e38'], // Scientific notation not allowed ['1.17E-38'], // Too close to zero ['-1.17E-38'], // Too close to zero (negative) ]); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArrayTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/RealArrayTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArrayTest.php
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer 1.2
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
🔇 Additional comments (4)
tests/MartinGeorgiev/Doctrine/DBAL/Types/RealArrayTest.php (4)
40-68: Test case coverage looks good.The test cases for valid transformations cover a comprehensive range of inputs, including boundary values, normal values, and edge cases, which is excellent for ensuring robustness.
70-79: Good explicit test for boundary condition.Great job including a specific test for the "too close to zero" error case. This ensures proper error handling and messaging for this boundary condition.
1-15: Class structure looks good.The test class is well-organized, properly extends the base test case, and correctly initializes the fixture in the setUp method.
17-23: Simple but effective name test.This test ensures that the type name is correctly registered, which is important for proper integration with Doctrine.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/MartinGeorgiev/Doctrine/DBAL/Types/RealArrayTest.php (1)
12-15: Consider calling parent::setUp()For consistency with DoublePrecisionArrayTest, consider adding a call to parent::setUp() in your setUp method.
protected function setUp(): void { + parent::setUp(); $this->fixture = new RealArray(); }tests/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArrayTest.php (1)
26-39: Some redundancy in invalid transformation test cases.Lines 34-37 include test cases that are already defined in the parent class's provideInvalidTransformations method (as seen in the referenced BaseFloatArrayTestCase snippet). Consider removing these redundant cases to improve maintainability.
public static function provideInvalidTransformations(): array { return \array_merge(parent::provideInvalidTransformations(), [ ['1.7976931348623157E+309'], // Too large ['-1.7976931348623157E+309'], // Too small ['1.123456789012345678'], // Too many decimal places (>15) ['2.2250738585072014E-309'], // Too close to zero ['-2.2250738585072014E-309'], // Too close to zero (negative) - ['not_a_number'], - ['1.23.45'], - ['1e'], // Invalid scientific notation - ['e1'], // Invalid scientific notation ]); }src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php (1)
102-110: Consider using more specific error messages for out-of-range values.While the validation is correct, the error message for out-of-range values is the same generic one used for invalid format values. Consider using a more specific error message for clarity.
if ($floatValue < (float) $this->getMinValue() || $floatValue > (float) $this->getMaxValue()) { - throw InvalidFloatArrayItemForPHPException::forValueThatIsNotAValidPHPFloat($item, static::TYPE_NAME); + throw InvalidFloatArrayItemForPHPException::forValueOutOfRange($item, static::TYPE_NAME); }This would require adding a new method to the
InvalidFloatArrayItemForPHPExceptionclass.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatArrayItemForDatabaseException.php(1 hunks)src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatArrayItemForPHPException.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseArrayTest.php(2 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArrayTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/DBAL/Types/RealArrayTest.php(1 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
tests/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArrayTest.php (3)
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatArrayItemForPHPException.php (1)
InvalidFloatArrayItemForPHPException(14-35)tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.php (3)
BaseFloatArrayTestCase(11-81)provideInvalidTransformations(28-41)provideValidTransformations(69-69)src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php (1)
transformArrayItemForPHP(84-127)
tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseArrayTest.php (1)
tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.php (1)
throws_domain_exception_when_invalid_array_item_value(74-80)
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php (4)
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatArrayItemForDatabaseException.php (8)
InvalidFloatArrayItemForDatabaseException(14-55)isNotANumber(21-24)doesNotMatchRegex(26-29)isAScientificNotationWithExcessPrecision(31-34)isANormalNumberWithExcessPrecision(36-39)isBelowMinValue(41-44)isAboveMaxValue(46-49)absoluteValueIsTooCloseToZero(51-54)src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatArrayItemForPHPException.php (4)
InvalidFloatArrayItemForPHPException(14-35)forValueThatIsNotAValidPHPFloat(21-24)forValueThatIsTooCloseToZero(26-29)forValueThatExceedsMaximumPrecision(31-34)src/MartinGeorgiev/Doctrine/DBAL/Types/RealArray.php (4)
getMinValue(19-22)getMaxValue(24-27)getMaxPrecision(29-32)getMinAbsoluteValue(34-37)src/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArray.php (4)
getMinValue(19-22)getMaxValue(24-27)getMaxPrecision(29-32)getMinAbsoluteValue(34-37)
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatArrayItemForDatabaseException.php (1)
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatArrayItemForPHPException.php (1)
create(16-19)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer 1.2
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.4 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: sloth
🔇 Additional comments (30)
tests/MartinGeorgiev/Doctrine/DBAL/Types/RealArrayTest.php (4)
1-24: Well-structured test class with appropriate type name verification.The test class is correctly set up to test RealArray functionality with proper type name verification.
25-35: Comprehensive test cases for invalid transformations.Good coverage of invalid transformations specific to the REAL type constraints, including values exceeding bounds, precision limits, and values too close to zero.
37-68: Appropriate test cases for valid transformations.The test cases cover a good range of valid values within the constraints of PostgreSQL's REAL type.
70-103: Well-structured exception testing.The exception tests appropriately verify that the correct exceptions are thrown with proper error messages for values too close to zero or exceeding precision limits.
tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseArrayTest.php (2)
97-97: Good naming change for more domain-specific test method.Renaming from "throws_conversion_exception_when_invalid_array_item_value" to "throws_domain_exception_when_invalid_array_item_value" improves clarity about the nature of the exception scenario being tested.
113-113: Good naming change for consistency.Renaming follows the same pattern as the previous method, improving consistency and clarity in test method naming.
tests/MartinGeorgiev/Doctrine/DBAL/Types/DoublePrecisionArrayTest.php (3)
10-24: Well-structured test class with proper setup and type verification.The class correctly extends BaseFloatArrayTestCase and initializes the DoublePrecisionArray fixture.
41-54: Good test cases for valid transformations.The test cases appropriately include scientific notation which is supported by DOUBLE PRECISION type, showing good attention to type-specific details.
56-102: Comprehensive exception testing.The tests thoroughly check for appropriate exceptions when values violate the DOUBLE PRECISION type constraints, with good coverage of edge cases.
src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatArrayItemForPHPException.php (3)
1-35: Well-designed exception class with clear error messages.This exception class provides a clean abstraction for handling specific validation failures during float array conversion. The factory methods for different validation scenarios and the formatted error messages improve code readability and maintainability.
16-19: Good use of private helper method for DRY error message creation.The private create method eliminates duplication in error message formatting across different validation scenarios.
21-34: Well-defined factory methods for specific error scenarios.Each factory method provides a clear, specific error message that helps identify the exact nature of the validation failure, which will be very helpful for debugging.
src/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArray.php (8)
1-15: New abstract class follows good design principles.The abstract class is well-structured with a clear purpose of handling float array data types. It properly extends
BaseArrayand includes the necessary exception imports.
17-25: Good design with abstract constraints.The abstract methods approach ensures concrete implementations must define their specific constraints (min/max values, precision, etc.), which is a good use of abstraction.
27-36: Clean error handling pattern.Using a try-catch block to wrap validation logic and return a boolean result is a clean pattern that keeps the public method simple while delegating the detailed validation to a private method.
38-64: Thorough validation logic for database items.The validation logic is comprehensive, checking for:
- Type validation (must be a number)
- Regex pattern validation
- Precision validation for both scientific notation and regular decimal formats
This ensures data integrity when storing values in the database.
66-74: Proper comparison with explicit type casting.The code correctly casts the string values to float before comparison, avoiding potential string comparison issues mentioned in previous feedback.
76-82: Good handling of values close to zero.The check for values too close to zero will help prevent potential precision issues when working with very small numbers that might be outside PostgreSQL's supported range.
84-98: PHP transformation has consistent validation.The validation logic in
transformArrayItemForPHPmirrors the validation inisValidArrayItemForDatabase, ensuring consistency when transforming values. The handling of null values is also implemented correctly.
112-127: Consistent handling of scientific notation and decimal formats.The code correctly handles both scientific notation and regular decimal formats. The special case for scientific notation (lines 114-116) is appropriate since precision is already validated during the conversion to float.
tests/MartinGeorgiev/Doctrine/DBAL/Types/BaseFloatArrayTestCase.php (7)
1-14: Well-structured test case class.The test case is properly organized with the right imports and a clear abstract base class pattern that will allow specific implementations to extend it.
15-23: Test method correctly validates detection of invalid values.This test method properly checks that invalid values are detected by the
isValidArrayItemForDatabasemethod.
25-41: Comprehensive test data for invalid transformations.The data provider includes a wide range of invalid values, including edge cases like invalid scientific notation formats, which is good for thorough testing.
43-51: Test method name doesn't match what's being tested.The method is named
can_transform_from_php_value, but it only tests if the value is valid for the database usingisValidArrayItemForDatabase. It doesn't actually test the transformation process itself.- public function can_transform_from_php_value(float $phpValue, string $postgresValue): void + public function can_validate_php_value_for_database(float $phpValue, string $postgresValue): void { self::assertTrue($this->fixture->isValidArrayItemForDatabase($phpValue)); }Alternatively, modify the test to actually check the transformation:
public function can_transform_from_php_value(float $phpValue, string $postgresValue): void { self::assertTrue($this->fixture->isValidArrayItemForDatabase($phpValue)); + // Test actual transformation + $transformedValue = $this->fixture->convertToDatabaseValue([$phpValue], \Doctrine\DBAL\Platforms\PostgreSQLPlatform::class); + self::assertStringContainsString($postgresValue, $transformedValue); }
53-61: Good test for PHP value transformation.This test correctly verifies that string values from the database are properly transformed into PHP float values.
63-69: Clear type definitions in PHPDoc.The return type annotation for
provideValidTransformationsis well-defined with specific array structure, which helps with type checking and IDE support.
71-80: Fixed scientific notation exception test.The test for invalid scientific notation has been corrected to use a more generic assertion message and a valid test case (
1.e234instead of the previously hardcoded1.23e4), addressing previous feedback.src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidFloatArrayItemForDatabaseException.php (3)
1-15: Well-structured exception class.The exception class properly extends Doctrine's
ConversionExceptionand includes appropriate namespace and documentation.
16-19: Clean factory pattern for exception creation.The private
createmethod follows good practice by centralizing the error message formatting, making the code DRY and easier to maintain.
21-54: Comprehensive set of specific exception factory methods.The class provides a rich set of factory methods for different error conditions, each with a clear and descriptive error message. This will make debugging easier when issues occur.
One observation: The implementation in
InvalidFloatArrayItemForDatabaseExceptionis similar toInvalidFloatArrayItemForPHPExceptionbut with slight differences in method signatures. The former takes only$valuewhile the latter takes both$valueand$type. This is consistent with their usage patterns in the code.
Summary by CodeRabbit
Documentation
New Features
Tests