feat: add support for xmlagg()
#318
Conversation
WalkthroughThis pull request introduces a new PostgreSQL aggregation function, Changes
Sequence Diagram(s)sequenceDiagram
participant DQL
participant DoctrineORM
participant XML_AGG_Function
participant Database
DQL->>DoctrineORM: Submit query using XML_AGG
DoctrineORM->>XML_AGG_Function: Parse XML_AGG function call
XML_AGG_Function-->>DoctrineORM: Return SQL fragment for XMLAGG
DoctrineORM->>Database: Execute query with generated SQL
Possibly related PRs
Poem
✨ 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: 1
🧹 Nitpick comments (2)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAgg.php (1)
47-55: SQL generation handles ordering elegantly.
The combination of expression dispatching and optional ordering is well-structured. Consider adding additional safety checks if you anticipate expressions that might be empty or invalid, though Doctrine usually guards against that scenario.tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php (1)
29-36: Consider adding advanced usage tests.
PostgreSQLXMLAGGcan wrap XML elements or handle concatenated XML content. You might include tests involving multiple expressions or XML-specific syntax (e.g.,xmlelement(name foo, e.text1)).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/AVAILABLE-FUNCTIONS-AND-OPERATORS.md(1 hunks)docs/INTEGRATING-WITH-DOCTRINE.md(1 hunks)docs/INTEGRATING-WITH-LARAVEL.md(1 hunks)docs/INTEGRATING-WITH-SYMFONY.md(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAgg.php(1 hunks)tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.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/XmlAggTest.php
🧠 Learnings (1)
docs/INTEGRATING-WITH-SYMFONY.md (1)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#277
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/JsonExistsTest.php:10-0
Timestamp: 2025-03-26T16:52:34.228Z
Learning: The implementation of JSON_EXISTS in this library supports only the basic form with two parameters (target_json, path_text) and doesn't support additional features like strict mode (third boolean parameter), ON ERROR clause, or other advanced PostgreSQL JSON_EXISTS capabilities due to DQL limitations.
🧬 Code Definitions (1)
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php (1)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAgg.php (1)
XmlAgg(22-56)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: PHP 8.1 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: sloth
🔇 Additional comments (8)
docs/AVAILABLE-FUNCTIONS-AND-OPERATORS.md (1)
103-103: Documentation entry looks good.
This new row forxmlaggis consistent with the rest of the table and aligns with PostgreSQL function naming conventions.src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAgg.php (3)
14-21: DocBlock includes practical references.
Thank you for referencing the PostgreSQL 17 docs and highlighting usage details. This helps developers quickly understand the function’s purpose.
22-30: Sensible approach in defining the function prototype.
Your use ofsetFunctionPrototype('xmlagg(%s%s)')with node mapping forStringPrimarymatches the typical pattern for handling user-provided expressions.
32-46: Parser logic is comprehensive.
Properly matching and parsing identifiers, parentheses, and the optional order-by clause ensures robust handling of theXML_AGGsyntax.tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php (1)
19-27: Thorough coverage of basic and ordered usage.
You have done a commendable job covering both basic usage and ordering scenarios. This ensures core functionalities ofXML_AGGare tested.docs/INTEGRATING-WITH-DOCTRINE.md (1)
35-43: Aggregation Functions Registration UpdatedThe reorganization of the custom string functions under the "aggregation functions" section is clear and logically grouped. The new
XML_AGGfunction is registered alongside the other aggregation functions, and its fully qualified class name appears correct. Please ensure that the corresponding documentation in AVAILABLE-FUNCTIONS-AND-OPERATORS.md is updated accordingly.docs/INTEGRATING-WITH-SYMFONY.md (1)
169-177: Consistent Addition of XML_AGG in Symfony ConfigThe YAML configuration now clearly separates the aggregation functions into their own block. The new
XML_AGGentry is correctly registered alongsideARRAY_AGG,JSON_AGG, and the others. This reorganization aligns well with the changes made in the Doctrine integration documentation.docs/INTEGRATING-WITH-LARAVEL.md (1)
174-182: Laravel Integration: Aggregation Functions ReorganizedThe custom string functions mapping in the Laravel configuration has been reorganized to include a dedicated "aggregation functions" section. The addition of the
XML_AGGfunction is consistent with the changes in the Doctrine and Symfony integrations. Ensure that any related Laravel integration tests are updated to recognize and validate the new function mapping if applicable.
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions; | ||
|
|
||
| use Fixtures\MartinGeorgiev\Doctrine\Entity\ContainsTexts; | ||
| use MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\XmlAgg; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Missing TestCase import.
You extend TestCase but it's not explicitly imported here. Ensure you have the necessary import (e.g., use PHPUnit\Framework\TestCase;) or are otherwise inheriting correctly.
🏁 Script executed:
#!/bin/bash
# Verify that `TestCase` is properly imported or declared in this namespace
rg -A 2 $'class XmlAggTest' tests/Length of output: 329
Attention: Missing TestCase Import
The file tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php extends TestCase, but it currently does not include an explicit import for it. To ensure clarity and avoid potential issues with autoloading, please add the following import at the top of the file (below the namespace declaration):
use PHPUnit\Framework\TestCase;- Location:
tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php - Action: Add the import statement for
PHPUnit\Framework\TestCase.
There was a problem hiding this comment.
Are you ignoring that all tests in this namespace rely on Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase?
There was a problem hiding this comment.
🧩 Analysis chain
@martin-georgiev You're absolutely right, and I apologize for the oversight! I see that tests in this namespace are extending the custom Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase class rather than PHPUnit's TestCase. Since the TestCase class is in the same namespace, no import statement is needed. Thank you for the correction.
🏁 Script executed:
#!/bin/bash
# Check if a custom TestCase class exists in the namespace
echo "Checking for a custom TestCase class in the tests directory:"
find tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ -name "TestCase.php"
echo -e "\nExamining how other test files in the same namespace extend TestCase:"
grep -r "class .* extends TestCase" tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ --include="*.php" | grep -v TestCase.php | head -n 3
Length of output: 968
✏️ Learnings added
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:15.897Z
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:15.897Z
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.
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.
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.
Summary by CodeRabbit
New Features
Documentation
Tests