NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Intl] Add`DateIntervalFormatter`#39912

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

YaFou wants to merge1 commit intosymfony:5.4fromYaFou:date-interval-formatter

Closed

[Intl] Add`DateIntervalFormatter`#39912

YaFou wants to merge1 commit intosymfony:5.4fromYaFou:date-interval-formatter

Conversation

Copy link

Contributor

YaFou commentedJan 20, 2021•
edited
Loading

Q	A
Branch?	5.x
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix#29929
License	MIT
Doc PR	TODO

$formatter =newDateIntervalFormatter();$formatter->formatInterval(new \DateInterval('P1DT5H2M'));// one day, 5 hours and 2 minutes

You can set a precision. The precision cuts the formatted content to a given number of elements:

$formatter =newDateIntervalFormatter();$formatter->formatInterval(new \DateInterval('P1DT20H2M'),2);// one day and 20 hours

carsonbot added Status: Needs Review Translation Feature labels

Jan 20, 2021

carsonbot changed the title~~[Translation] [WIP] Add DateIntervalFormatter~~[Translator] [WIP] Add DateIntervalFormatter

Jan 20, 2021

YaFou force-pushed thedate-interval-formatter branch 2 times, most recently from0169445 to5189d2dCompare

January 20, 2021 20:54

YaFou mentioned this pull request

Jan 20, 2021

[Translation] Add human readable DateInterval#29929

Closed

OskarStark reviewed

Jan 21, 2021

View reviewed changes

src/Symfony/Component/Translation/CHANGELOG.md OutdatedShow resolvedHide resolved

OskarStark reviewed

Jan 21, 2021

View reviewed changes

src/Symfony/Component/Translation/Formatter/DateIntervalFormatter.php OutdatedShow resolvedHide resolved

Copy link

Contributor

ro0NL commentedJan 21, 2021

there's an interesting ICU concept: listPattern
https://github.com/unicode-org/icu/blob/6092942e23785d85de5b40ea2f47768f7fa9345b/icu4c/source/data/locales/en.txt#L2007

relative/duration time formats
https://github.com/unicode-org/icu/blob/6092942e23785d85de5b40ea2f47768f7fa9345b/icu4c/source/data/locales/en.txt#L1441-L1456
https://github.com/unicode-org/icu/blob/6092942e23785d85de5b40ea2f47768f7fa9345b/icu4c/source/data/unit/en.txt#L286

ideally we'd implement this at the ICU message format level 😁 but that's far fetched.

overall, having a generic localized interval formatter in the Intl component seems more useful.

Copy link

Contributor

fancyweb commentedJan 21, 2021•
edited
Loading

overall, having a generic localized interval formatter in the Intl component seems more useful.

I agree and we could use it in the Validator component too for#33401

YaFou force-pushed thedate-interval-formatter branch from2888345 to7f6cbbaCompare

January 21, 2021 10:04

YaFou requested a review fromyceruto as acode owner

January 21, 2021 10:04

YaFou force-pushed thedate-interval-formatter branch 2 times, most recently fromb43f917 to0ed15bdCompare

January 21, 2021 10:08

nicolas-grekas added this to the5.x milestone

Jan 21, 2021

OskarStark reviewed

Jan 27, 2021

View reviewed changes

src/Symfony/Component/Intl/Tests/DateIntervalFormatter/DateIntervalFormatterTest.phpShow resolvedHide resolved

YaFou changed the title~~[Translator] [WIP] Add DateIntervalFormatter~~[Intl] [WIP] Add DateIntervalFormatter

Jun 26, 2021

YaFou force-pushed thedate-interval-formatter branch 5 times, most recently from1c09134 to894cc36Compare

June 26, 2021 09:01

Copy link

ContributorAuthor

YaFou commentedJun 26, 2021

Ready for review

YaFou changed the title~~[Intl] [WIP] Add DateIntervalFormatter~~[Intl] Add DateIntervalFormatter

Jun 26, 2021

YaFou requested a review fromOskarStark

