Conversation
Reviewer's GuideReplaces SpoonDate-based Twig date/time formatting with IntlDateFormatter and introduces a custom time-ago implementation and localized labels, while adding a helper for parameterized labels. Sequence diagram for the new timeAgo Twig filtersequenceDiagram
participant TwigTemplate
participant FrontendTemplateModifiers as FrontendTemplateModifiers
participant FrontendLanguage as FrontendCoreLanguageLanguage
TwigTemplate->>FrontendTemplateModifiers: timeAgo(timestamp)
activate FrontendTemplateModifiers
FrontendTemplateModifiers->>FrontendTemplateModifiers: now = time()
FrontendTemplateModifiers->>FrontendTemplateModifiers: diff = |now - timestamp|
FrontendTemplateModifiers->>FrontendTemplateModifiers: compute seconds, minutes, hours, days, months, years
FrontendTemplateModifiers->>FrontendTemplateModifiers: select count and keySingular/keyPlural
alt count <= 0
FrontendTemplateModifiers->>FrontendLanguage: lbl(TimeAgoEmpty)
FrontendLanguage-->>FrontendTemplateModifiers: label
FrontendTemplateModifiers-->>TwigTemplate: label
else count == 1
FrontendTemplateModifiers->>FrontendLanguage: lbl(keySingular)
FrontendLanguage-->>FrontendTemplateModifiers: label
FrontendTemplateModifiers-->>TwigTemplate: label
else count > 1
FrontendTemplateModifiers->>FrontendLanguage: lblWithParameters(keyPlural, [count])
FrontendLanguage-->>FrontendTemplateModifiers: formattedLabel
FrontendTemplateModifiers-->>TwigTemplate: formattedLabel
end
deactivate FrontendTemplateModifiers
Class diagram for updated date and time Twig modifiersclassDiagram
class FrontendCoreEngineTemplateModifiers {
<<static>>
+string formatDate(var)
+string formatDateTime(var)
+string formatTime(var)
+string timeAgo(timestamp)
}
class BackendCoreEngineTemplateModifiers {
<<static>>
+string formatDate(var)
+string formatDateTime(var)
+string formatTime(var)
}
class CommonCoreTwigExtensionsBaseTwigModifiers {
<<static>>
+string spoonDate(timestamp, format, language)
}
class FrontendCoreLanguageLanguage {
<<static>>
+string lbl(key, fallback)
+string lblWithParameters(key, parameters, fallback)
+string getLabel(key, fallback)
}
class BackendCoreLanguageLanguage {
<<static>>
+string getInterfaceLanguage()
}
class FrontendCoreLanguageLocale {
<<static>>
+string frontendLanguage()
}
class IntlDateFormatter {
+IntlDateFormatter(locale, dateType, timeType, timezone, calendar, pattern)
+string format(timestamp)
}
class BackendCoreEngineAuthentication {
<<static>>
+User getUser()
}
class User {
+string getSetting(key)
}
%% Relationships
FrontendCoreEngineTemplateModifiers ..> FrontendCoreLanguageLocale : uses
FrontendCoreEngineTemplateModifiers ..> IntlDateFormatter : uses
FrontendCoreEngineTemplateModifiers ..> FrontendCoreLanguageLanguage : uses
BackendCoreEngineTemplateModifiers ..> BackendCoreLanguageLanguage : uses
BackendCoreEngineTemplateModifiers ..> IntlDateFormatter : uses
BackendCoreEngineTemplateModifiers ..> BackendCoreEngineAuthentication : uses
BackendCoreEngineAuthentication ..> User : returns
CommonCoreTwigExtensionsBaseTwigModifiers ..> BackendCoreLanguageLanguage : uses
CommonCoreTwigExtensionsBaseTwigModifiers ..> IntlDateFormatter : uses
FrontendCoreLanguageLanguage ..> FrontendCoreLanguageLanguage : calls getLabel
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The new IntlDateFormatter usages in the front- and backend TemplateModifiers replace user-/configurable date/time formats with hardcoded patterns (e.g. 'dd.MM.yyyy', 'EEEE d MMMM yyyy', 'HH:mm'), which changes behavior and ignores existing settings; consider deriving the pattern or style from the existing configuration instead of hardcoding.
- BaseTwigModifiers::spoonDate() now ignores its $format and $language parameters and always uses BackendLanguage::getInterfaceLanguage() with a fixed pattern, which breaks the previous contract and likely frontend usage; it should respect the passed format and language (or be clearly deprecated and replaced).
- The timeAgo() implementation no longer wraps the relative text in an with a title showing the exact date/time, changing the generated markup and losing the precise timestamp tooltip; if this is not intentional, consider preserving the previous HTML structure while still using the new labels.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new IntlDateFormatter usages in the front- and backend TemplateModifiers replace user-/configurable date/time formats with hardcoded patterns (e.g. 'dd.MM.yyyy', 'EEEE d MMMM yyyy', 'HH:mm'), which changes behavior and ignores existing settings; consider deriving the pattern or style from the existing configuration instead of hardcoding.
- BaseTwigModifiers::spoonDate() now ignores its $format and $language parameters and always uses BackendLanguage::getInterfaceLanguage() with a fixed pattern, which breaks the previous contract and likely frontend usage; it should respect the passed format and language (or be clearly deprecated and replaced).
- The timeAgo() implementation no longer wraps the relative text in an <abbr> with a title showing the exact date/time, changing the generated markup and losing the precise timestamp tooltip; if this is not intentional, consider preserving the previous HTML structure while still using the new labels.
## Individual Comments
### Comment 1
<location path="src/Common/Core/Twig/Extensions/BaseTwigModifiers.php" line_range="193" />
<code_context>
+ public static function spoonDate($timestamp, $format = 'Y-m-d H:i:s', $language = 'en')
</code_context>
<issue_to_address>
**issue (bug_risk):** spoonDate no longer respects its $format and $language parameters and always formats a date only.
Previously this delegated to `\SpoonDate::getDate($format, $timestamp, $language)`, which respected the provided format, language, and optional time component. The new version hardcodes `BackendLanguage::getInterfaceLanguage()`, ignores `$language` and `$format`, and always uses `'dd MMMM yyyy'`, changing behavior and potentially breaking existing templates relying on custom formats or date+time output. Please either (a) translate `$format` into an `IntlDateFormatter` pattern and use `$language` as the locale (falling back to styles when no format is given), or (b) keep `spoonDate` as a thin wrapper around the legacy behavior and introduce a separate Intl-based helper.
</issue_to_address>
### Comment 2
<location path="src/Frontend/Core/Engine/TemplateModifiers.php" line_range="36-42" />
<code_context>
- // format the date
- return SpoonDate::getDate($format, (int) $var, BackendLanguage::getInterfaceLanguage());
+ $date = new IntlDateFormatter(
+ BackendLanguage::getInterfaceLanguage(),
+ IntlDateFormatter::SHORT,
</code_context>
<issue_to_address>
**issue (bug_risk):** formatDate now ignores the configured frontend date format and uses a hardcoded pattern.
The previous implementation respected `Core.date_format_short` from `fork.settings`, enabling per-site date format configuration. The new hardcoded `'dd.MM.yyyy'` pattern changes existing behavior and ignores that CMS setting. If you still want to use Intl while honoring configuration, consider deriving the Intl pattern from the stored format or storing Intl-compatible patterns in settings. Otherwise, sites using custom date formats may see unexpected changes.
</issue_to_address>
### Comment 3
<location path="src/Backend/Core/Engine/TemplateModifiers.php" line_range="30-36" />
<code_context>
- // format the date
- return SpoonDate::getDate($format, (int) $var, BackendLanguage::getInterfaceLanguage());
+ $date = new IntlDateFormatter(
+ BackendLanguage::getInterfaceLanguage(),
+ IntlDateFormatter::SHORT,
+ IntlDateFormatter::NONE,
+ null,
+ null,
+ 'dd MMMM yyyy'
+ )->format((int) $var);
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Backend formatDate no longer respects per-user date format settings.
Previously this used `Authentication::getUser()->getSetting('date_format')`, so each user could control their own date format. The new code hardcodes `'dd MMMM yyyy'`, removing that per-user configurability. Please consider deriving the pattern from the user’s setting (e.g. storing Intl-compatible patterns or mapping legacy formats to Intl) instead of hardcoding it.
</issue_to_address>
### Comment 4
<location path="src/Frontend/Core/Engine/TemplateModifiers.php" line_range="250-254" />
<code_context>
+ return Language::lbl('TimeAgoEmpty');
+ }
+
+ if ($count === 1) {
+ return Language::lbl($keySingular);
+ }
+
+ return Language::lblWithParameters(
+ $keyPlural,
+ [
</code_context>
<issue_to_address>
**question:** timeAgo now returns plain text without the previous <abbr> wrapper or exact timestamp tooltip.
Previously, the relative time was wrapped in an `<abbr>` with a `title` showing the exact timestamp, aiding accessibility and precision on hover. The new version returns only the localized relative string. If this change wasn’t intentional, consider keeping the `<abbr>` wrapper (with the precise timestamp in `title`) while still using the new `Language` labels for the visible text.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| $date = new IntlDateFormatter( | ||
| Locale::frontendLanguage(), | ||
| IntlDateFormatter::SHORT, | ||
| IntlDateFormatter::NONE, | ||
| null, | ||
| null, | ||
| 'dd.MM.yyyy' |
There was a problem hiding this comment.
issue (bug_risk): formatDate now ignores the configured frontend date format and uses a hardcoded pattern.
The previous implementation respected Core.date_format_short from fork.settings, enabling per-site date format configuration. The new hardcoded 'dd.MM.yyyy' pattern changes existing behavior and ignores that CMS setting. If you still want to use Intl while honoring configuration, consider deriving the Intl pattern from the stored format or storing Intl-compatible patterns in settings. Otherwise, sites using custom date formats may see unexpected changes.
| $date = new IntlDateFormatter( | ||
| BackendLanguage::getInterfaceLanguage(), | ||
| IntlDateFormatter::SHORT, | ||
| IntlDateFormatter::NONE, | ||
| null, | ||
| null, | ||
| 'dd MMMM yyyy' |
There was a problem hiding this comment.
issue (bug_risk): Backend formatDate no longer respects per-user date format settings.
Previously this used Authentication::getUser()->getSetting('date_format'), so each user could control their own date format. The new code hardcodes 'dd MMMM yyyy', removing that per-user configurability. Please consider deriving the pattern from the user’s setting (e.g. storing Intl-compatible patterns or mapping legacy formats to Intl) instead of hardcoding it.
There was a problem hiding this comment.
Op zich ok voor mij. Maar dan ook de setting weghalen in de interface?
There was a problem hiding this comment.
Ja, als we hier bewust afscheid nemen van per-gebruiker datumformaten in de backend, is het logischer om die setting ook op te ruimen.
Concreet zou ik voorstellen:
-
Functioneel beslissen
- Bevestigen dat we in de backend voortaan altijd het interface-locale + een vaste (of per-locale) Intl-pattern gebruiken.
- Controleren of die
date_format-instelling nog elders in de backend wordt gebruikt (bijv. exports, logs, andere templates). Als niet, dan is verwijderen veilig.
-
Opruimen in de code
- De user-setting key
date_formatniet langer aanbieden in het gebruikersprofiel / instellingenformulier. - Eventuele referenties naar
getSetting('date_format')verwijderen of migreren naar het nieuwe IntlDateFormatter-gebaseerde pad.
- De user-setting key
-
Migratie / backwards compatibility
- Als je zeker weet dat
date_formatnergens meer gebruikt wordt, kunnen we hem stilzwijgend laten uitsterven (hij blijft misschien nog in de database staan maar heeft geen effect). - Of, als je het proper wil doen, een kleine migratie schrijven die de kolom/waarde opruimt of een upgrade note toevoegen.
- Als je zeker weet dat
Wil je dat ik even meekijk of er nog andere usages van date_format in dit project zijn, dan kunnen we beslissen of het een volledige cleanup moet worden of enkel het verbergen van de instelling in de UI.
| public static function formatDate($var): string | ||
| { | ||
| // get setting | ||
| $format = Authentication::getUser()->getSetting('date_format'); |
There was a problem hiding this comment.
Die setting wordt niet meer gebruikt?
| public static function formatDateTime($var): string | ||
| { | ||
| // get setting | ||
| $format = Authentication::getUser()->getSetting('datetime_format'); |
There was a problem hiding this comment.
Die setting wordt niet meer gebruikt?
| public static function formatTime($var): string | ||
| { | ||
| // get setting | ||
| $format = Authentication::getUser()->getSetting('time_format'); |
There was a problem hiding this comment.
Die setting wordt niet meer gebruikt?
There was a problem hiding this comment.
Die "format" wordt niet meer gebruikt?
| public static function formatDateTime($var): string | ||
| { | ||
| // get setting | ||
| $format = FrontendModel::get('fork.settings')->get('Core', 'date_format_long'); |
There was a problem hiding this comment.
Die setting wordt niet meer gebruikt?
| public static function formatTime($var): string | ||
| { | ||
| // get setting | ||
| $format = FrontendModel::get('fork.settings')->get('Core', 'time_format'); |
There was a problem hiding this comment.
Die setting wordt niet meer gebruikt?
| return ''; | ||
| } | ||
|
|
||
| // return |
There was a problem hiding this comment.
There was a problem hiding this comment.
Ik kreeg dat niet goed omdat dat met translatorinterface werkt voor die vertalingen, was moeilijk om te combineren met de locale.
| * | ||
| * @return string | ||
| */ | ||
| public static function lblWithParameters(string $key, array $parameters = [], bool $fallback = true): string |
There was a problem hiding this comment.
Moet je dat dan ook niet voorzien voor messages en errors?
| $date = new IntlDateFormatter( | ||
| BackendLanguage::getInterfaceLanguage(), | ||
| IntlDateFormatter::SHORT, | ||
| IntlDateFormatter::NONE, | ||
| null, | ||
| null, | ||
| 'dd MMMM yyyy' |
There was a problem hiding this comment.
Op zich ok voor mij. Maar dan ook de setting weghalen in de interface?
| public static function formatTime($var): string | ||
| { | ||
| // get setting | ||
| $format = Authentication::getUser()->getSetting('time_format'); |
There was a problem hiding this comment.
Niet logischer om dit te verwijderen. Dan is er geen referentie meer naar Spoon.
Denk dat je wel alle templates snel kan doorzoeken om te zien of het wordt gebruikt.
There was a problem hiding this comment.
Al denk ik dat het wel nuttig is om een method aan te bieden waarmee we het formaat kunnen meegeven. Niet?
| public static function formatDate($var): string | ||
| { | ||
| // get setting | ||
| $format = FrontendModel::get('fork.settings')->get('Core', 'date_format_short'); |
There was a problem hiding this comment.
Zou dan ook de setting weghalen in de interface
| public static function formatDateTime($var): string | ||
| { | ||
| // get setting | ||
| $format = FrontendModel::get('fork.settings')->get('Core', 'date_format_long'); |
| public static function formatTime($var): string | ||
| { | ||
| // get setting | ||
| $format = FrontendModel::get('fork.settings')->get('Core', 'time_format'); |
…aGridFunctions where they were still used.
…format to filter. fix twig-cs warnings on templates
Type
Pull request description
Replaces SpoonDate with IntlDateFormatter to format dates and timestamps.
Summary by Sourcery
Replace legacy date utilities with IntlDateFormatter-based Twig filters for consistent date, time, and relative time formatting across frontend, backend, and shared templates.
Enhancements:
Documentation: