fix(#424): address unintentional loss of string data type when retrieving PHP array item values from a stored TextArray value
#443
Conversation
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
…ving PHP array item values from a stored `TextArray` value
WalkthroughRefactors TextArray::transformFromPostgresTextArray to always return an array of strings by normalizing parsed elements. Updates integration and unit tests to cover string preservation across numeric, boolean-like, and null-like values, including regression cases linked to GitHub issue #424. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant T as TextArray::transformFromPostgresTextArray
participant X as PostgresArrayToPHPArrayTransformer
C->>T: transformFromPostgresTextArray(postgresValue)
T->>X: transformPostgresArrayToPHPArray(postgresValue)
X-->>T: values (mixed[])
rect rgba(200,230,255,0.3)
note right of T: Normalize each element
T->>T: for each v:<br/>- if string: keep<br/>- if bool: "true"/"false"<br/>- if null: "null"<br/>- else: (string) v
end
T-->>C: string[] values
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. Comment |
![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
🧹 Nitpick comments (3)
src/MartinGeorgiev/Doctrine/DBAL/Types/TextArray.php (1)
65-91: Good fix: normalize parsed values to strings at TextArray level (addresses #424).Logic is clear and preserves DB semantics by mapping bool/null explicitly; remaining scalars are string-cast.
Consider adding a precise return phpdoc to help static analysis:
/** @return array<int,string> */ protected function transformFromPostgresTextArray(string $postgresValue): arraytests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTest.php (1)
140-147: Docblock type fix for provider shape.
expectedResult: array{string}denotes a single-element shaped array. Use a list of strings instead.- * expectedResult: array{string} + * expectedResult: list<string>tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTypeTest.php (1)
33-57: Nit: label consistency for the “null element” case.Outer key says “null element as string” while the description says “null elements”. Align wording to avoid confusion.
- 'text array with null elements', + 'text array with null element as string',
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/MartinGeorgiev/Doctrine/DBAL/Types/TextArray.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTypeTest.php(2 hunks)tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTest.php(1 hunks)tests/Unit/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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#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'.
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.
📚 Learning: 2025-04-11T11:23:44.192Z
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.
Applied to files:
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTest.phptests/Unit/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTypeTest.php
📚 Learning: 2025-09-01T18:48:28.508Z
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.
Applied to files:
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTest.phptests/Unit/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.php
📚 Learning: 2025-08-19T13:07:15.184Z
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'.
Applied to files:
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTest.phpsrc/MartinGeorgiev/Doctrine/DBAL/Types/TextArray.php
📚 Learning: 2025-08-24T16:52:32.488Z
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#0
File: :0-0
Timestamp: 2025-08-24T16:52:32.488Z
Learning: This repository uses BaseType extension pattern for custom Doctrine DBAL types with PostgreSQL platform assertions, comprehensive unit and integration testing with data providers, and dedicated exception classes for type conversion errors.
Applied to files:
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTest.phptests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTypeTest.php
📚 Learning: 2025-04-22T00:03:37.733Z
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#357
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpMatchTest.php:28-62
Timestamp: 2025-04-22T00:03:37.733Z
Learning: This project focuses on providing Doctrine ORM interfaces to PostgreSQL functions. Tests should validate correct DQL-to-SQL translation rather than PostgreSQL functionality itself. Test cases should focus on parameter passing and SQL generation, not on testing specific PostgreSQL regex pattern behaviors.
Applied to files:
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTest.phptests/Unit/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.php
📚 Learning: 2025-03-26T02:46:12.804Z
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.
Applied to files:
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTest.phptests/Unit/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.php
📚 Learning: 2025-08-19T13:07:15.184Z
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.
Applied to files:
src/MartinGeorgiev/Doctrine/DBAL/Types/TextArray.php
📚 Learning: 2025-08-19T13:07:15.184Z
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 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'.
Applied to files:
src/MartinGeorgiev/Doctrine/DBAL/Types/TextArray.php
🧬 Code graph analysis (3)
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTest.php (3)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTypeTest.php (2)
DataProvider(22-28)provideGithubIssue424TestCases(65-85)tests/Unit/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformerTest.php (6)
DataProvider(18-23)DataProvider(176-183)DataProvider(197-202)DataProvider(239-246)Test(225-230)Test(232-237)src/MartinGeorgiev/Doctrine/DBAL/Types/TextArray.php (1)
convertToPHPValue(54-61)
src/MartinGeorgiev/Doctrine/DBAL/Types/TextArray.php (3)
src/MartinGeorgiev/Utils/PostgresArrayToPHPArrayTransformer.php (2)
PostgresArrayToPHPArrayTransformer(16-250)transformPostgresArrayToPHPArray(29-90)src/MartinGeorgiev/Doctrine/DBAL/Types/JsonbArray.php (1)
transformPostgresArrayToPHPArray(35-38)src/MartinGeorgiev/Doctrine/DBAL/Types/BaseArray.php (1)
transformPostgresArrayToPHPArray(127-135)
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTypeTest.php (1)
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTest.php (4)
DataProvider(36-41)DataProvider(43-48)DataProvider(126-138)provideGithubIssue424TestCases(147-181)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: sloth
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.1
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.4
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.4
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.3
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.4
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.4
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.1
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.1
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.3
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.3
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.2
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.3
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.2
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.2
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.2
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.1
- GitHub Check: wait-for-tests-worflows-before-upload
🔇 Additional comments (4)
tests/Unit/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTest.php (2)
126-139: LGTM: regression coverage ensures all returned elements are strings.Verifies both exact result and type of each element; matches the new normalization behavior.
147-181: Test data reads well and matches TextArray normalization.Covers numeric, mixed, boolean-like, quoted/unquoted, and null-like scenarios with strict comparisons.
tests/Integration/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTypeTest.php (2)
23-23: Nice: combined data providers to exercise both standard and #424 scenarios.Keeps the integration path covered without duplicating test bodies.
60-84: LGTM: #424 provider clarifies the expected string-preservation behavior end-to-end.Scenarios reflect the DB optimization of unquoted tokens and ensure the type normalization holds after roundtrip.
Comment on lines
+169
to
+172
| 'github #424 regression: numeric strings should be preserved as strings when unquoted' => [ | ||
| 'phpValue' => ['1', 'test', 'true'], | ||
| 'postgresValue' => '{1,test,true}', | ||
| ], |
There was a problem hiding this comment.
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.
Owner
Author
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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'.
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.
Summary by CodeRabbit
Bug Fixes
Tests