June 26, 2021 09:03

[Translation] Add DateIntervalFormatter

86a078a

YaFou force-pushed thedate-interval-formatter branch from894cc36 to86a078aCompare

June 26, 2021 09:05

HeahDude reviewed

Jun 26, 2021

View reviewed changes

src/Symfony/Bridge/Twig/Extension/IntlExtension.php

		useTwig\Extension\AbstractExtension;
		useTwig\TwigFilter;

		class IntlExtensionextends AbstractExtension

Copy link

Contributor

HeahDudeJun 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What about doing this inhttps://github.com/twigphp/intl-extra/blob/2.x/IntlExtension.php instead?
Currently it may conflict withhttps://github.com/twigphp/twig-extra-bundle/blob/2.x/Resources/config/intl.php.

Copy link

Member

nicolas-grekasOct 18, 2021•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think@HeahDude's idea is a good one. Can you please remove that code from this PR and move it to twig instead?

Copy link

Contributor

sstok commentedJul 4, 2021

FYIhttps://carbon.nesbot.com/docs/#api-interval provides a fully localized version of DateInterval withforHumans().

OskarStark changed the title~~[Intl] Add DateIntervalFormatter~~[Intl] AddDateIntervalFormatter

Aug 4, 2021

Copy link

Contributor

OskarStark commentedAug 4, 2021

Open to finish this PR@YaFou ?

Copy link

ContributorAuthor

YaFou commentedAug 4, 2021

Open to finish this PR@YaFou ?

For me, it is finished. I saw comments which suggest to add ICU but I don't think we have to add something to big to Symfony core.

nicolas-grekas reviewed

Oct 18, 2021

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm mixed here: I get the value, but what about i18n? Is the language designed to be an implementation detail or should the interface allow declaring it?

In the end, my question ends up as: shouldn't we delegate this to a 3rd party lib, eg Carbon?

src/Symfony/Bridge/Twig/Extension/IntlExtension.php

		useTwig\Extension\AbstractExtension;
		useTwig\TwigFilter;

		class IntlExtensionextends AbstractExtension

Copy link

Member

nicolas-grekasOct 18, 2021•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think@HeahDude's idea is a good one. Can you please remove that code from this PR and move it to twig instead?

src/Symfony/Bundle/FrameworkBundle/Resources/config/translation.php

		->tag('container.service_subscriber', ['id' =>'translator'])
		->tag('kernel.cache_warmer')

		->set('translation.formatter.date_interval', DateIntervalFormatter::class)

Copy link

Member

nicolas-grekasOct 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I don't get the need for this service

src/Symfony/Bundle/FrameworkBundle/Resources/config/intl.php

		returnstaticfunction (ContainerConfigurator$container) {
		$container->services()
		->set('intl.date_interval_formatter', DateIntervalFormatter::class)
		->alias(DateIntervalFormatterInterface::class,'intl.date_interval_formatter')

Copy link

Member

nicolas-grekasOct 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

alias should be unindented by one

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

		'kernel.reset',
		]);

		if (class_exists(DateIntervalFormatterInterface::class)) {

Copy link

Member

nicolas-grekasOct 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

interface_exists, since it's not a class but an interface

src/Symfony/Component/Intl/CHANGELOG.md

		5.4
		---

		* Add`DateIntervalFormatter` to humanize a`DateInterval` or a difference between two`DateTimeInterface` objects

Copy link

Member

nicolas-grekasOct 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

missing space at start of line

src/Symfony/Component/Intl/DateIntervalFormatter/DateIntervalFormatterInterface.php

		/**
		* @param \DateInterval\|string $interval
		*/
		publicfunctionformatInterval($interval,int$precision =0):string;

Copy link

Member

nicolas-grekasOct 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

what is $precision supposed to mean? reading the interface should provide a clue

Copy link

Member

fabpot commentedOct 30, 2021

Having something like this in core without i18n is a no-go for me. As soon as we will merge it, people will start asking for it and they will be right.
Let's close for now.