Skip to content

fix(#424): address unintentional loss of string data type when retrieving PHP array item values from a stored TextArray value #443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 6, 2025
Merged

fix(#424): address unintentional loss of string data type when retrieving PHP array item values from a stored TextArray value #443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions src/MartinGeorgiev/Doctrine/DBAL/Types/TextArray.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,31 @@ public function convertToPHPValue($value, AbstractPlatform $platform): ?array

protected function transformFromPostgresTextArray(string $postgresValue): array
{
if ($postgresValue === '{}') {
return [];
$values = PostgresArrayToPHPArrayTransformer::transformPostgresArrayToPHPArray($postgresValue);

// No matter what the original PHP array items' data types were,
// once they are stored in PostgreSQL, all of them will become strings.
// Therefore, we need to ensure all items in the returned PHP array are strings.
foreach ($values as $key => $value) {
if (\is_string($value)) {
continue;
}

if (\is_bool($value)) {
$values[$key] = $value ? 'true' : 'false';

continue;
}

if ($value === null) {
$values[$key] = 'null';

continue;
}

$values[$key] = (string) $value; // @phpstan-ignore-line
}

return PostgresArrayToPHPArrayTransformer::transformPostgresArrayToPHPArray($postgresValue);
return $values;
}
}
58 changes: 52 additions & 6 deletions tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ protected function getPostgresTypeName(): string
}

#[DataProvider('provideValidTransformations')]
#[DataProvider('provideGithubIssue424TestCases')]
#[Test]
public function can_handle_array_values(string $testName, array $arrayValue): void
{
Expand All @@ -29,12 +30,57 @@ public function can_handle_array_values(string $testName, array $arrayValue): vo
public static function provideValidTransformations(): array
{
return [
'simple text array' => ['simple text array', ['foo', 'bar', 'baz']],
'text array with special chars' => ['text array with special chars', ['foo"bar', 'baz\qux', 'with,comma']],
'text array with empty strings' => ['text array with empty strings', ['', 'not empty', '']],
'text array with unicode' => ['text array with unicode', ['café', 'naïve', 'résumé']],
'text array with numbers as strings' => ['text array with numbers as strings', ['123', '456', '789']],
'text array with null elements' => ['text array with null elements', ['foo', null, 'baz']],
'simple text array' => [
'simple text array',
['foo', 'bar', 'baz'],
],
'text array with special chars' => [
'text array with special chars',
['foo"bar', 'baz\qux', 'with,comma'],
],
'text array with empty strings' => [
'text array with empty strings',
['', 'not empty', ''],
],
'text array with unicode' => [
'text array with unicode',
['café', 'naïve', 'résumé'],
],
'text array with numbers as strings' => [
'text array with numbers as strings',
['123', '456', '789'],
],
'text array with null element as string' => [
'text array with null elements',
['foo', 'null', 'baz'],
],
];
}

/**
* This test scenarios specifically verify the scenarios from GitHub issue #424
* where PostgreSQL optimizes {"1","test"} to {1,test} and we have to ensure
* that TextArray correctly preserves string types when converted back for PHP.
*/
public static function provideGithubIssue424TestCases(): array
{
return [
'numeric values' => [
'Numeric values should be preserved as strings',
['1', 'test'],
],
'mixed values' => [
'Mixed numeric values should be preserved as strings',
['1', '2.5', '3.14', 'test', 'true', ''],
],
'boolean values' => [
'Boolean values should be converted to strings',
['1', '', '1', ''],
],
'null values' => [
'Null values should be converted to strings',
['', 'null', 'NULL'],
],
];
}
}
57 changes: 57 additions & 0 deletions tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,4 +122,61 @@ public function can_handle_backslashes_correctly(): void

$this->assertEquals($expectedPhpValue, $this->fixture->convertToPHPValue($postgresValue, $this->platform));
}

#[DataProvider('provideGithubIssue424TestCases')]
#[Test]
public function can_preserve_string_types_retrieved_from_database_for_github_issue_424(string $testName, string $postgresValue, array $expectedResult): void
{
$result = $this->fixture->convertToPHPValue($postgresValue, $this->platform);

$this->assertSame($expectedResult, $result, $testName);

// Verify all values are strings
foreach ($result as $value) {
$this->assertIsString($value, $testName.' - all values should be strings');
}
}

/**
* @return list<array{
* testName: string,
* postgresValue: string,
* expectedResult: array{string}
* }>
*/
public static function provideGithubIssue424TestCases(): array
{
return [
[
'testName' => 'Numeric values should be preserved as strings',
'postgresValue' => '{1,test}',
'expectedResult' => ['1', 'test'],
],
[
'testName' => 'Mixed values should be preserved as strings',
'postgresValue' => '{1,2.5,3.14,test,true,false}',
'expectedResult' => ['1', '2.5', '3.14', 'test', 'true', 'false'],
],
[
'testName' => 'Boolean-like values should be converted to strings',
'postgresValue' => '{true,false}',
'expectedResult' => ['true', 'false'],
],
[
'testName' => 'Quoted boolean-like values should remain as strings',
'postgresValue' => '{"true","false","t","f"}',
'expectedResult' => ['true', 'false', 't', 'f'],
],
[
'testName' => 'Mixed quoted/unquoted values should all be strings',
'postgresValue' => '{1,"2",true,"false",3.14,"test"}',
'expectedResult' => ['1', '2', 'true', 'false', '3.14', 'test'],
],
[
'testName' => 'Null values should be converted to strings',
'postgresValue' => '{"",null,"null","NULL"}',
'expectedResult' => ['', 'null', 'null', 'NULL'],
],
];
}
}
4 changes: 4 additions & 0 deletions tests/Unit/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,10 @@ public static function provideValidTransformations(): array
'phpValue' => [' foo '],
'postgresValue' => '{" foo "}',
],
'github #424 regression: numeric strings should be preserved as strings when unquoted' => [
'phpValue' => ['1', 'test', 'true'],
'postgresValue' => '{1,test,true}',
],
Comment on lines +169 to +172
Copy link

