feat: add support for REGEXP_COUNT(), REGEXP_INSTR() and REGEXP_SUBSTR() and extend support for REGEXP_REPLACE()
#352
Conversation
…SUBSTR()` and extend support for `REGEXP_REPLACE()`
WalkthroughThis update introduces new Doctrine DQL function classes for PostgreSQL regular expression operations: Changes
Sequence Diagram(s)sequenceDiagram
participant DQL as Doctrine DQL Query
participant Func as Regexp* Function Class
participant SQL as PostgreSQL
DQL->>Func: Parse function call with arguments
Func->>Func: Validate argument count and pattern
Func->>SQL: Generate SQL with appropriate REGEXP_* function and arguments
SQL-->>Func: Return SQL result
Func-->>DQL: Return result to Doctrine
Possibly related PRs
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ 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:
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 (4)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpInstrTest.php (1)
26-36: Test SQL statements align with PostgreSQL function usage, but consider adding additional test cases.The tests cover various parameter combinations, from basic usage to the full 6-parameter version. However, PostgreSQL documentation describes more complex use cases for REGEXP_INSTR that could be valuable to test:
- Testing return option 1 (returns end position of match instead of start position)
- Testing with case-sensitive vs case-insensitive flag differences
- Testing matches that span multiple characters
Consider adding additional test cases that demonstrate more complex regex patterns and the effect of the return_option parameter, such as:
protected function getExpectedSqlStatements(): array { return [ 'finds position of pattern' => "SELECT regexp_instr(c0_.text1, 'c(.)(...)') AS sclr_0 FROM ContainsTexts c0_", 'finds position of digits' => "SELECT regexp_instr(c0_.text1, '\\d+') AS sclr_0 FROM ContainsTexts c0_", 'with start position' => "SELECT regexp_instr(c0_.text1, '\\d+', 1) AS sclr_0 FROM ContainsTexts c0_", 'with start and occurrence' => "SELECT regexp_instr(c0_.text1, '\\d+', 1, 2) AS sclr_0 FROM ContainsTexts c0_", 'with start, occurrence and return_option' => "SELECT regexp_instr(c0_.text1, '\\d+', 1, 2, 0) AS sclr_0 FROM ContainsTexts c0_", 'with all parameters' => "SELECT regexp_instr(c0_.text1, '\\d+', 1, 2, 0, 'i') AS sclr_0 FROM ContainsTexts c0_", + 'with return_option 1' => "SELECT regexp_instr(c0_.text1, '\\d+', 1, 1, 1) AS sclr_0 FROM ContainsTexts c0_", + 'case sensitivity test' => "SELECT regexp_instr(c0_.text1, '[A-Z]+') AS sclr_0 FROM ContainsTexts c0_", + 'case insensitivity test' => "SELECT regexp_instr(c0_.text1, '[A-Z]+', 1, 1, 0, 'i') AS sclr_0 FROM ContainsTexts c0_", ]; }Make corresponding additions to the
getDqlStatements()method as well.src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpCount.php (1)
17-17: Consider expanding the documentation exampleWhile the current example is helpful, consider adding examples that demonstrate all possible parameter variations (3 and 4 parameters) to better illustrate the full functionality.
- * @example Using it in DQL: "SELECT REGEXP_COUNT(e.text, '\d\d\d') FROM Entity e" + * @example Using it in DQL: + * Basic usage: "SELECT REGEXP_COUNT(e.text, '\d\d\d') FROM Entity e" + * With position: "SELECT REGEXP_COUNT(e.text, '\d\d\d', 1) FROM Entity e" + * With all parameters: "SELECT REGEXP_COUNT(e.text, '\d\d\d', 1, 'i') FROM Entity e"tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpCountTest.php (2)
26-35: Test cases duplicate "with flags" and "with all parameters"The test cases for "with flags" and "with all parameters" are identical in both expected SQL and DQL. Consider either removing one of these duplicates or replacing it with a different test case to improve test coverage.
- 'with all parameters' => "SELECT regexp_count(c0_.text1, '\\d\\d\\d', 1, 'i') AS sclr_0 FROM ContainsTexts c0_", + 'with different flags' => "SELECT regexp_count(c0_.text1, '\\d\\d\\d', 1, 'g') AS sclr_0 FROM ContainsTexts c0_",
37-46: Corresponding DQL statements also have duplicationThe DQL statements for "with flags" and "with all parameters" are identical. This should be updated to match any changes made to the expected SQL statements.
- 'with all parameters' => \sprintf("SELECT REGEXP_COUNT(e.text1, '\\d\\d\\d', 1, 'i') FROM %s e", ContainsTexts::class), + 'with different flags' => \sprintf("SELECT REGEXP_COUNT(e.text1, '\\d\\d\\d', 1, 'g') FROM %s e", ContainsTexts::class),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpCount.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpInstr.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplace.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpSubstr.php(1 hunks)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpCountTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpInstrTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplaceTest.php(2 hunks)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpSubstrTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/**/*.php`: Use the PostgreSQL official documentation to verify that tests include comprehensive use cases and example SQL que...
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/**/*.php: Use the PostgreSQL official documentation to verify that tests include comprehensive use cases and example SQL queries for the tested SQL functions and operators.
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpCountTest.phptests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpInstrTest.phptests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplaceTest.phptests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpSubstrTest.php
🧠 Learnings (1)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplaceTest.php (2)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase, and therefore don't need an explicit import.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), not PHPUnit's TestCase, and therefore don't need an explicit import statement.
🧬 Code Graph Analysis (2)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplaceTest.php (5)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php (1)
BaseVariadicFunction(20-135)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidArgumentForVariadicFunctionException.php (1)
InvalidArgumentForVariadicFunctionException(7-56)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplace.php (1)
RegexpReplace(15-41)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunctionTestCase.php (1)
BaseVariadicFunctionTestCase(14-42)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php (1)
buildEntityManager(116-119)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpSubstrTest.php (4)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php (1)
BaseVariadicFunction(20-135)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidArgumentForVariadicFunctionException.php (1)
InvalidArgumentForVariadicFunctionException(7-56)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpSubstr.php (1)
RegexpSubstr(20-46)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php (1)
buildEntityManager(116-119)
🔇 Additional comments (15)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpSubstr.php (2)
7-19: Well-documented PostgreSQL function implementation.The documentation properly explains the PostgreSQL
REGEXP_SUBSTR()function with a link to the official documentation and includes a helpful DQL example. The function description accurately reflects PostgreSQL's behavior of returning a substring matching the regular expression pattern or NULL if no match is found.
20-46: Implementation correctly maps to PostgreSQL's function signature.The class properly extends
BaseVariadicFunctionand defines the correct argument patterns for PostgreSQL'sREGEXP_SUBSTR(). The implementation supports all 6 possible parameters described in the PostgreSQL documentation:
- source string (required)
- regex pattern (required)
- start position (optional)
- occurrence (optional)
- flags (optional)
- subexpression (optional)
The argument types match PostgreSQL's requirements, with strings represented as StringPrimary and numeric values as ArithmeticPrimary.
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpSubstrTest.php (4)
12-46: Test cases validate SQL generation for all valid argument combinations.The tests properly verify SQL generation for the
REGEXP_SUBSTR()function with various argument combinations, covering basic usage, digit extraction, and all optional parameters. The DQL to SQL transformation is tested correctly.According to PostgreSQL documentation, the test cases align with expected usage patterns:
- Basic usage with only string and pattern parameters
- Extraction of digit patterns with '\d+'
- Usage with position, occurrence, flags, and subexpression parameters
However, consider adding a few more test cases to cover additional PostgreSQL regex features:
protected function getExpectedSqlStatements(): array { return [ 'extracts pattern' => "SELECT regexp_substr(c0_.text1, 'c(.)(...)') AS sclr_0 FROM ContainsTexts c0_", 'extracts digits' => "SELECT regexp_substr(c0_.text1, '\\d+') AS sclr_0 FROM ContainsTexts c0_", 'extracts digits with start and N parameters' => "SELECT regexp_substr(c0_.text1, '\\d+', 1, 4) AS sclr_0 FROM ContainsTexts c0_", 'extracts digits with start and N parameters and flags' => "SELECT regexp_substr(c0_.text1, '\\d+', 1, 4, 'i') AS sclr_0 FROM ContainsTexts c0_", 'extracts digits with start, N and subexpr parameters and flags' => "SELECT regexp_substr(c0_.text1, '\\d+', 1, 4, 'i', 3) AS sclr_0 FROM ContainsTexts c0_", + 'uses multiple flags' => "SELECT regexp_substr(c0_.text1, '\\w+', 1, 1, 'inx') AS sclr_0 FROM ContainsTexts c0_", + 'uses character class alternation' => "SELECT regexp_substr(c0_.text1, '[a-z]+|[0-9]+') AS sclr_0 FROM ContainsTexts c0_", ]; }With corresponding additions to
getDqlStatements().
48-55: Test properly validates minimum argument count.The test correctly verifies that an exception is thrown when fewer than 2 arguments are provided to the
REGEXP_SUBSTR()function, which aligns with PostgreSQL's requirement for at least a string and pattern parameter.
57-64: Test properly validates maximum argument count.The test correctly verifies that an exception is thrown when more than 6 arguments are provided to the
REGEXP_SUBSTR()function, which aligns with PostgreSQL's maximum of 6 parameters (string, pattern, start, occurrence, flags, subexpression).
7-11: Appropriate imports included for the test class.All necessary classes are imported, including the exception class for validation tests and the entity fixture class used in the DQL queries.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpInstr.php (2)
7-19: Documentation looks accurate and comprehensive.The docblock properly describes the PostgreSQL REGEXP_INSTR() function with a link to the official documentation. The examples show proper usage in DQL, which provides clear guidance for users.
20-45: Implementation aligns with PostgreSQL's REGEXP_INSTR() function.The implementation is well-structured:
- Extends BaseVariadicFunction appropriately
- Sets correct min (2) and max (6) argument counts
- Defines node mapping patterns for various valid argument combinations
- Properly sets function name as 'regexp_instr'
The node mapping patterns accurately reflect the PostgreSQL function signature, supporting the various parameter combinations detailed in the PostgreSQL documentation.
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpInstrTest.php (2)
50-57: Test for too few arguments looks good.The test properly verifies exception handling for insufficient arguments, checking both the exception type and the error message.
59-66: Test for too many arguments looks good.The test correctly validates exception handling when providing more than the maximum allowed arguments, checking both the exception type and the error message.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplace.php (1)
32-40: 🛠️ Refactor suggestion
⚠️ Potential issue
getMin/MaxArgumentCount()should reflect the PostgreSQL limitsBecause PostgreSQL allows at most 4 parameters,
getMaxArgumentCount()ought to return 4.
Keeping it at 6 masks invalid user input until runtime when PostgreSQL rejects the statement.- protected function getMaxArgumentCount(): int - { - return 6; - } + protected function getMaxArgumentCount(): int + { + // PostgreSQL 15/16: 3 mandatory + 1 optional = 4 + return 4; + }Likely an incorrect or invalid review comment.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpCount.php (2)
1-43: Implementation correctly aligns with PostgreSQL's REGEXP_COUNT functionThe class implementation follows the proper structure for extending Doctrine with custom DQL functions. It correctly:
- Extends BaseVariadicFunction
- Defines minimum (2) and maximum (4) argument constraints
- Specifies supported argument patterns
- Includes appropriate documentation with PostgreSQL reference
23-26: Validate node mapping pattern matches PostgreSQL specificationThe node mapping patterns correctly handle:
- Basic form: source string, pattern
- Extended form: source string, pattern, position, flags
This matches the PostgreSQL documentation for REGEXP_COUNT function parameters.
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpCountTest.php (2)
48-55: Test coverage for invalid arguments is appropriateThe test correctly validates the function's behavior when provided with too few arguments, checking both the exception type and message.
57-64: Test coverage for too many arguments is appropriateThe test correctly validates the function's behavior when provided with too many arguments, checking both the exception type and message.
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpInstrTest.php
Show resolved
Hide resolved
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplaceTest.php
Show resolved
Hide resolved
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplaceTest.php
Show resolved
Hide resolved
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpCountTest.php
Show resolved
Hide resolved
|
@coderabbitai, you're wrong about |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🌐 Web query: 💡 Result: PostgreSQL
|
| Argument | Description | Required | Notes |
|---|---|---|---|
| source | String to search | Yes | Can be a column or literal |
| pattern | POSIX regex pattern | Yes | Use double backslashes (\\) for escapes |
| replacement | Replacement string | Yes | Can use backreferences \1, \2, ... |
| flags | Optional, controls match (e.g., 'g', 'i', 'n') |
No | Default: replaces first occurrence only |
| start (>=16) | Position to begin search | No | Default is 1 (first character) |
| N (>=16) | Which occurrence to replace (0 = all, use 'g' as well) |
No | Must specify start if specifying N |
Usage Tips
- Test your regular expressions with sample data before applying to large tables.
- Remember that complex regex operations can impact performance on large datasets.
- Properly escape backslashes in both the pattern and replacement as per SQL string rules[1][5].
Further Reading
- Official PostgreSQL Docs: Pattern Matching Functions[1]
- String Functions Overview[4]
You can use REGEXP_REPLACE for a wide range of text transformation tasks, including formatting, data cleaning, and normalization, making it a versatile tool in PostgreSQL[2][3][6].
Citations:
- 1: https://www.postgresql.org/docs/current/functions-matching.html
- 2: https://neon.tech/postgresql/postgresql-string-functions/regexp_replace
- 3: https://neon.tech/docs/functions/regexp_replace
- 4: https://www.postgresql.org/docs/current/functions-string.html
- 5: https://www.datacamp.com/doc/postgresql/regexp_replace
- 6: https://www.pgtutorial.com/postgresql-string-functions/postgresql-regexp_replace/
- 7: https://docs.aws.amazon.com/redshift/latest/dg/REGEXP_REPLACE.html
- 8: https://www.geeksforgeeks.org/postgresql-regexp_replace-function/
- 9: https://docs.oracle.com/en/database/oracle/oracle-database/21/sqlrf/REGEXP_REPLACE.html
- 10: https://postgrespro.com/docs/postgresql/10/functions-string
@martin-georgiev Thank you for pointing out the error! I've reviewed the PostgreSQL documentation on REGEXP_REPLACE and here's the correct information:
The REGEXP_REPLACE function in PostgreSQL accepts between 3 and 6 arguments:
REGEXP_REPLACE(source, pattern, replacement [, flags [, start [, N]]])
Where:
- The first 3 parameters are required (source string, pattern, replacement)
flagsis optional (e.g., 'g' for global replacement, 'i' for case-insensitive)start(available in PostgreSQL 16+) is the starting position for searchN(available in PostgreSQL 16+) specifies which occurrence to replace
This aligns with your implementation in the PR where you changed RegexpReplace to extend BaseVariadicFunction and set minimum argument count to 3 and maximum to 6.
I apologize for the incorrect information in my summary.
✏️ Learnings added
Learnt from: martin-georgiev
URL: https://github.com/martin-georgiev/postgresql-for-doctrine/pull/352
Timestamp: 2025-04-20T22:41:09.046Z
Learning: PostgreSQL's REGEXP_REPLACE function accepts between 3 and 6 arguments: REGEXP_REPLACE(source, pattern, replacement [, flags [, start [, N]]]). The first 3 parameters are required, and the rest are optional. The start and N parameters are available in PostgreSQL 16 and later.
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.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@martin-georgiev Thank you for the correction. You're right to point out the error. After reviewing the official PostgreSQL documentation, I understand now that
This aligns with the implementation in your PR. I appreciate you pointing out this inaccuracy. 🌐 Web query: 💡 Result: PostgreSQL
|
| Example Query | Description | Result |
|---|---|---|
SELECT regexp_replace('apple pie', 'apple', 'banana'); |
Replace "apple" with "banana" | "banana pie" |
SELECT regexp_replace('Hello World', 'world', 'there', 'i'); |
Case-insensitive replace | "Hello there" |
SELECT regexp_replace('123-456-7890', '(\\d{3})-(\\d{3})-(\\d{4})', '(\\1) \\2-\\3'); |
Format phone number using groups | "(123) 456-7890" |
SELECT regexp_replace('foo bar baz', '\\s+', '_', 'g'); |
Replace all whitespace with underscores | "foo_bar_baz" |
Notes
- Be careful to double backslashes in patterns and replacement strings when writing queries in SQL, due to escape sequence handling[5].
- In older PostgreSQL versions (prior to v16), only the four-argument form is available, so you can't specify the start position or the Nth match to replace[2][4].
- If a match is not found, the function returns the original string unchanged.
Reference
For the most up-to-date documentation, refer to PostgreSQL's official documentation: 9.7. Pattern Matching[1][4].
Excerpt from the Official Docs:
The regexp_replace function provides substitution of new text for substrings that match POSIX regular expression patterns. It has the syntax regexp_replace(source, pattern, replacement [, start [, N ]] [, flags ]). (Notice that N cannot be specified unless start is, but flags can be given in any case.)[1]
This sums up the function’s use and options as described in the official PostgreSQL documentation[1][4].
Citations:
- 1: https://www.postgresql.org/docs/current/functions-matching.html
- 2: https://neon.tech/docs/functions/regexp_replace
- 3: https://neon.tech/postgresql/postgresql-string-functions/regexp_replace
- 4: https://www.postgresql.org/docs/current/functions-string.html
- 5: https://www.datacamp.com/doc/postgresql/regexp_replace
- 6: https://postgresql.kr/docs/8.3/functions-matching.html
- 7: https://postgrespro.com/list/thread-id/1427665
- 8: https://www.pgtutorial.com/postgresql-string-functions/postgresql-regexp_replace/
- 9: https://www.postgresql.org/docs/9.1/functions-string.html
- 10: https://www.postgresql.org/docs/current/functions.html
@martin-georgiev Thank you for the correction. You're absolutely right about regexp_replace. According to the official PostgreSQL documentation:
The full syntax is:
regexp_replace(source, pattern, replacement [, start [, N]] [, flags])Where:
source,pattern, andreplacementare requiredstartis the position to begin searching (optional, PostgreSQL 16+)Nis which occurrence to replace (optional, PostgreSQL 16+)flagsare modifiers like 'g' for global replacement (optional)
This means it can accept between 3 and 6 parameters, supporting the more advanced functionality you're implementing in your PR.
I appreciate you pointing me to the correct documentation.
✏️ Learnings added
Learnt from: martin-georgiev
URL: https://github.com/martin-georgiev/postgresql-for-doctrine/pull/352
Timestamp: 2025-04-20T22:41:39.223Z
Learning: PostgreSQL's regexp_replace function has the full syntax: regexp_replace(source, pattern, replacement [, start [, N]] [, flags]). In newer PostgreSQL versions, it supports between 3 and 6 parameters, with the additional parameters allowing for specifying the start position and which occurrence to replace.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplaceTest.php (1)
41-46: DQL statements match expected SQL output.The DQL statements properly correspond to the expected SQL statements defined earlier in the class, testing all the argument variations supported by the RegexpReplace function up to PostgreSQL 16.
However, same PostgreSQL version compatibility concern applies to test cases using 5 and 6 arguments.
🧹 Nitpick comments (1)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplaceTest.php (1)
28-36: Test coverage could be enhanced with additional PostgreSQL documentation examples.While the test covers the different argument variations, it would be beneficial to include more real-world examples from the PostgreSQL documentation to demonstrate the functionality of regexp_replace.
Consider adding test cases that demonstrate:
- Basic replacement with backreferences (e.g.,
regexp_replace('Thomas', '.[mN]a.', 'M'))- Case-insensitive replacement with the 'i' flag
- Global replacement with the 'g' flag
- Combination of flags
These examples would better align with the PostgreSQL documentation examples and provide more comprehensive test coverage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpCount.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpInstr.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplace.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpSubstr.php(1 hunks)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpCountTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpInstrTest.php(1 hunks)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplaceTest.php(2 hunks)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpSubstrTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpInstr.php
- src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpCount.php
- src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplace.php
- tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpInstrTest.php
- src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpSubstr.php
- tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpCountTest.php
- tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpSubstrTest.php
🧰 Additional context used
📓 Path-based instructions (1)
`tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/**/*.php`: Use the PostgreSQL official documentation to verify that tests include comprehensive use cases and example SQL que...
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/**/*.php: Use the PostgreSQL official documentation to verify that tests include comprehensive use cases and example SQL queries for the tested SQL functions and operators.
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplaceTest.php
🧠 Learnings (1)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplaceTest.php (2)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase, and therefore don't need an explicit import.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), not PHPUnit's TestCase, and therefore don't need an explicit import statement.
🧬 Code Graph Analysis (1)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplaceTest.php (5)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php (1)
BaseVariadicFunction(20-135)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidArgumentForVariadicFunctionException.php (1)
InvalidArgumentForVariadicFunctionException(7-56)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplace.php (1)
RegexpReplace(19-47)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunctionTestCase.php (1)
BaseVariadicFunctionTestCase(14-42)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php (1)
buildEntityManager(116-119)
🔇 Additional comments (4)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplaceTest.php (4)
8-10: Appropriate imports for new function behavior.The added imports properly support the newly refactored RegexpReplace function, which now extends BaseVariadicFunction and needs to handle InvalidArgumentForVariadicFunctionException for argument validation.
12-17: Good implementation of BaseVariadicFunctionTestCase requirements.The test class now correctly extends BaseVariadicFunctionTestCase and implements the required createFixture() method that instantiates the RegexpReplace function with the appropriate name.
50-57: Good validation test for minimum argument count.This test properly verifies that the RegexpReplace function throws an appropriate exception with the correct error message when fewer than the minimum required arguments (3) are provided.
59-66: Good validation test for maximum argument count.This test properly verifies that the RegexpReplace function throws an appropriate exception with the correct error message when more than the maximum allowed arguments (6) are provided.
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RegexpReplaceTest.php
Show resolved
Hide resolved
…SUBSTR()` and extend support for `REGEXP_REPLACE()` (martin-georgiev#352)
Summary by CodeRabbit
REGEXP_COUNT,REGEXP_INSTR,REGEXP_SUBSTR, and enhancedREGEXP_REPLACEwith variadic argument support.REGEXP_REPLACEfunction to support a variable number of arguments.REGEXP_COUNT,REGEXP_INSTR,REGEXP_REPLACE, andREGEXP_SUBSTRfunctions, including validation for correct argument counts and error handling.