Skip to content

Commit 6c1b045

Browse files
roxblnfkroxblnfk
committed
feat: Warn when ambiguous DateInterval format detected
1 parent dfa5fb0 commit 6c1b045

File tree

2 files changed

+209
-1
lines changed

2 files changed

+209
-1
lines changed

‎src/Internal/Support/DateInterval.php‎

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,21 @@ public static function parse(mixed $interval, string $format = self::FORMAT_MILL
6060
{
6161
switch (true) {
6262
case \is_string($interval):
63-
return CarbonInterval::fromString($interval);
63+
$carbon = CarbonInterval::fromString($interval);
64+
if (self::isIso8601DurationFormat($interval)) {
65+
$builtin = new \DateInterval($interval);
66+
$carbon->compare($builtin) === 0 or \trigger_error(
67+
\sprintf(
68+
'Ambiguous duration "%s": Carbon and DateInterval parse it differently. ' .
69+
'Use new \DateInterval("%s") for ISO 8601 standard parsing or PT/P prefix to clarify intent.',
70+
$interval,
71+
$interval,
72+
),
73+
\E_USER_WARNING,
74+
);
75+
}
76+
77+
return $carbon;
6478

6579
case $interval instanceof \DateInterval:
6680
return CarbonInterval::instance($interval);
@@ -180,4 +194,27 @@ private static function validateFormat(string $format): void
180194
throw new \InvalidArgumentException($message);
181195
}
182196
}
197+
198+
/**
199+
* Checks if a string matches the ISO 8601 duration format that PHP's DateInterval constructor accepts.
200+
*
201+
* Valid format: P[n]Y[n]M[n]W[n]D[T[n]H[n]M[n]S]
202+
* - Must start with P (period)
203+
* - Date elements (Y, M, W, D) come before T
204+
* - Time elements (H, M, S) come after T
205+
* - At least one date or time element must be present
206+
* - Alternative datetime format P<date>T<time> is also supported
207+
*
208+
* Examples: P2D, PT5M, P1Y2M3DT4H5M6S, P0001-00-00T00:00:00
209+
*/
210+
private static function isIso8601DurationFormat(string $interval): bool
211+
{
212+
// ISO 8601 duration format: P[n]Y[n]M[n]W[n]D[T[n]H[n]M[n]S]
213+
// At least one element (Y, M, W, D, H, M, or S) must be present
214+
// Alternative format: P<date>T<time> like P0001-00-00T00:00:00
215+
return \preg_match(
216+
'/^P(?=.)(?:\d+Y)?(?:\d+M)?(?:\d+W)?(?:\d+D)?(?:T(?=.)(?:\d+H)?(?:\d+M)?(?:\d+(?:\.\d+)?S)?)?$|^P\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}$/',
217+
$interval
218+
) === 1 && $interval !== 'P' && $interval !== 'PT';
219+
}
183220
}

‎tests/Unit/Internal/Support/DateIntervalTestCase.php‎

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,35 @@ public static function provideValuesToParse(): iterable
3131
yield [null, DateInterval::FORMAT_MILLISECONDS, 0, '0/0/0/0'];
3232
}
3333

