(feat) Add support for DATE_TRUNC #493
Open
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
|
Warning Rate limit exceeded@janklan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
WalkthroughAdds a new PostgreSQL-style DQL function DATE_TRUNC (variadic, 2–3 args) with truncation-field and timezone validation, a dedicated InvalidTruncFieldException, docs updates showing registration/examples, and unit tests covering SQL generation and validation paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/USE-CASES-AND-EXAMPLES.md (1)
100-111: Fix DATE_TRUNC example with four arguments and invalid timezoneThe second DATE_TRUNC example,
SELECT DATE_TRUNC('day', e.timestampWithTz, '2 hours', 'UTC') FROM Entity edoes not match the implemented function:
DateTrunconly allows 2 or 3 arguments (field,source, optionaltime_zone).- If used as written, it will trigger
InvalidArgumentForVariadicFunctionExceptiondue to too many arguments.'2 hours'is not a valid timezone string for the current timezone validation.Consider updating to a 3‑argument example with a valid timezone, e.g.:
-SELECT DATE_TRUNC('day', e.timestampWithTz, '2 hours', 'UTC') FROM Entity e +SELECT DATE_TRUNC('day', e.timestampWithTz, 'UTC') FROM Entity eThis keeps the “timezone parameter is optional” wording accurate and matches the actual API.
🧹 Nitpick comments (2)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php (1)
15-34: Exception wiring is fine; consider renaming parameter for clarity
InvalidTruncFieldExceptioncorrectly provides:
forNonLiteralNode()for non-literal field arguments.forInvalidField()including the list of allowedDateTrunc::FIELDS.Only minor nit: the first parameter of
forInvalidField()is named$timezonebut represents the truncation field value. Renaming to$fieldwould better reflect its purpose without changing behavior:- public static function forInvalidField(string $timezone, string $functionName): self + public static function forInvalidField(string $field, string $functionName): self { return new self(\sprintf( 'Invalid date_trunc field value "%s" provided for %s. Must be one of: %s.', - $timezone, + $field, $functionName, \implode(', ', DateTrunc::FIELDS) )); }tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTruncTest.php (1)
16-143: DateTrunc tests are solid; consider adding a non‑literal field caseThe test suite nicely covers:
- DQL → SQL translation for 2‑ and 3‑argument usages and WHERE clause.
- Argument count validation via
InvalidArgumentForVariadicFunctionException.- Field value validation against
DateTrunc::FIELDS.- Timezone validation via
InvalidTimezoneException.To fully exercise
InvalidTruncFieldException::forNonLiteralNode(), you could add a case where the first argument isn’t a string literal (e.g. using a field expression as the truncation unit). For example:public function test_throws_exception_for_non_literal_field(): void { $this->expectException(InvalidTruncFieldException::class); $this->expectExceptionMessage( 'The date_trunc field parameter for date_trunc must be a string literal' ); $dql = \sprintf("SELECT DATE_TRUNC(e.datetimetz1, e.datetimetz1) FROM %s e", ContainsDates::class); $this->buildEntityManager()->createQuery($dql)->getSQL(); }(adjust the exact message assertion once DateTrunc uses
InvalidTruncFieldException::forNonLiteralNode()).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/DATE-AND-RANGE-FUNCTIONS.md(1 hunks)docs/INTEGRATING-WITH-DOCTRINE.md(1 hunks)docs/INTEGRATING-WITH-LARAVEL.md(1 hunks)docs/INTEGRATING-WITH-SYMFONY.md(1 hunks)docs/USE-CASES-AND-EXAMPLES.md(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTruncTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: karpilin
Repo: martin-georgiev/postgresql-for-doctrine PR: 386
File: tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php:34-39
Timestamp: 2025-05-27T16:10:35.054Z
Learning: PostgreSQL's to_timestamp function returns a fallback result (like '2005-01-01 00:00:00+00') when given an invalid format string, rather than throwing an exception. This behavior is different from some other formatting functions and should be tested accordingly.
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 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.
📚 Learning: 2025-08-09T15:31:42.054Z
Learnt from: landure
Repo: martin-georgiev/postgresql-for-doctrine PR: 411
File: src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidLtreeForPHPException.php:11-19
Timestamp: 2025-08-09T15:31:42.054Z
Learning: In the postgresql-for-doctrine library, exception classes for invalid PHP values (Invalid*ForPHPException) follow a consistent pattern using var_export() in their create() method to format error messages. When adding new exception classes, maintain consistency with existing patterns rather than introducing new approaches, even if technically superior alternatives exist.
Applied to files:
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php
📚 Learning: 2025-03-29T03:31:17.114Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 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.
Applied to files:
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.phpsrc/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.phpdocs/INTEGRATING-WITH-SYMFONY.mddocs/DATE-AND-RANGE-FUNCTIONS.mddocs/INTEGRATING-WITH-DOCTRINE.mddocs/INTEGRATING-WITH-LARAVEL.mdtests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTruncTest.php
📚 Learning: 2025-05-23T11:11:57.951Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 383
File: tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RadiansTest.php:1-9
Timestamp: 2025-05-23T11:11:57.951Z
Learning: Tests in the `Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase directly, and therefore don't need an explicit import.
Applied to files:
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.phpsrc/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.phpdocs/INTEGRATING-WITH-SYMFONY.mddocs/DATE-AND-RANGE-FUNCTIONS.mddocs/INTEGRATING-WITH-DOCTRINE.mddocs/INTEGRATING-WITH-LARAVEL.mdtests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTruncTest.php
📚 Learning: 2025-08-25T21:29:08.737Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 0
File: :0-0
Timestamp: 2025-08-25T21:29:08.737Z
Learning: The repository should separate value object validation exceptions (extending \InvalidArgumentException) from DBAL conversion exceptions (extending ConversionException) - the Ltree implementation demonstrates this ideal pattern with InvalidLtreeException for validation and InvalidLtreeForPHPException/InvalidLtreeForDatabaseException for conversion errors.
Applied to files:
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php
📚 Learning: 2025-08-25T21:29:08.737Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 0
File: :0-0
Timestamp: 2025-08-25T21:29:08.737Z
Learning: Value object validation in this repository should use dedicated exception classes extending \InvalidArgumentException (like InvalidLtreeException, InvalidPointException) with clear documentation separating validation concerns from DBAL conversion concerns, and should include protected static assertion methods for validation logic.
Applied to files:
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php
📚 Learning: 2025-03-11T12:32:10.726Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
Applied to files:
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.phpdocs/INTEGRATING-WITH-SYMFONY.mddocs/INTEGRATING-WITH-LARAVEL.md
📚 Learning: 2025-09-01T18:48:28.508Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 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/ORM/Query/AST/Functions/DateTruncTest.php
📚 Learning: 2025-04-22T00:03:37.733Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 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/ORM/Query/AST/Functions/DateTruncTest.php
🧬 Code graph analysis (2)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php (4)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTimezoneException.php (1)
InvalidTimezoneException(14-33)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php (3)
InvalidTruncFieldException(15-35)forNonLiteralNode(17-24)forInvalidField(26-34)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php (1)
BaseVariadicFunction(20-143)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/TimezoneValidationTrait.php (1)
validateTimezone(25-36)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTruncTest.php (5)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php (1)
BaseVariadicFunction(20-143)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php (1)
DateTrunc(23-94)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidArgumentForVariadicFunctionException.php (1)
InvalidArgumentForVariadicFunctionException(7-56)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTimezoneException.php (1)
InvalidTimezoneException(14-33)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php (1)
InvalidTruncFieldException(15-35)
🔇 Additional comments (4)
docs/INTEGRATING-WITH-DOCTRINE.md (1)
158-165: DATE_TRUNC registration for Doctrine configuration looks correctThe DATE_TRUNC mapping to
MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\DateTrunc::classis consistent with the new function and aligned with existing date-specific registrations.docs/DATE-AND-RANGE-FUNCTIONS.md (1)
9-18: DATE_TRUNC entry in date/time function table is consistentThe new
date_trunc→DATE_TRUNC→DateTruncrow matches the implementation and follows the existing documentation pattern.docs/INTEGRATING-WITH-SYMFONY.md (1)
226-233: Symfony DQL registration for DATE_TRUNC is correctThe new
DATE_TRUNCentry understring_functionscorrectly points toMartinGeorgiev\Doctrine\ORM\Query\AST\Functions\DateTruncand is consistent with the rest of the date-specific configuration.docs/INTEGRATING-WITH-LARAVEL.md (1)
243-250: Laravel custom_string_functions mapping for DATE_TRUNC is consistentThe
'DATE_TRUNC' => DateTrunc::classentry fits the existing pattern for date-specific functions and matches the new DateTrunc implementation.
janklan
force-pushed
the
date-trunc
branch
2 times, most recently
from
4822f49 to
3da060e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php (1)
26-33: Clarify parameter naming in forInvalidField()
forInvalidField()currently uses$timezoneas the parameter name, even though it represents the invalid truncation field. The behavior is correct but the name is confusing when reading the code next toDateTrunc::FIELDS.Consider renaming the parameter to better reflect its meaning, for example:
- public static function forInvalidField(string $timezone, string $functionName): self + public static function forInvalidField(string $field, string $functionName): self { return new self(\sprintf( 'Invalid date_trunc field value "%s" provided for %s. Must be one of: %s.', - $timezone, + $field, $functionName, \implode(', ', DateTrunc::FIELDS) )); }If you prefer to avoid changing the parameter name for potential named-argument callers, you could keep
$timezonein the signature but assign it to a local$fieldvariable and use that in the message.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/DATE-AND-RANGE-FUNCTIONS.md(1 hunks)docs/INTEGRATING-WITH-DOCTRINE.md(1 hunks)docs/INTEGRATING-WITH-LARAVEL.md(1 hunks)docs/INTEGRATING-WITH-SYMFONY.md(1 hunks)docs/USE-CASES-AND-EXAMPLES.md(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTruncTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/INTEGRATING-WITH-LARAVEL.md
- docs/DATE-AND-RANGE-FUNCTIONS.md
- tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTruncTest.php
- docs/USE-CASES-AND-EXAMPLES.md
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: karpilin
Repo: martin-georgiev/postgresql-for-doctrine PR: 386
File: tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php:34-39
Timestamp: 2025-05-27T16:10:35.054Z
Learning: PostgreSQL's to_timestamp function returns a fallback result (like '2005-01-01 00:00:00+00') when given an invalid format string, rather than throwing an exception. This behavior is different from some other formatting functions and should be tested accordingly.
📚 Learning: 2025-05-23T11:11:57.951Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 383
File: tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RadiansTest.php:1-9
Timestamp: 2025-05-23T11:11:57.951Z
Learning: Tests in the `Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase directly, and therefore don't need an explicit import.
Applied to files:
docs/INTEGRATING-WITH-DOCTRINE.mdsrc/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.phpdocs/INTEGRATING-WITH-SYMFONY.mdsrc/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php
📚 Learning: 2025-03-29T03:31:17.114Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 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.
Applied to files:
docs/INTEGRATING-WITH-DOCTRINE.mdsrc/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.phpdocs/INTEGRATING-WITH-SYMFONY.mdsrc/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php
📚 Learning: 2025-08-09T15:31:42.054Z
Learnt from: landure
Repo: martin-georgiev/postgresql-for-doctrine PR: 411
File: src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidLtreeForPHPException.php:11-19
Timestamp: 2025-08-09T15:31:42.054Z
Learning: In the postgresql-for-doctrine library, exception classes for invalid PHP values (Invalid*ForPHPException) follow a consistent pattern using var_export() in their create() method to format error messages. When adding new exception classes, maintain consistency with existing patterns rather than introducing new approaches, even if technically superior alternatives exist.
Applied to files:
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.phpsrc/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php
📚 Learning: 2025-08-25T21:29:08.737Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 0
File: :0-0
Timestamp: 2025-08-25T21:29:08.737Z
Learning: The repository should separate value object validation exceptions (extending \InvalidArgumentException) from DBAL conversion exceptions (extending ConversionException) - the Ltree implementation demonstrates this ideal pattern with InvalidLtreeException for validation and InvalidLtreeForPHPException/InvalidLtreeForDatabaseException for conversion errors.
Applied to files:
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php
📚 Learning: 2025-08-25T21:29:08.737Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 0
File: :0-0
Timestamp: 2025-08-25T21:29:08.737Z
Learning: Value object validation in this repository should use dedicated exception classes extending \InvalidArgumentException (like InvalidLtreeException, InvalidPointException) with clear documentation separating validation concerns from DBAL conversion concerns, and should include protected static assertion methods for validation logic.
Applied to files:
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php
📚 Learning: 2025-03-11T12:32:10.726Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
Applied to files:
docs/INTEGRATING-WITH-SYMFONY.mdsrc/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php
📚 Learning: 2025-04-18T10:33:52.412Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 350
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php:78-88
Timestamp: 2025-04-18T10:33:52.412Z
Learning: In SQL generation code for the postgresql-for-doctrine library, it's better to fail explicitly with clear errors than to use fallback behavior that makes assumptions about node types, as this could lead to functional bugs or security issues.
Applied to files:
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php
🧬 Code graph analysis (2)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php (1)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php (1)
DateTrunc(23-94)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php (4)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTimezoneException.php (1)
InvalidTimezoneException(14-33)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php (3)
InvalidTruncFieldException(15-35)forNonLiteralNode(17-24)forInvalidField(26-34)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php (1)
BaseVariadicFunction(20-143)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/TimezoneValidationTrait.php (1)
validateTimezone(25-36)
🪛 GitHub Actions: CI
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php
[error] 1-1: PHP CS Fixer: no_unused_imports violation. Removed unused import 'InvalidTimezoneException'.
⏰ 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). (21)
- GitHub Check: PostgreSQL 18 + PostGIS 3.6 + PHP 8.5
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.5
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.4
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.5
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.4
- GitHub Check: PostgreSQL 18 + PostGIS 3.6 + PHP 8.3
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.4
- GitHub Check: PostgreSQL 17 + PostGIS 3.6 + PHP 8.3
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.3
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.3
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.2
- GitHub Check: PostgreSQL 18 + PostGIS 3.6 + PHP 8.2
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.2
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.2
- GitHub Check: PostgreSQL 17 + PostGIS 3.6 + PHP 8.2
- GitHub Check: PostgreSQL 18 + PostGIS 3.6 + PHP 8.1
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.1
- GitHub Check: PostgreSQL 17 + PostGIS 3.6 + PHP 8.1
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.1
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.1
- GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer latest
🔇 Additional comments (2)
docs/INTEGRATING-WITH-SYMFONY.md (1)
226-233: DATE_TRUNC registration in Symfony config is consistentThe new
DATE_TRUNCmapping aligns with existing date functions and correctly referencesDateTrunc::class.docs/INTEGRATING-WITH-DOCTRINE.md (1)
158-165: DATE_TRUNC registration in Doctrine config follows existing patternThe
DATE_TRUNCcustom string function is registered consistently with other date functions and points to the correctDateTruncclass.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/DATE-AND-RANGE-FUNCTIONS.md(1 hunks)docs/INTEGRATING-WITH-DOCTRINE.md(1 hunks)docs/INTEGRATING-WITH-LARAVEL.md(1 hunks)docs/INTEGRATING-WITH-SYMFONY.md(1 hunks)docs/USE-CASES-AND-EXAMPLES.md(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTruncTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/INTEGRATING-WITH-SYMFONY.md
- docs/USE-CASES-AND-EXAMPLES.md
- docs/INTEGRATING-WITH-LARAVEL.md
- docs/INTEGRATING-WITH-DOCTRINE.md
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: karpilin
Repo: martin-georgiev/postgresql-for-doctrine PR: 386
File: tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/ToTimestampTest.php:34-39
Timestamp: 2025-05-27T16:10:35.054Z
Learning: PostgreSQL's to_timestamp function returns a fallback result (like '2005-01-01 00:00:00+00') when given an invalid format string, rather than throwing an exception. This behavior is different from some other formatting functions and should be tested accordingly.
📚 Learning: 2025-05-23T11:11:57.951Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 383
File: tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/RadiansTest.php:1-9
Timestamp: 2025-05-23T11:11:57.951Z
Learning: Tests in the `Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\Unit\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase directly, and therefore don't need an explicit import.
Applied to files:
docs/DATE-AND-RANGE-FUNCTIONS.mdsrc/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.phpsrc/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.phptests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTruncTest.php
📚 Learning: 2025-03-29T03:31:17.114Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 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.
Applied to files:
docs/DATE-AND-RANGE-FUNCTIONS.mdsrc/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.phpsrc/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.phptests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTruncTest.php
📚 Learning: 2025-04-18T10:33:52.412Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 350
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php:78-88
Timestamp: 2025-04-18T10:33:52.412Z
Learning: In SQL generation code for the postgresql-for-doctrine library, it's better to fail explicitly with clear errors than to use fallback behavior that makes assumptions about node types, as this could lead to functional bugs or security issues.
Applied to files:
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php
📚 Learning: 2025-08-09T15:31:42.054Z
Learnt from: landure
Repo: martin-georgiev/postgresql-for-doctrine PR: 411
File: src/MartinGeorgiev/Doctrine/DBAL/Types/Exceptions/InvalidLtreeForPHPException.php:11-19
Timestamp: 2025-08-09T15:31:42.054Z
Learning: In the postgresql-for-doctrine library, exception classes for invalid PHP values (Invalid*ForPHPException) follow a consistent pattern using var_export() in their create() method to format error messages. When adding new exception classes, maintain consistency with existing patterns rather than introducing new approaches, even if technically superior alternatives exist.
Applied to files:
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.phpsrc/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php
📚 Learning: 2025-03-11T12:32:10.726Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
Applied to files:
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php
📚 Learning: 2025-08-25T21:29:08.737Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 0
File: :0-0
Timestamp: 2025-08-25T21:29:08.737Z
Learning: The repository should separate value object validation exceptions (extending \InvalidArgumentException) from DBAL conversion exceptions (extending ConversionException) - the Ltree implementation demonstrates this ideal pattern with InvalidLtreeException for validation and InvalidLtreeForPHPException/InvalidLtreeForDatabaseException for conversion errors.
Applied to files:
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php
📚 Learning: 2025-08-25T21:29:08.737Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 0
File: :0-0
Timestamp: 2025-08-25T21:29:08.737Z
Learning: Value object validation in this repository should use dedicated exception classes extending \InvalidArgumentException (like InvalidLtreeException, InvalidPointException) with clear documentation separating validation concerns from DBAL conversion concerns, and should include protected static assertion methods for validation logic.
Applied to files:
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php
📚 Learning: 2025-09-01T18:48:28.508Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 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/ORM/Query/AST/Functions/DateTruncTest.php
📚 Learning: 2025-04-22T00:03:37.733Z
Learnt from: martin-georgiev
Repo: martin-georgiev/postgresql-for-doctrine PR: 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/ORM/Query/AST/Functions/DateTruncTest.php
🧬 Code graph analysis (3)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php (3)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php (3)
InvalidTruncFieldException(15-35)forNonLiteralNode(17-24)forInvalidField(26-34)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php (1)
BaseVariadicFunction(20-143)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Traits/TimezoneValidationTrait.php (1)
validateTimezone(25-36)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php (1)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php (1)
DateTrunc(22-93)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTruncTest.php (6)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php (1)
BaseVariadicFunction(20-143)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php (1)
DateTrunc(22-93)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidArgumentForVariadicFunctionException.php (1)
InvalidArgumentForVariadicFunctionException(7-56)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTimezoneException.php (1)
InvalidTimezoneException(14-33)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php (1)
InvalidTruncFieldException(15-35)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TestCase.php (1)
buildEntityManager(115-118)
⏰ 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: PostgreSQL 17 + PostGIS 3.5 + PHP 8.5
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.4
- GitHub Check: PostgreSQL 18 + PostGIS 3.6 + PHP 8.5
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.5
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.3
- GitHub Check: PostgreSQL 18 + PostGIS 3.6 + PHP 8.4
- GitHub Check: PostgreSQL 17 + PostGIS 3.6 + PHP 8.2
- GitHub Check: PostgreSQL 17 + PostGIS 3.6 + PHP 8.4
- GitHub Check: PostgreSQL 18 + PostGIS 3.6 + PHP 8.2
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.2
- GitHub Check: PostgreSQL 17 + PostGIS 3.6 + PHP 8.1
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.2
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.1
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.1
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.1
- GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: wait-for-tests-worflows-before-upload
🔇 Additional comments (11)
docs/DATE-AND-RANGE-FUNCTIONS.md (1)
14-14: LGTM!The documentation entry correctly maps the PostgreSQL
date_truncfunction to theDATE_TRUNCDQL identifier and follows the established table format.tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTruncTest.php (6)
18-28: LGTM!The fixture setup and function mapping follow the established test patterns in this codebase.
30-46: LGTM!The test cases cover the key scenarios: with timezone (3 arguments), without timezone (2 arguments), and usage in WHERE clauses. The expected SQL correctly reflects PostgreSQL's
date_truncsyntax.
48-73: LGTM!The test correctly validates argument count boundaries and verifies appropriate exception messages for both under and over-specification of arguments.
75-95: LGTM!Using a data provider that iterates over
DateTrunc::FIELDSensures all valid truncation fields are tested, maintaining consistency with the implementation.
97-119: LGTM!The test coverage for invalid fields includes edge cases like empty strings, whitespace, and clearly invalid values, ensuring robust validation.
121-143: LGTM!The timezone validation tests cover similar edge cases and verify that the trait-based validation correctly rejects invalid timezone identifiers.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTrunc.php (4)
28-42: LGTM!The
FIELDSconstant correctly lists all valid PostgreSQL truncation units as documented in the PostgreSQL reference, providing a clear contract for validation.
44-62: LGTM!The function configuration correctly specifies:
StringPrimarynode mapping for literal string arguments- 2–3 argument range matching PostgreSQL's
date_trunc('field', source [, time_zone])signature
64-74: LGTM!The validation sequence is correct: parent validation for argument count, then field validation, and finally optional timezone validation when a third argument is present.
81-92: LGTM!The field validation logic correctly:
- Ensures the argument is a string literal
- Normalizes the field value by trimming quotes and converting to lowercase
- Validates against the
FIELDSconstant- Throws appropriate
InvalidTruncFieldExceptionfor invalid cases
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Exception/InvalidTruncFieldException.php
Outdated
Show resolved
Hide resolved
| /** | ||
| * @see https://www.postgresql.org/docs/current/functions-datetime.html#FUNCTIONS-DATETIME-TRUNC | ||
| */ | ||
| public const FIELDS = [ |
Owner
There was a problem hiding this comment.
Can you elaborate on why this is a public constant? There is no such pattern in any other classes in the repository. On the surface, it is needed for tests primarily.
Contributor
Author
There was a problem hiding this comment.
I wanted to both (a) enumerate the allowed options in InvalidTruncFieldException message and (b) have the list of valid options available for input validation.
It's in a public constant because it will likely never change (hence not a static variable), it's exposed as a class constant because it's needed in two separate classes, and it's not hardcoded as two separate arrays in each class because if it does change, someone will have to remember changing it in both places.
At the end of the day if you think it's better as two separate hardcoded arrays (because it's unlikely to change), let's do it that way for sake of overall consistency.
| $field = \strtolower(\trim((string) $node->value, "'\"")); | ||
|
|
||
| if (!\in_array($field, self::FIELDS, true)) { | ||
| throw InvalidTruncFieldException::forInvalidField($field, $this->getFunctionName()); |
Owner
There was a problem hiding this comment.
Why not let PostgreSQL decide whether this is fine? Excessive validation may limit support for future PostgreSQL versions.
Contributor
Author
There was a problem hiding this comment.
Personal preference of code that fails fast - this will fail before any SQL query gets executed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The subject says it all.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.