Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix IntlExtension::formatDateTime uses prototype pattern if set #3845

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

drjayvee
Copy link
Contributor

@drjayvee drjayvee commented Jun 5, 2023

See twigphp/intl-extra#7 for details

@admdly
Copy link

admdly commented Nov 24, 2023

Is there any chance this could get reviewed/merged? @fabpot

We've been having this issue over at FOSSBilling/FOSSBilling for a while, and it would be great to get a fix in place.

@drjayvee
Copy link
Contributor Author

Hey Adam,

Could you please write a test for this? My other PR (#3844) was blocked until test coverage was added (and rightly so).

It should be really easy to copy one of the tests and adjust it slightly.

@BelleNottelling
Copy link

Sadly this patch goes and breaks the usage of the IntlDateFormatter as in my testing the options set there are no longer respected. Appling the patch and using format_datetime with the date format set to none still displays a date. Likewise setting the time format to none still shows a time.

So it "fixes" the error by instead breaking it which is a shame as the current problem where format_date and format_datetime both behave identically by only following the set pattern is quite frustrating.

So with the existing twig releases format_date will still display a time if a time format is set. We also can't workaround that by manually specifying a format for a given filter as it's ignored. With this patch that behavior goes away, but seemingly because what's set via the IntlDateFormatter is completely ignored.

@drjayvee
Copy link
Contributor Author

drjayvee commented Apr 2, 2024

@BelleNottelling I'm afraid I don't quite follow. It's been a while since I've looked at this bug/PR. In fact, my original explanation is from September 2021. It's possible the behavior has changed since then.

Can you show some actual code that breaks only with this PR's code? Looking at the diff, it only affects behavior when

  1. a dateFormatterPrototype is configured, and
  2. neither dateFormat or timeFormat arguments are provided (to either filter)

Likewise setting the time format to none still shows a time.

I don't see how this PR's diff affects formatting in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants