fix: parse text[] as array<string> #157

adrienbrault · 2023-09-12T09:34:02Z

Hey,

The way MartinGeorgiev\Doctrine\DBAL\Types\TextArray converts database values to php floats/int broke my expectations.

When I save ['a', '1', '0.1'] to my database, I expect to get back the same ['a', '1', '0.1'], NOT ['a', 1, 0.1]. Especially when the column name is "text".

I also checked postgres itself, and there is no magic casting:

database=# select pg_typeof(('{0.4}'::text[])[1]);
 pg_typeof
-----------
 text
(1 row)

greptile-apps

PR Summary

This PR fixes type handling in PostgreSQL text arrays by ensuring numeric values remain as strings rather than being automatically converted to integers/floats.

Modified src/MartinGeorgiev/Utils/DataStructure.php to preserve string types for numeric values in text arrays
Updated tests/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTest.php to use strict type checking with assertSame()
Adjusted test data in tests/MartinGeorgiev/Utils/DataStructureTest.php to properly handle numeric values as strings
Aligned behavior with PostgreSQL's native text[] type which maintains all elements as strings

_{💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!}

_{3 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-12-27T14:10:38Z

src/MartinGeorgiev/Utils/DataStructure.php

-
-                    continue;
-                }
-
                if (!\is_string($text)) {
                    $exceptionMessage = 'Unsupported data type encountered. Expected null, integer, float or string value. Instead it is "%s".';


logic: Error message mentions expecting integer/float values but code no longer handles these types

greptile-apps · 2024-12-27T14:10:39Z

tests/MartinGeorgiev/Utils/DataStructureTest.php

@@ -43,7 +43,7 @@ public static function provideValidTransformations(): array
                    0 => 'dfasdf',
                    1 => 'qw,,e{q"we',
                    2 => "'qrer'",
-                    3 => 604,
+                    3 => '604',
                    4 => '"aaa","b""bb","ccc"',
                ],
                'postgresValue' => '{"dfasdf","qw,,e{q\"we","\'qrer\'",604,"\"aaa\",\"b\"\"bb\",\"ccc\""}',


logic: Mismatch between test data - phpValue has '604' as string but postgresValue has 604 as unquoted number. This will cause type inconsistency.

martin-georgiev · 2024-12-27T14:17:09Z

tests/MartinGeorgiev/Doctrine/DBAL/Types/TextArrayTest.php