34+
public static function provideIso8601DurationFormats(): \Generator
35+
{
36+
// Valid ISO 8601 duration formats
37+
yield 'two days' => ['P2D', true];
38+
yield 'two seconds' => ['PT2S', true];
39+
yield 'six years five minutes' => ['P6YT5M', true];
40+
yield 'three months' => ['P3M', true];
41+
yield 'three minutes' => ['PT3M', true];
42+
yield 'full format' => ['P1Y2M3DT4H5M6S', true];
43+
yield 'weeks only' => ['P2W', true];
44+
yield 'hours and minutes' => ['PT1H30M', true];
45+
yield 'days and hours' => ['P1DT12H', true];
46+
yield 'alternative datetime format' => ['P0001-00-00T00:00:00', true];
47+
yield 'decimal seconds' => ['PT1.5S', true];
48+
49+
// Invalid formats (Carbon-specific or non-ISO 8601)
50+
yield 'only period marker' => ['P', false];
51+
yield 'only period and time marker' => ['PT', false];
52+
yield 'natural language' => ['2 days', false];
53+
yield 'human readable' => ['1 hour 30 minutes', false];
54+
yield 'no period marker' => ['2D', false];
55+
yield 'wrong order' => ['P4D1Y', false];
56+
yield 'time without T' => ['P1H30M', false];
57+
yield 'negative value' => ['P-2D', false];
58+
yield 'negative prefix' => ['-P2D', false];
59+
yield 'spaces' => ['P 2 D', false];
60+
yield 'lowercase' => ['p2d', false];
61+
}
62+
3463
#[DataProvider('provideValuesToParse')]
3564
public function testParse(mixed $value, string $format, int $microseconds, string $formatted): void
3665
{
@@ -62,4 +91,146 @@ public function testParseFromDuration(): void
6291
self::assertSame(5124, (int) $i->totalSeconds);
6392
self::assertSame(123_456, $i->microseconds);
6493
}
94+
95+
#[DataProvider('provideIso8601DurationFormats')]
96+
public function testParseDetectsIso8601FormatCorrectly(string $interval, bool $shouldBeIso8601): void
97+
{
98+
// Arrange
99+
$reflection = new \ReflectionClass(DateInterval::class);
100+
$method = $reflection->getMethod('isIso8601DurationFormat');
101+
$method->setAccessible(true);
102+
103+
// Act
104+
$result = $method->invoke(null, $interval);
105+
106+
// Assert
107+
self::assertSame(
108+
$shouldBeIso8601,
109+
$result,
110+
\sprintf(
111+
'String "%s" should %s recognized as ISO 8601 duration format',
112+
$interval,
113+
$shouldBeIso8601 ? 'be' : 'NOT be',
114+
),
115+
);
116+
}
117+
118+
public static function provideCarbonDateIntervalDifferences(): \Generator
119+
{
120+
// Cases where Carbon and DateInterval parse the same string differently
121+
// Format: [interval string, expected warning]
122+
123+
// P2M: Carbon parses as 2 minutes, DateInterval as 2 months
124+
yield 'P2M - ambiguous months/minutes' => ['P2M', true];
125+
126+
// PT1H5M: Carbon may parse as 1 hour (dropping the 5M), DateInterval as 1 hour 5 minutes
127+
yield 'PT1H5M - potential parsing issue' => ['PT1H5M', false]; // This should work correctly
128+
129+
// Cases that should NOT trigger warning (identical parsing)
130+
yield 'PT2M - explicit minutes with T' => ['PT2M', false];
131+
yield 'P1Y - explicit years' => ['P1Y', false];
132+
yield 'P2D - explicit days' => ['P2D', false];
133+
yield 'PT5S - explicit seconds' => ['PT5S', false];
134+
}
135+
136+
#[DataProvider('provideCarbonDateIntervalDifferences')]
137+
public function testParseTriggersWarningWhenCarbonAndDateIntervalDiffer(
138+
string $interval,
139+
bool $shouldTriggerWarning,
140+
): void {
141+
// Arrange
142+
$warningTriggered = false;
143+
$warningMessage = '';
144+
145+
\set_error_handler(static function (int $errno, string $errstr) use (&$warningTriggered, &$warningMessage): bool {
146+
if ($errno === \E_USER_WARNING && \str_contains($errstr, 'Ambiguous duration')) {
147+
$warningTriggered = true;
148+
$warningMessage = $errstr;
149+
return true;
150+
}
151+
return false;
152+
});
153+
154+
// Act
155+
try {
156+
$result = DateInterval::parse($interval);
157+
} finally {
158+
\restore_error_handler();
159+
}
160+
161+
// Assert
162+
self::assertInstanceOf(\Carbon\CarbonInterval::class, $result);
163+
164+
if ($shouldTriggerWarning) {
165+
self::assertTrue(
166+
$warningTriggered,
167+
\sprintf('Expected warning for interval "%s" but none was triggered', $interval),
168+
);
169+
self::assertStringContainsString(
170+
'Ambiguous duration',
171+
$warningMessage,
172+
'Warning message should mention ambiguous duration',
173+
);
174+
self::assertStringContainsString(
175+
\sprintf('"%s"', $interval),
176+
$warningMessage,
177+
'Warning message should contain the interval value',
178+
);
179+
self::assertStringContainsString(
180+
'Carbon and DateInterval parse it differently',
181+
$warningMessage,
182+
'Warning message should explain the issue',
183+
);
184+
} else {
185+
self::assertFalse(
186+
$warningTriggered,
187+
\sprintf(
188+
'Did not expect warning for interval "%s" but one was triggered: %s',
189+
$interval,
190+
$warningMessage,
191+
),
192+
);
193+
}
194+
}
195+
196+
public static function provideNonIso8601FormatsNoWarning(): \Generator
197+
{
198+
// Natural language formats that Carbon accepts but aren't ISO 8601
199+
// These should NOT trigger warnings because they don't match ISO 8601 format
200+
yield 'natural language - 2 days' => ['2 days'];
201+
yield 'natural language - 1 hour' => ['1 hour'];
202+
yield 'natural language - 30 minutes' => ['30 minutes'];
203+
}
204+
205+
#[DataProvider('provideNonIso8601FormatsNoWarning')]
206+
public function testParseDoesNotTriggerWarningForNonIso8601Formats(string $interval): void
207+
{
208+
// Arrange
209+
$warningTriggered = false;
210+
211+
\set_error_handler(static function (int $errno, string $errstr) use (&$warningTriggered): bool {
212+
if ($errno === \E_USER_WARNING && \str_contains($errstr, 'Ambiguous duration')) {
213+
$warningTriggered = true;
214+
return true;
215+
}
216+
return false;
217+
});
218+
219+
// Act
220+
try {
221+
$result = DateInterval::parse($interval);
222+
} finally {
223+
\restore_error_handler();
224+
}
225+
226+
// Assert
227+
self::assertInstanceOf(\Carbon\CarbonInterval::class, $result);
228+
self::assertFalse(
229+
$warningTriggered,
230+
\sprintf(
231+
'Non-ISO 8601 format "%s" should not trigger DateInterval comparison warning',
232+
$interval,
233+
),
234+
);
235+
}
65236
}

0 commit comments

Comments
 (0)