@coderabbitai coderabbitai bot Sep 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix: test contradicts transformer’s contract (should return typed values for unquoted tokens).

transformPostgresArrayToPHPArray('{1,test,true}') intentionally returns [1, 'test', true] (int/bool for unquoted numeric/boolean tokens). The new case expects strings and will fail/regress the intended API. Either move this case under TextArray tests (which normalize to strings) or update the expected values here.

Apply this minimal fix to keep the test aligned with the transformer behavior:

-            'github #424 regression: numeric strings should be preserved as strings when unquoted' => [
-                'phpValue' => ['1', 'test', 'true'],
+            'github #424 context: unquoted numeric/boolean tokens remain typed at transformer level' => [
+                'phpValue' => [1, 'test', true],
                 'postgresValue' => '{1,test,true}',
             ],
📝 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.

Suggested change
'github #424 regression: numeric strings should be preserved as strings when unquoted' => [
'phpValue' => ['1', 'test', 'true'],
'postgresValue' => '{1,test,true}',
],
'github #424 context: unquoted numeric/boolean tokens remain typed at transformer level' => [
'phpValue' => [1, 'test', true],
'postgresValue' => '{1,test,true}',
],
🤖 Prompt for AI Agents 
In tests/Unit/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.php
around lines 169 to 172, the new test case expects numeric/boolean tokens to
remain strings but the transformer contract returns typed values for unquoted
tokens; update the test to match the transformer by changing 'phpValue' =>
['1','test','true'] to 'phpValue' => [1,'test',true] (or alternatively move this
case into the TextArray tests where string normalization is expected) so the
test aligns with the transformer's intended behavior.

Copy link
Owner Author

@martin-georgiev martin-georgiev Sep 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct; however, I believe the old test scenario is less realistic because TextArray users shall expect the database to normalise data to strings only, which is where data type loss occurs. Hence, the tradeoff (or assumption) that the intended PHP data is always a string and not other data types sounds reasonable.

Copy link

@coderabbitai coderabbitai bot Sep 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martin-georgiev Thank you for the clarification! Your explanation makes perfect sense from the TextArray design perspective. You're right that TextArray users should expect consistent string normalization, and this test case documents an important input scenario that the transformer needs to handle for TextArray's use case. The design tradeoff of assuming intended PHP data is always strings for TextArray is indeed reasonable and aligns with the PostgreSQL text[] type semantics.

I appreciate you taking the time to explain the broader design context - it helps understand why this test case structure supports the overall fix for issue #424.

✏️ Learnings added 
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#443
File: tests/Unit/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.php:169-172
Timestamp: 2025-09-06T22:15:32.732Z
Learning: In martin-georgiev/postgresql-for-doctrine, TextArray users should expect the database to normalize data to strings only. The design assumes that intended PHP data for TextArray is always strings, not other data types, which is a reasonable tradeoff that aligns with PostgreSQL text[] type semantics.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#340
File: tests/MartinGeorgiev/Doctrine/DBAL/Types/InetArrayTest.php:145-145
Timestamp: 2025-04-11T11:23:44.192Z
Learning: In the PostgreSQL for Doctrine test cases, methods that test database-to-PHP conversions should use `mixed` type for parameter and include non-string test cases in their data providers, following the pattern in classes like InetTest, CidrTest, and MacaddrTest.

Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#0
File: :0-0
Timestamp: 2025-03-26T02:46:12.804Z
Learning: The PR "preserve the type of floats and integers when transforming back and forth between PostgreSQL and PHP" improves type handling by ensuring that integers remain integers, floats remain floats, numeric strings stay as strings, and booleans are properly converted through the transformation process.

Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#421
File: docs/AVAILABLE-TYPES.md:31-33
Timestamp: 2025-08-19T13:07:15.184Z
Learning: In martin-georgiev/postgresql-for-doctrine, the point[] array type should be documented as "point[]" not "_point" in the AVAILABLE-TYPES.md table, to be consistent with all other array types like text[], jsonb[], inet[], etc.

Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#434
File: tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/PostGIS/ST_CrossesTest.php:12-31
Timestamp: 2025-09-01T18:48:28.508Z
Learning: When analyzing unit test files in this codebase, always verify the actual file structure and existing patterns before suggesting changes. The ContainsGeometries fixture exists at ./fixtures/MartinGeorgiev/Doctrine/Entity/ContainsGeometries.php and the PostGIS unit tests use the namespace Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\PostGIS consistently.

Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#421
File: docs/AVAILABLE-TYPES.md:31-33
Timestamp: 2025-08-19T13:07:15.184Z
Learning: In martin-georgiev/postgresql-for-doctrine, the AVAILABLE-TYPES.md documentation table's second column shows DBAL type names (what TYPE_NAME constants contain and getName() method returns) not PostgreSQL internal catalogue names. Array types use the format like 'text[]', 'jsonb[]', 'inet[]' - not the underscore-prefixed PostgreSQL internal names like '_text', '_jsonb', '_inet'.

];
}

Expand Down
Loading