added test cases for (#351) #389
Conversation
WalkthroughA new private static method was added to a test class to manually parse Postgres array strings into PHP arrays. The main test method now asserts both the existing transformation and the new parsing method. Additional test cases were added to cover special characters, backslashes, and regression issues. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as PostgresArrayToPHPArrayTransformerTest
participant Transformer as PostgresArrayToPHPArrayTransformer
participant LegacyParser as transformPostgresTextArrayToPHPArray
Test->>Transformer: Call transformPostgresArrayToPHPArray(postgresArray)
Transformer-->>Test: PHP array result
Test->>LegacyParser: Call transformPostgresTextArrayToPHPArray(postgresArray)
LegacyParser-->>Test: PHP array result (manual parsing)
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)tests/Unit/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.php (1)🔇 Additional comments (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Unit/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.php(3 hunks)
🔇 Additional comments (3)
tests/Unit/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.php (3)
22-23: LGTM! Good approach for version comparison testing.The dual assertion strategy effectively compares V3 and V2 behavior in the same test case, making it easy to identify behavioral differences between versions.
146-153: LGTM! Comprehensive regression test coverage.The new test cases effectively cover the GitHub #351 regression scenarios with special characters and backslashes, including both single and multi-element array cases.
264-302:❓ Verification inconclusive
Verify the legacy method actually represents v2.10.3 behavior.
Since this method is meant to replicate v2.10.3 logic, ensure the implementation accurately reflects the actual behavior of that version, including any quirks or limitations that existed.
🏁 Script executed:
#!/bin/bash # Description: Search for the original v2.10.3 implementation to verify accuracy # Expected: Find the original transformation logic to compare # Search for version tags and transformation method in git history git tag | grep -E "v?2\.10\.3" # Search for the original transformation method implementation rg -A 50 "transformPostgres.*Array.*PHP" --type phpLength of output: 45874
Confirm legacy v2.10.3 transform logic
I didn’t find a
v2.10.3tag or historical implementation in this branch—instead, the only “v2.10.3” logic lives in the test’stransformPostgresTextArrayToPHPArrayhelper. Please manually verify that this method faithfully reproduces the original v2.10.3 behavior (including its edge-case quirks around nulls, escaping, numeric precision, etc.).
| // function from v2.10.3 | ||
| private static function transformPostgresTextArrayToPHPArray(string $postgresArray): array | ||
| { | ||
| $transform = static function (string $textArrayToTransform): array { | ||
| $indicatesMultipleDimensions = \mb_strpos($textArrayToTransform, '},{') !== false | ||
| || \mb_strpos($textArrayToTransform, '{{') === 0; | ||
| if ($indicatesMultipleDimensions) { | ||
| throw new \InvalidArgumentException('Only single-dimensioned arrays are supported'); | ||
| } | ||
|
|
||
| $phpArray = \str_getcsv(\trim($textArrayToTransform, '{}'), escape: '\\'); | ||
| foreach ($phpArray as $i => $text) { | ||
| if ($text === null) { | ||
| unset($phpArray[$i]); | ||
|
|
||
| break; | ||
| } | ||
|
|
||
| $isInteger = \is_numeric($text) && ''.(int) $text === $text; | ||
| if ($isInteger) { | ||
| $phpArray[$i] = (int) $text; | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| $isFloat = \is_numeric($text) && ''.(float) $text === $text; | ||
| if ($isFloat) { | ||
| $phpArray[$i] = (float) $text; | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| $phpArray[$i] = \stripslashes(\str_replace('\"', '"', $text)); | ||
| } | ||
|
|
||
| return $phpArray; | ||
| }; | ||
|
|
||
| return $transform($postgresArray); | ||
| } |
There was a problem hiding this comment.
Critical issues in the legacy transformation method implementation.
The legacy method has several problems that will cause test failures and incorrect behavior:
-
Logic error with null handling: The break statement on line 278 will terminate processing after the first element if any element is null, leaving subsequent elements unprocessed.
-
Inconsistent exception type: Uses
\InvalidArgumentExceptioninstead ofInvalidArrayFormatExceptionused elsewhere in the codebase. -
Missing type conversions: Doesn't handle boolean values (
true,false,t,f) or null string conversion (NULL,null) like the V3 transformer does. -
Unnecessary complexity: The closure wrapper adds no value and makes the code harder to read.
Apply this diff to fix the critical issues:
- // function from v2.10.3
- private static function transformPostgresTextArrayToPHPArray(string $postgresArray): array
- {
- $transform = static function (string $textArrayToTransform): array {
- $indicatesMultipleDimensions = \mb_strpos($textArrayToTransform, '},{') !== false
- || \mb_strpos($textArrayToTransform, '{{') === 0;
- if ($indicatesMultipleDimensions) {
- throw new \InvalidArgumentException('Only single-dimensioned arrays are supported');
- }
-
- $phpArray = \str_getcsv(\trim($textArrayToTransform, '{}'), escape: '\\');
- foreach ($phpArray as $i => $text) {
- if ($text === null) {
- unset($phpArray[$i]);
-
- break;
- }
-
- $isInteger = \is_numeric($text) && ''.(int) $text === $text;
- if ($isInteger) {
- $phpArray[$i] = (int) $text;
-
- continue;
- }
-
- $isFloat = \is_numeric($text) && ''.(float) $text === $text;
- if ($isFloat) {
- $phpArray[$i] = (float) $text;
-
- continue;
- }
-
- $phpArray[$i] = \stripslashes(\str_replace('\"', '"', $text));
- }
-
- return $phpArray;
- };
-
- return $transform($postgresArray);
- }
+ // function from v2.10.3
+ private static function transformPostgresTextArrayToPHPArray(string $postgresArray): array
+ {
+ $indicatesMultipleDimensions = \mb_strpos($postgresArray, '},{') !== false
+ || \mb_strpos($postgresArray, '{{') === 0;
+ if ($indicatesMultipleDimensions) {
+ throw new InvalidArrayFormatException('Only single-dimensioned arrays are supported');
+ }
+
+ $phpArray = \str_getcsv(\trim($postgresArray, '{}'), escape: '\\');
+ foreach ($phpArray as $i => $text) {
+ // Handle null values
+ if (\in_array(\strtolower($text), ['null', ''], true)) {
+ $phpArray[$i] = null;
+ continue;
+ }
+
+ // Handle boolean values
+ if (\in_array(\strtolower($text), ['true', 't'], true)) {
+ $phpArray[$i] = true;
+ continue;
+ }
+ if (\in_array(\strtolower($text), ['false', 'f'], true)) {
+ $phpArray[$i] = false;
+ continue;
+ }
+
+ $isInteger = \is_numeric($text) && ''.(int) $text === $text;
+ if ($isInteger) {
+ $phpArray[$i] = (int) $text;
+ continue;
+ }
+
+ $isFloat = \is_numeric($text) && ''.(float) $text === $text;
+ if ($isFloat) {
+ $phpArray[$i] = (float) $text;
+ continue;
+ }
+
+ $phpArray[$i] = \stripslashes(\str_replace('\"', '"', $text));
+ }
+
+ return $phpArray;
+ }📝 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.
| // function from v2.10.3 | |
| private static function transformPostgresTextArrayToPHPArray(string $postgresArray): array | |
| { | |
| $transform = static function (string $textArrayToTransform): array { | |
| $indicatesMultipleDimensions = \mb_strpos($textArrayToTransform, '},{') !== false | |
| || \mb_strpos($textArrayToTransform, '{{') === 0; | |
| if ($indicatesMultipleDimensions) { | |
| throw new \InvalidArgumentException('Only single-dimensioned arrays are supported'); | |
| } | |
| $phpArray = \str_getcsv(\trim($textArrayToTransform, '{}'), escape: '\\'); | |
| foreach ($phpArray as $i => $text) { | |
| if ($text === null) { | |
| unset($phpArray[$i]); | |
| break; | |
| } | |
| $isInteger = \is_numeric($text) && ''.(int) $text === $text; | |
| if ($isInteger) { | |
| $phpArray[$i] = (int) $text; | |
| continue; | |
| } | |
| $isFloat = \is_numeric($text) && ''.(float) $text === $text; | |
| if ($isFloat) { | |
| $phpArray[$i] = (float) $text; | |
| continue; | |
| } | |
| $phpArray[$i] = \stripslashes(\str_replace('\"', '"', $text)); | |
| } | |
| return $phpArray; | |
| }; | |
| return $transform($postgresArray); | |
| } | |
| // function from v2.10.3 | |
| private static function transformPostgresTextArrayToPHPArray(string $postgresArray): array | |
| { | |
| $indicatesMultipleDimensions = \mb_strpos($postgresArray, '},{') !== false | |
| || \mb_strpos($postgresArray, '{{') === 0; | |
| if ($indicatesMultipleDimensions) { | |
| throw new InvalidArrayFormatException('Only single-dimensioned arrays are supported'); | |
| } | |
| $phpArray = \str_getcsv(\trim($postgresArray, '{}'), escape: '\\'); | |
| foreach ($phpArray as $i => $text) { | |
| // Handle null values | |
| if (\in_array(\strtolower($text), ['null', ''], true)) { | |
| $phpArray[$i] = null; | |
| continue; | |
| } | |
| // Handle boolean values | |
| if (\in_array(\strtolower($text), ['true', 't'], true)) { | |
| $phpArray[$i] = true; | |
| continue; | |
| } | |
| if (\in_array(\strtolower($text), ['false', 'f'], true)) { | |
| $phpArray[$i] = false; | |
| continue; | |
| } | |
| $isInteger = \is_numeric($text) && ''.(int) $text === $text; | |
| if ($isInteger) { | |
| $phpArray[$i] = (int) $text; | |
| continue; | |
| } | |
| $isFloat = \is_numeric($text) && ''.(float) $text === $text; | |
| if ($isFloat) { | |
| $phpArray[$i] = (float) $text; | |
| continue; | |
| } | |
| $phpArray[$i] = \stripslashes(\str_replace('\"', '"', $text)); | |
| } | |
| return $phpArray; | |
| } |
🤖 Prompt for AI Agents
In tests/Unit/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.php
between lines 263 and 302, fix the legacy transformPostgresTextArrayToPHPArray
method by removing the break statement that prematurely stops processing on null
elements, replace the \InvalidArgumentException with InvalidArrayFormatException
for consistency, add handling to convert boolean string values ('true', 'false',
't', 'f') and null strings ('NULL', 'null') to their proper PHP types, and
eliminate the unnecessary closure wrapper to simplify the code structure.
| 'postgresValue' => '{"!@#\$%^&*()_+=-}{[]|\":;\'\?><,./"}', | ||
| 'github #351 regression #1: string with special characters and backslash' => [ | ||
| 'phpValue' => ['⥀!@#$%^&*()_+=-}{[]|":;\'\?><,./'], | ||
| 'postgresValue' => '{"⥀!@#$%^&*()_+=-}{[]|\\":;\'\\\\?><,./"}', |
There was a problem hiding this comment.
@j4r3kb why do you expect PHP's '\? to become '\\\\? in Postgres?
There was a problem hiding this comment.
I used Xdebug. Have a breakpoint where the value is returned from the DB, then used the option "Copy value as var export" - maybe it was a mistake on my part.
Plain copy value gives {"⥀!@#$%^&*()_+=-}{[]|\":;'\\?><,./"}. Then you have to quote it in the code to be part of the array, so it should become
'postgresValue' => '{"⥀!@#$%^&*()_+=-}{[]|\":;\'\\?><,./"}',
I think the tricky part in the input array of 'phpValue' => ['⥀!@#$%^&*()_+=-}{[]|":;\'\?><,./'], is the \' inside the string. The array value is enclosed in single quotes and contains a single quote inside that has to be escaped by \.
So, as it returns from the DB it should stay like this and only the \ before ? should be returned as \\.
Sorry If I mess something up with those return values, it's confusing with the escaping.
However you alter the code keep in mind the function I pasted here from v2. They both should have the same result right?
There was a problem hiding this comment.
I pushed a new commit with a changed postgresValue.
|
@j4r3kb I moved your test cases in https://github.com/martin-georgiev/postgresql-for-doctrine/pull/391/files#diff-33c98a8b8541e82e2190de26db1e1f6c9314c609aa6405cc46b2d64f490200beR37-R38 |
2 participants
Highlight differences between v2 and v3 of the package
Summary by CodeRabbit