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 14 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.34% +1.94%
- Complexity 953 1173 +220
============================================
Files 108 124 +16
Lines 3018 3584 +566
============================================
+ Hits 2849 3453 +604
+ Misses 169 131 -38 ☔ View full report in Codecov by Sentry. 🚀 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.
I don't think it's a good approach. Looks weird and is fragile. When named arguments are used, it breaks:
new DateHandler(
messageDateType: IntlDateFormatter::SHORT,
tooLateMessage: 'Max: {limit}.',
);There was a problem hiding this comment.
I think the best approach is to change defaults to null. It is backwards compatibility break and can be in major release only but still that's better than super-hacky way with func_num_args().
What does this PR do?
In date rules, use
formatproperty for formatting dates in error messages whenmessageFormatandmessageDateType/messageTimeTypeare not set.BC break
When
formatis set butmessageFormatis not, dates in error messages are now formatted usingformatinstead ofIntlDateFormatter::SHORT.Before:
After:
To keep the old behavior, set
messageFormatormessageDateTypeexplicitly.Coverage
Tests: 77 → 80. Line coverage: 115/115 (100%) → 119/119 (100%). MSI: 95% (111/116) → 95% (118/123).
Summary by CodeRabbit
Bug Fixes
Tests