@@ -44,7 +44,7 @@ public static function provideValidTransformations(): array
            ],
            [
                'phpValue' => [
-                    1,


Why was this changed?

martin-georgiev · 2024-12-27T14:17:36Z

tests/MartinGeorgiev/Utils/DataStructureTest.php

@@ -43,7 +43,7 @@ public static function provideValidTransformations(): array
                    0 => 'dfasdf',
                    1 => 'qw,,e{q"we',
                    2 => "'qrer'",
-                    3 => 604,


Why was this changed?

martin-georgiev · 2025-03-13T19:14:06Z

@coderabbitai wdyt?

coderabbitai · 2025-03-13T19:15:54Z

🧩 Analysis chain

I'll need to see the specific changes in this PR.

Let me look more specifically for the TextArray implementation and the changes:

Let's focus on analyzing the PHP files related to array type handling:

Let's ensure we're getting the TextArray file and its changes:

Let's take a different approach to find the relevant file:

Let's try to find all array type handlers in the codebase and inspect the PR changes directly:

Now that I can see the PR changes, let me check the specific files mentioned in the commit:

Let's try a more direct approach:

Now I need to analyze the functionality of the TextArray type to understand the impact of the changes:

Since I still haven't found the file, let's try another approach:

Let's look at the PR directly from GitHub:

Now I have a better understanding of the PR. Let me analyze the significance of the change:

@martin-georgiev After reviewing the PR, I believe this is a correct fix. The current behavior of TextArray is converting numeric-looking strings to their respective numeric types (int/float), which doesn't match PostgreSQL's behavior.

When PostgreSQL stores data in a text[] column, all elements remain as text strings regardless of content. This PR ensures that the PHP representation maintains the same types - keeping all values as strings even if they look like numbers.

This change improves type consistency and follows the principle of least surprise. Users would expect ['a', '1', '0.1'] stored in a text array to come back exactly the same way, not with automatic type conversions.

I recommend approving this PR as it makes the library's behavior more predictable and consistent with PostgreSQL's native typing system.

🏁 Script executed:

#!/bin/bash
# Get the PR details
gh pr view 157 --repo martin-georgiev/postgresql-for-doctrine --json files

# Get information about the repository structure to understand the context better
find . -type f -name "*.php" | grep -i textarray || echo "No TextArray.php files found directly"

# Let's look for TextArray class
find . -type f -name "*.php" | xargs grep -l "TextArray" || echo "No files mentioning TextArray"

Length of output: 793

🏁 Script executed:

#!/bin/bash
# Get the diff of the PR
gh pr diff 157 --repo martin-georgiev/postgresql-for-doctrine

# List files changed in the PR
gh pr view 157 --repo martin-georgiev/postgresql-for-doctrine --json files --jq '.files[].path'

# Find the TextArray class and get its contents
find . -type f -name "*.php" -exec grep -l "class TextArray" {} \; | xargs cat

Length of output: 9094

🏁 Script executed:

#!/bin/bash
# Clone the repository to ensure we have the complete codebase
cd ..
rm -rf postgresql-for-doctrine || true
git clone https://github.com/martin-georgiev/postgresql-for-doctrine.git
cd postgresql-for-doctrine

# Fetch all branches and checkout the PR branch
git fetch origin pull/157/head:pr-157
git checkout pr-157

# Find the TextArray.php file
find . -name "TextArray.php"

# Display the TextArray.php content
find . -name "TextArray.php" -exec cat {} \;

# Show the difference between main and PR branch for this file
git diff main...HEAD -- $(find . -name "TextArray.php")

Length of output: 2645

🏁 Script executed:

#!/bin/bash
# Search for files with "Array" in their name in the src directory
find ./src -name "*Array*.php"

# Display the content of all found files
find ./src -name "*Array*.php" -exec echo "File: {}" \; -exec cat {} \; -exec echo -e "\n---\n" \;

# Show the diff for these files between main and the PR branch
for file in $(find ./src -name "*Array*.php"); do
  echo "Diff for $file:"
  git diff main...HEAD -- $file
  echo -e "\n---\n"
done

Length of output: 34615

🏁 Script executed:

#!/bin/bash
# Switch back to the main branch to see the original implementation
git checkout main

# Find TextArray files
find ./src -name "*TextArray*.php" -o -name "*Array*.php" | grep -i text

# Show content of the TextArray file
file=$(find ./src -name "*TextArray*.php" -o -name "*Array*.php" | grep -i text | head -1)
if [ -n "$file" ]; then
  echo "Content of $file on main branch:"
  cat "$file"
  
  # Switch to PR branch and show the diff
  git checkout pr-157
  echo -e "\nContent of $file on PR branch:"
  cat "$file"
  
  echo -e "\nDiff between main and PR for $file:"
  git diff main...pr-157 -- "$file"
else
  echo "TextArray file not found"
fi

Length of output: 5280

🏁 Script executed:

#!/bin/bash
# Find PHP files that might contain the TextArray class
find ./src -type f -name "*.php" -exec grep -l "class TextArray" {} \;

# Let's look for files that mention "TextArray" in their content
find ./src -type f -name "*.php" -exec grep -l "TextArray" {} \;

# Show the PR changes
git checkout pr-157
git show HEAD

Length of output: 78207

🏁 Script executed:

#!/bin/bash
# Find any PHP file that might be related to array types
find ./src -type f -name "*.php" -exec grep -l "Array" {} \;

# Get the exact commit changes from the PR
pr_commit=$(git log --format="%H" -n 1)
git show $pr_commit

Length of output: 71498

🏁 Script executed:

#!/bin/bash
# Check for the TextArray type implementation
git checkout pr-157
find ./src -type f -name "*.php" | xargs grep -l "class TextArray" --include="*.php"

# Let me look at the contents of that file
textarray_file=$(find ./src -type f -name "*.php" | xargs grep -l "class TextArray" --include="*.php")
if [ -n "$textarray_file" ]; then
  echo "Contents of $textarray_file:"
  cat "$textarray_file"
  
  echo -e "\nDiff for $textarray_file:"
  git diff main..HEAD -- "$textarray_file"
else
  echo "TextArray class file not found"
fi

Length of output: 2837

🏁 Script executed:

#!/bin/bash
# Get the list of modified files in the PR
git diff --name-only main..HEAD

# Show the diff for each modified file
git diff main..HEAD

Length of output: 55

🏁 Script executed:

#!/bin/bash
# Get current implementation by file name
file="src/DBAL/Types/TextArray.php"
if [ -f "$file" ]; then
  echo "Contents of $file on PR branch:"
  cat "$file"
  
  # Switch to main branch to see original implementation
  git checkout main
  echo -e "\nContents of $file on main branch:"
  if [ -f "$file" ]; then
    cat "$file"
  else
    echo "File doesn't exist on main branch"
  fi
else
  echo "File $file not found on PR branch"
fi

Length of output: 200

🏁 Script executed:

#!/bin/bash
# Check if we're in the right repository
git remote -v

# Try with a broader search
find . -type f -name "*.php" | xargs grep -l "TextArray" | head -1 | xargs cat

# Look at the commit message
git log -1 --pretty=full

Length of output: 2211

🏁 Script executed:

#!/bin/bash
# Get the PR details including files changed
gh pr view 157 --repo martin-georgiev/postgresql-for-doctrine --json files,body,title,commits

# Get the PR diff
gh pr diff 157 --repo martin-georgiev/postgresql-for-doctrine

Length of output: 6310

@martin-georgiev After carefully examining the PR, I believe this is a correct fix that should be approved. The changes address a clear inconsistency between PostgreSQL's behavior and the current implementation.

The current implementation of TextArray is automatically converting numeric-looking strings to integers and floats when retrieving data from the database. This doesn't match PostgreSQL's actual behavior, where elements in a text[] remain as text strings regardless of their content (as confirmed by the PR author's query showing that pg_typeof(('{0.4}'::text[])[1]) returns "text").

The PR makes three key changes:

Removes the type-juggling code in DataStructure::transformPostgresTextArrayToPHPArray()
Updates tests to expect string values instead of numbers
Strengthens the tests by using assertSame() instead of assertEquals() for stricter type checking

This change correctly ensures that when users store ['a', '1', '0.1'] in a text array column, they get back exactly the same values with preserved types. This is more intuitive and matches user expectations when working with a text[] data type.

✅ Actions performed

Comments resolved.

martin-georgiev · 2025-03-13T19:17:58Z

@coderabbitai, how else can you improve this functionality?

coderabbitai · 2025-03-13T19:18:38Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find and examine the TextArray class implementation
find ./src -type f -name "*.php" | xargs grep -l "class TextArray" | xargs cat

# Look for other array type implementations that might have related functionality
find ./src -type f -name "*Array*.php" | xargs cat

# Check any base classes or interfaces for array types
find ./src -type f -name "*.php" | xargs grep -l "abstract class.*Array" || echo "No abstract Array classes found"
find ./src -type f -name "*.php" | xargs grep -l "interface.*Array" || echo "No Array interfaces found"

# Look for any utility classes that help with array conversions
find ./src -type f -name "*.php" | xargs grep -l "DataStructure" | xargs cat

Length of output: 28147

@martin-georgiev After examining the code, here are several ways to improve the TextArray functionality:

Refactor TextArray to extend BaseArray: Currently, TextArray extends BaseType while other array types extend BaseArray. This creates inconsistency and duplicates logic. Refactoring TextArray to extend BaseArray would follow the pattern established by other array types.
Support multi-dimensional text arrays: Both DataStructure utility methods explicitly throw exceptions for multi-dimensional arrays. Adding support for nested arrays would enhance functionality.
Add better validation: Improve input validation and provide more specific error messages when invalid data is encountered.
Add explicit type handling options: Provide configuration options for whether numeric-looking strings should be preserved as strings or converted to numbers, making behavior configurable based on use case.
Improve performance: The current string parsing approach could be optimized for handling large arrays.
Add type hints for PHP 8: Update method signatures with better type hinting for improved static analysis.
Enhance documentation: Add more detailed PHPDoc comments explaining behavior, especially regarding type preservation.

Would you like me to elaborate on any of these improvements or provide a code example for implementing one of them?

martin-georgiev · 2025-03-30T12:58:15Z

@coderabbitai will this still be an issue with the release of #304?

coderabbitai · 2025-03-30T12:59:16Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR #304 details
gh pr view 304 --repo martin-georgiev/postgresql-for-doctrine --json title,body,files,commits,state

# Get PR #157 details for comparison
gh pr view 157 --repo martin-georgiev/postgresql-for-doctrine --json title,body,files

# Check if PR #304 affects TextArray.php
gh pr diff 304 --repo martin-georgiev/postgresql-for-doctrine | grep -A 10 -B 10 "TextArray"

# Check if PR #304 modifies the DataStructure class
gh pr diff 304 --repo martin-georgiev/postgresql-for-doctrine | grep -A 10 -B 10 "DataStructure"

Length of output: 26234

@martin-georgiev After analyzing PR #304, I can confirm that it definitely addresses the issue fixed in PR #157 and goes much further in solving it.

PR #157 specifically fixed the issue where numeric-looking strings in TextArray were being converted to actual numbers by removing the type juggling code in the DataStructure class.

PR #304 completely replaces the DataStructure class with the new ArrayDataTransformer implementation that has comprehensive type preservation for all kinds of values:

The PR description explicitly states: "Numeric strings stay as strings: '1.0' → '1.0'"
The implementation includes a specific test called preserves_numeric_string_types() that verifies strings like '1', '1.0', '1.00' remain as strings through the transformation process
It uses assertSame() instead of assertEquals() for stricter type checking in tests
It handles many additional edge cases like scientific notation, very large numbers, and maintains proper type preservation for all numeric formats

So not only is the issue from PR #157 fixed in PR #304, but the new implementation provides a much more robust and comprehensive solution for type preservation across all array types.

Code Analysis Tools

Tool Analysis Results

CodeRabbit ran static analysis tools on your files and found the following:

No specific issues were found in your files. You can proceed with your changes.

coderabbitai · 2025-03-30T12:59:28Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get information about PR #304
echo "PR #304 Details:"
gh pr view 304 --repo martin-georgiev/postgresql-for-doctrine --json number,title,body,state,author

# Get files changed in PR #304
echo -e "\nFiles changed in PR #304:"
gh pr view 304 --repo martin-georgiev/postgresql-for-doctrine --json files --jq '.files[].path'

# Check if PR #304 touches TextArray.php or related functionality
echo -e "\nCheck if PR #304 affects array type implementations:"
gh pr diff 304 --repo martin-georgiev/postgresql-for-doctrine | grep -A 10 -B 10 "Array" || echo "No direct mentions of Array in the diff"

# Let's also look at what PR #157 addressed
echo -e "\nPR #157 addressed:"
gh pr view 157 --repo martin-georgiev/postgresql-for-doctrine --json title,body

Length of output: 23690

@martin-georgiev Based on my analysis of PR #304, the issue addressed in PR #157 will be completely resolved with the release of PR #304. In fact, PR #304 provides a more comprehensive solution to the type preservation problem.

Looking at the changes in PR #304:

It completely replaces the old DataStructure class with a new ArrayDataTransformer class that properly preserves types during conversion
The problematic code that converted numeric-looking strings to integers/floats has been removed

It includes specific tests that verify numeric string types are preserved:

public function preserves_numeric_string_types(): void
{
    $input = ['1', '1.0', '1.00', 1, 1.01];
    // ...
    self::assertSame([
        '1',
        '1.0',
        '1.00',
        1,
        1.01,
    ], $output);
}

PR #304 is even more comprehensive than PR #157 as it also handles other numeric formats like scientific notation and very large numbers, plus it adds better exception handling.

So no, this won't still be an issue with the release of PR #304 - it's fully addressed and improved upon.

Code Analysis Tools

Tool Analysis Results

CodeRabbit ran static analysis tools on your files and found the following:

No specific issues were found in your files. You can proceed with your changes.

martin-georgiev · 2025-03-30T13:03:13Z

Closing in favour of #304

adrienbrault and others added 3 commits September 12, 2023 11:28

fix: parse text[] as array<string>

2189954

Merge branch 'main' into fix-text-array-parsing

74b3669

Merge branch 'main' into fix-text-array-parsing

599c717

greptile-apps bot reviewed Dec 27, 2024

View reviewed changes

martin-georgiev reviewed Dec 27, 2024

View reviewed changes

martin-georgiev closed this Mar 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: parse text[] as array<string> #157

fix: parse text[] as array<string> #157

adrienbrault commented Sep 12, 2023

greptile-apps bot left a comment

greptile-apps bot Dec 27, 2024

greptile-apps bot Dec 27, 2024

martin-georgiev Dec 27, 2024

martin-georgiev Dec 27, 2024

martin-georgiev commented Mar 13, 2025

coderabbitai bot commented Mar 13, 2025

martin-georgiev commented Mar 13, 2025

coderabbitai bot commented Mar 13, 2025

martin-georgiev commented Mar 30, 2025 •

edited

Loading

coderabbitai bot commented Mar 30, 2025

Tool Analysis Results

coderabbitai bot commented Mar 30, 2025

Tool Analysis Results

martin-georgiev commented Mar 30, 2025

fix: parse text[] as array<string> #157

fix: parse text[] as array<string> #157

Conversation

adrienbrault commented Sep 12, 2023

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Dec 27, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 27, 2024

Choose a reason for hiding this comment

martin-georgiev Dec 27, 2024

Choose a reason for hiding this comment

martin-georgiev Dec 27, 2024

Choose a reason for hiding this comment

martin-georgiev commented Mar 13, 2025

coderabbitai bot commented Mar 13, 2025

martin-georgiev commented Mar 13, 2025

coderabbitai bot commented Mar 13, 2025

martin-georgiev commented Mar 30, 2025 • edited Loading

coderabbitai bot commented Mar 30, 2025

Tool Analysis Results

coderabbitai bot commented Mar 30, 2025

Tool Analysis Results

martin-georgiev commented Mar 30, 2025

martin-georgiev commented Mar 30, 2025 •

edited

Loading