Fix #709: Use format for error message dates when unset#790
Fix #709: Use format for error message dates when unset#790WarLikeLaux wants to merge 16 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #790 +/- ##
============================================
+ Coverage 94.40% 96.31% +1.91%
- Complexity 953 1161 +208
============================================
Files 108 124 +16
Lines 3018 3556 +538
============================================
+ Hits 2849 3425 +576
+ Misses 169 131 -38 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates date/time validation error message formatting so that when message-specific formatting options aren’t provided, the rule’s format is used to render limit values in error messages (addressing #709). This changes default error message output and is a documented BC break.
Changes:
- Update
BaseDateHandler::formatDate()to fall back to the rule’sformatwhen message format/type options are unset. - Adjust existing date/time rule tests to match the new formatting behavior.
- Add new test cases to verify precedence between
formatand message-format settings.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/Rule/Date/BaseDateHandler.php |
Implements fallback to rule format for error-message date/time formatting when message formatting isn’t configured. |
tests/Rule/Date/DateTest.php |
Updates expectations and adds coverage for format-driven message formatting and messageFormat override behavior. |
tests/Rule/Date/DateTimeTest.php |
Updates expected min/max/timestamp error message formatting to use the rule format. |
tests/Rule/Date/TimeTest.php |
Updates expected time error message formatting and adds a precedence test for format vs handler message type. |
CHANGELOG.md |
Adds a changelog entry for the behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WarLikeLaux
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughWhen date error message formatting properties are unset, this change enables fallback to the rule's ChangesDate Error Message Format Fallback
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 full review |
✅ Actions performedFull review triggered. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Rule/Date/BaseDateHandler.php`:
- Around line 200-203: The current fallback assigns $format = $rule->getFormat()
whenever $format is null even if handler-level messageDateType/messageTimeType
are set, which forces pattern-style formatting and ignores resolved Intl styles;
change the condition so you only fall back to $rule->getFormat() when $format is
null AND both $ruleMessageDateType and $ruleMessageTimeType are null AND the
handler-level messageDateType and messageTimeType (e.g. $this->messageDateType
and $this->messageTimeType) are also null, ensuring explicit handler message
types are not bypassed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ecfccb7-db11-4252-9e0d-14f3a736b356
📒 Files selected for processing (5)
CHANGELOG.mdsrc/Rule/Date/BaseDateHandler.phptests/Rule/Date/DateTest.phptests/Rule/Date/DateTimeTest.phptests/Rule/Date/TimeTest.php
Co-authored-by: Sergei Predvoditelev <sergey.predvoditelev@gmail.com>
| ?? $this->getTimeTypeFromRule($rule); | ||
|
|
||
| $format = $rule->getMessageFormat() ?? $this->messageFormat; | ||
| if ($format === null && $ruleMessageDateType === null && $ruleMessageTimeType === null) { |
There was a problem hiding this comment.
This overrides explicit handler-level message formatting. For example, an app-level new TimeHandler(messageTimeType: IntlDateFormatter::FULL) is now ignored whenever the rule has format, because this fallback only checks the rule-level message type properties. That makes global message formatting configuration ineffective unless every rule also sets messageTimeType / messageDateType or messageFormat. We need to preserve handler-level precedence here; with the current constructor defaults, that may require distinguishing default handler message types from explicitly configured ones before falling back to $rule->getFormat().
There was a problem hiding this comment.
@WarLikeLaux — the fix does address samdark's concern. Looking at the current formatDate() method in the file, the fallback condition is:
if (
$format === null
&& $ruleMessageDateType === null
&& $ruleMessageTimeType === null
&& $this->messageDateType === null // ← handler-level check added
&& $this->messageTimeType === null // ← handler-level check added
) {
$format = $rule->getFormat();
}The two additional guards ($this->messageDateType === null and $this->messageTimeType === null) mean that if a handler is configured with, say, new TimeHandler(messageTimeType: IntlDateFormatter::FULL), the condition evaluates to false and the fallback to $rule->getFormat() is skipped — preserving the handler-level configuration exactly as samdark requested.
The diff in this review thread appears truncated and doesn't show those last two lines, but the actual file already contains them. samdark's concern is fully addressed.
There was a problem hiding this comment.
@samdark got handler precedence working with the SHORT defaults preserved (BC), tests + mutation coverage green. But I'm relying on func_num_args() to distinguish a passed arg from the default, which is brittle - tied to arg positions. Not sure it's the right call. How would you approach distinguishing default vs explicit here, and do you want the explicit-null-follows-rule-type bit at all? Happy to redo it your way.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
We can try add default* parameters, that don't override rule parameters and use current parameters to override rule parameters.
There was a problem hiding this comment.
Reworked per @vjik - new defaultMessageDateType/defaultMessageTimeType keep the old SHORT default without overriding the rule, and messageDateType/messageTimeType are now the override over the rule's format. Dropped func_num_args().
The BC checker flags the messageDateType default going from SHORT to null, but I think it's only the signature - new *Handler() formats exactly as before because default* carries the SHORT. So the report is red, the runtime behavior isn't.
There was a problem hiding this comment.
@coderabbitai please double-check the implementation and the BC impact
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -e php '(DateHandler|TimeHandler|DateTimeHandler)' --exec cat {}Length of output: 24858
@WarLikeLaux — I've gone through the implementation and all three concrete handler constructors in detail. Here's the full analysis:
Implementation Review
Priority chain in formatDate() – ✅ Correct
$formatterDateType = $ruleMessageDateType # rule.messageDateType (highest)
?? $this->messageDateType # explicit handler override
?? $this->defaultMessageDateType # carries SHORT default (BC fallback)
?? (Time ? NONE : SHORT) # last resort (never reached normally)
Format fallback condition – ✅ Correct
if (
$format === null
&& $this->messageDateType === null // blocks if handler explicitly set a type
&& $this->messageTimeType === null // blocks if handler explicitly set a type
&& $ruleMessageDateType === null
&& $ruleMessageTimeType === null
) {
$format = $rule->getFormat();
}All four scenarios work correctly:
| Scenario | Behavior |
|---|---|
| No format, no overrides anywhere | defaultMessageDateType = SHORT → Intl SHORT — same as before ✅ |
Rule has format, no message overrides |
Fallback activates → rule's format used in message ✅ |
new DateHandler(messageDateType: FULL) |
$this->messageDateType !== null → format fallback blocked → FULL Intl type ✅ |
new TimeHandler(messageTimeType: FULL) |
$this->messageTimeType !== null → format fallback blocked → FULL Intl type ✅ |
samdark's original concern (global messageDateType/messageTimeType on the handler being bypassed) is fully resolved.
BC Impact
The BC checker is technically correct — messageDateType default changed from int = SHORT to ?int = null in all three concrete handlers. However:
new DateHandler()with no rule format → still outputs Intl SHORT (viadefaultMessageDateType = SHORT) — no runtime BC break ✅new DateHandler()when rule hasformatset → now usesformatfor message display instead of Intl SHORT — this is the intentional BC break described in the PR and changelog ✅
So the BC checker flag is accurate as a signature change, but the only runtime difference is the intended new behavior (format-set rules show that format in error messages). All pre-existing code that never relied on message format specifically will continue to see Intl SHORT output.
Minor observation
One edge case worth being aware of: if someone configures new DateTimeHandler(messageDateType: FULL) (date type only, no messageTimeType), the messageDateType !== null check already blocks the format fallback for the entire call — including the time portion. This is technically correct (the format pattern covers both date and time anyway when a format is set), but it means the time portion defaults to Intl SHORT rather than potentially inheriting the rule's format. This is unlikely to matter in practice since if someone sets messageDateType, they probably want Intl formatting throughout.
Overall the implementation is solid. ✅
🧬 Code Graph Analysis Results
src/Rule/Date/BaseDateHandler.php
Constructor (config/defaults and message format inputs) — lines 35-48
public function __construct(
private readonly int $dateType,
private readonly int $timeType,
private readonly ?string $timeZone,
private readonly ?string $locale,
private readonly ?string $messageFormat,
private readonly ?int $messageDateType,
private readonly ?int $messageTimeType,
private readonly string $incorrectInputMessage,
private readonly string $tooEarlyMessage,
private readonly string $tooLateMessage,
private readonly ?int $defaultMessageDateType = null,
private readonly ?int $defaultMessageTimeType = null,
) {}validate() (type checking, timezone resolution, min/max comparisons, error creation) — lines 50-104
public function validate(mixed $value, RuleInterface $rule, ValidationContext $context): Result
{
if (!$rule instanceof Date && !$rule instanceof DateTime && !$rule instanceof Time) {
throw new UnexpectedRuleException([Date::class, DateTime::class, Time::class], $rule);
}
$timeZone = $rule->getTimeZone() ?? $this->timeZone;
if ($timeZone !== null) {
$timeZone = new DateTimeZone($timeZone);
}
$result = new Result();
$date = $this->prepareValue($value, $rule, $timeZone, false);
if ($date === null) {
$result->addError(
$rule->getIncorrectInputMessage() ?? $this->incorrectInputMessage,
[
'property' => $context->getTranslatedProperty(),
'Property' => $context->getCapitalizedTranslatedProperty(),
],
);
return $result;
}
$min = $this->prepareValue($rule->getMin(), $rule, $timeZone, true);
if ($min !== null && $date < $min) {
$result->addError(
$rule->getTooEarlyMessage() ?? $this->tooEarlyMessage,
[
'property' => $context->getTranslatedProperty(),
'Property' => $context->getCapitalizedTranslatedProperty(),
'value' => $this->formatDate($date, $rule, $timeZone),
'limit' => $this->formatDate($min, $rule, $timeZone),
],
);
return $result;
}
$max = $this->prepareValue($rule->getMax(), $rule, $timeZone, true);
if ($max !== null && $date > $max) {
$result->addError(
$rule->getTooLateMessage() ?? $this->tooLateMessage,
[
'property' => $context->getTranslatedProperty(),
'Property' => $context->getCapitalizedTranslatedProperty(),
'value' => $this->formatDate($date, $rule, $timeZone),
'limit' => $this->formatDate($max, $rule, $timeZone),
],
);
return $result;
}
return $result;
}prepareValue() (value parsing for int/timestamp, DateTimeInterface, string formats; strict min/max behavior) — lines 106-152
private function prepareValue(
mixed $value,
Date|DateTime|Time $rule,
?DateTimeZone $timeZone,
bool $strict,
): ?DateTimeInterfaceDocstring-style summary:
- Parameters:
$value(int/string/DateTimeInterface/null/other),$rule(Date|DateTime|Time),$timeZone(nullableDateTimeZone),$strict(bool). - Returns:
?DateTimeInterface(parsed/normalized value ornull). - Exceptions: if
$result === nulland$strict === true, it throwsLogicException('Invalid date value.'). - Implementation details:
int=>makeDateTimeFromTimestamp($timestamp, $timeZone)null=> returnsnullDateTimeInterface=> uses the provided objectstring=>- if
$rule->getFormat()starts withphp:=>prepareValueWithPhpFormat() - else =>
prepareValueWithIntlFormat()using rule-derived date/time types and locale ($rule->getLocale() ?? $this->locale)
- if
- After parsing, normalization depends on rule type:
Date=>setTime(0, 0)Time=>setDate(2024, 3, 29)
- Otherwise returns
null.
prepareValueWithPhpFormat() (strict parsing w/ warning_count rejection) — lines 154-170
private function prepareValueWithPhpFormat(
string $value,
string $format,
?DateTimeZone $timeZone,
): ?DateTimeInterfaceprepareValueWithIntlFormat() (IntlDateFormatter parse, lenient=false, int timestamp check) — lines 176-188
private function prepareValueWithIntlFormat(
string $value,
?string $format,
int $dateType,
int $timeType,
?DateTimeZone $timeZone,
?string $locale,
): ?DateTimeInterfaceformatDate() (message formatting type selection and php:-format fallback) — lines 190-227
private function formatDate(DateTimeInterface $date, Date|DateTime|Time $rule, ?DateTimeZone $timeZone): stringDocstring-style summary:
- Parameters:
$date(value to format),$rule(Date|DateTime|Time),$timeZone(nullableDateTimeZone). - Returns: formatted string.
- Implementation details:
- Determines
$formatterDateType/$formatterTimeTypefrom rule message-specific types, then handler defaults, then fallback toIntlDateFormatter::SHORTorIntlDateFormatter::NONEdepending on rule type. - Uses
$rule->getMessageFormat() ?? $this->messageFormat. If all message-type overrides are null, it falls back to$rule->getFormat(). - If message format starts with
php:, returns$date->format(substr($format, 4)); otherwise usesmakeFormatter()->format($date).
- Determines
makeFormatter() (IntlDateFormatter construction with pattern vs types) — lines 229-247
private function makeFormatter(
?string $format,
?string $locale,
int $dateType,
int $timeType,
?DateTimeZone $timeZone,
): IntlDateFormattersrc/ValidationContext.php
getTranslatedProperty() (provides {property} for message parameters) — lines 255-268
public function getTranslatedProperty(): string
{
$label = $this->propertyLabel ?? $this->property ?? 'value';
if ($this->propertyTranslator !== null) {
return $this->propertyTranslator->translate($label);
}
if ($this->defaultPropertyTranslator !== null) {
return $this->defaultPropertyTranslator->translate($label);
}
return $label;
}getCapitalizedTranslatedProperty() (provides {Property} for message parameters) — lines 270-273
public function getCapitalizedTranslatedProperty(): string
{
return StringHelper::uppercaseFirstCharacter($this->getTranslatedProperty());
}src/Result.php
addError() (how validation errors are collected with parameters/valuePath) — lines 283-287
public function addError(string|Stringable $message, array $parameters = [], array $valuePath = []): self
{
$this->errors[] = new Error($message, $parameters, $valuePath);
return $this;
}src/Exception/UnexpectedRuleException.php
Constructor (error thrown when rule type is not supported by the handler) — lines 40-64
public function __construct(
string|array $expectedClassName,
object $actualObject,
int $code = 0,
?Throwable $previous = null,
) {
parent::__construct(
sprintf(
'Expected "%s", but "%s" given.',
implode('", "', (array) $expectedClassName),
$actualObject::class,
),
$code,
$previous,
);
}src/Rule/Date/DateHandler.php
Constructor (default wiring into BaseDateHandler, including Date-specific types) — lines 19-44
public function __construct(
int $dateType = IntlDateFormatter::SHORT,
?string $timeZone = null,
?string $locale = null,
?string $messageFormat = null,
?int $messageDateType = null,
string $incorrectInputMessage = '{Property} must be a date.',
string $tooEarlyMessage = '{Property} must be no earlier than {limit}.',
string $tooLateMessage = '{Property} must be no later than {limit}.',
int $defaultMessageDateType = IntlDateFormatter::SHORT,
) {
parent::__construct(
$dateType,
IntlDateFormatter::NONE,
$timeZone,
$locale,
$messageFormat,
$messageDateType,
null,
$incorrectInputMessage,
$tooEarlyMessage,
$tooLateMessage,
$defaultMessageDateType,
null,
);
}src/Rule/Date/DateTimeHandler.php
Constructor (wires both date/time types + message-type defaults into BaseDateHandler) — lines 21-49
public function __construct(
int $dateType = IntlDateFormatter::SHORT,
int $timeType = IntlDateFormatter::SHORT,
?string $timeZone = null,
?string $locale = null,
?string $messageFormat = null,
?int $messageDateType = null,
?int $messageTimeType = null,
string $incorrectInputMessage = '{Property} must be a date.',
string $tooEarlyMessage = '{Property} must be no earlier than {limit}.',
string $tooLateMessage = '{Property} must be no later than {limit}.',
int $defaultMessageDateType = IntlDateFormatter::SHORT,
int $defaultMessageTimeType = IntlDateFormatter::SHORT,
) {
parent::__construct(
$dateType,
$timeType,
$timeZone,
$locale,
$messageFormat,
$messageDateType,
$messageTimeType,
$incorrectInputMessage,
$tooEarlyMessage,
$tooLateMessage,
$defaultMessageDateType,
$defaultMessageTimeType,
);
}src/Rule/Date/TimeHandler.php
Constructor (wires IntlDateFormatter::NONE for date in BaseDateHandler) — lines 19-44
public function __construct(
int $timeType = IntlDateFormatter::SHORT,
?string $timeZone = null,
?string $locale = null,
?string $messageFormat = null,
?int $messageTimeType = null,
string $incorrectInputMessage = '{Property} must be a time.',
string $tooEarlyMessage = '{Property} must be no earlier than {limit}.',
string $tooLateMessage = '{Property} must be no later than {limit}.',
int $defaultMessageTimeType = IntlDateFormatter::SHORT,
) {
parent::__construct(
IntlDateFormatter::NONE,
$timeType,
$timeZone,
$locale,
$messageFormat,
null,
$messageTimeType,
$incorrectInputMessage,
$tooEarlyMessage,
$tooLateMessage,
null,
$defaultMessageTimeType,
);
}✏️ Learnings added
Learnt from: WarLikeLaux
Repo: yiisoft/validator PR: 790
File: src/Rule/Date/BaseDateHandler.php:0-0
Timestamp: 2026-06-08T09:34:07.640Z
Learning: In `src/Rule/Date/BaseDateHandler.php` (PHP, Yiisoft Validator), the `formatDate()` method uses `defaultMessageDateType`/`defaultMessageTimeType` (introduced in PR `#790`) to carry the old `IntlDateFormatter::SHORT` default without overriding the rule's format. The concrete handlers (DateHandler, TimeHandler, DateTimeHandler) set `messageDateType`/`messageTimeType` to `null` (allowing format fallback) and `defaultMessageDateType`/`defaultMessageTimeType` to `SHORT` (preserving BC when no format is set). The format fallback (`$format = $rule->getFormat()`) only activates when `$this->messageDateType === null && $this->messageTimeType === null && $ruleMessageDateType === null && $ruleMessageTimeType === null`. Setting any handler-level message type explicitly blocks the fallback, preserving handler-level Intl formatting configuration.
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.
🧠 Learnings used
Learnt from: WarLikeLaux
Repo: yiisoft/validator PR: 790
File: src/Rule/Date/BaseDateHandler.php:200-203
Timestamp: 2026-05-30T12:54:01.222Z
Learning: In `src/Rule/Date/BaseDateHandler.php` (PHP, Yiisoft Validator), the `formatDate()` fallback in `formatDate()` is intentional by design (issue `#709`): when `messageFormat` is unset and the rule has no message-specific date/time types, the rule's own `format` property takes priority over handler-level `messageDateType`/`messageTimeType` defaults. This is a documented breaking change. To preserve handler-level Intl formatting, callers must set message properties explicitly on the rule.
What does this PR do?
In date rules, use the
formatproperty for formatting dates in error messages whenmessageFormatandmessageDateType/messageTimeTypeare not set.Handler-level message types take precedence over the rule's
format: an explicitnew TimeHandler(messageTimeType: IntlDateFormatter::FULL)keeps formatting the limit even when the rule has aformat. NewdefaultMessageDateType/defaultMessageTimeTypeparameters carry the previousSHORTdefault without overriding the rule, so omitting message types behaves as before.BC break
When
formatis set butmessageFormat/messageDateType/messageTimeTypeare not, dates in error messages are now formatted usingformatinstead ofIntlDateFormatter::SHORT.Before:
After:
To keep the old behavior, set
messageFormat,messageDateType, ormessageTimeTypeexplicitly.Summary by CodeRabbit
Bug Fixes
Tests