Skip to content

fix: Time::setTestNow() does not work with fa Locale - #6116

Merged
kenjis merged 1 commit into
codeigniter4:developfrom
kenjis:fix-Time-setTestNow-bug
Jun 14, 2022
Merged

fix: Time::setTestNow() does not work with fa Locale#6116
kenjis merged 1 commit into
codeigniter4:developfrom
kenjis:fix-Time-setTestNow-bug

Conversation

@kenjis

@kenjis kenjis commented Jun 13, 2022

Copy link
Copy Markdown
Member

Description

ErrorException : DateTime::modify(): Failed to parse time string (۲۰۱۷-۰۳-۱۰ ۰۰:۰۰:۰۰) at position 0 (�): Unexpected character
 .../CodeIgniter4/system/I18n/Time.php:96

Checklist:

  • Securely signed commits
  • [] Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • [] User guide updated
  • Conforms to style guide

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Jun 13, 2022
@kenjis
kenjis force-pushed the fix-Time-setTestNow-bug branch from 5d2e49c to 457d5ec Compare June 13, 2022 09:54
@MGatner

MGatner commented Jun 13, 2022

Copy link
Copy Markdown
Member

I'm not sure what is happening with Coveralls. Maybe it is getting confused about the different branches?

@MGatner MGatner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually now I'm not sure about this. The User Guide is very clear that toLocalizedString() is for localization whereas toDateTimeString() "will return a string formatted as you would commonly use for datetime columns in a database (Y-m-d H:i:s)" (https://codeigniter4.github.io/userguide/libraries/time.html#todatetimestring).

Reading that makes me think the problem is actually using the localization in toDateTimeString().

@kenjis

kenjis commented Jun 13, 2022

Copy link
Copy Markdown
Member Author

It is difficult to read but I think toDateTimeString() is localized version of Y-m-d H:i:s.

This is the first of three helper methods to work with the IntlDateFormatter without having to remember their values. This will return a string formatted as you would commonly use for datetime columns in a database (Y-m-d H:i:s):
https://codeigniter4.github.io/CodeIgniter4/libraries/time.html?highlight=time#todatetimestring

It is clear if you see the implementation.

@MGatner

MGatner commented Jun 13, 2022

Copy link
Copy Markdown
Member

You're right! But the UG is still confusing: I would have used toDateTimeString() here as well. I wonder if we need a new method specifically for non-localized database values? Is there ever a case where your timestamp values need to be localized for storage input?

@MGatner

MGatner commented Jun 13, 2022

Copy link
Copy Markdown
Member

After some reading it looks like DATETIME/TIMESTAMP inputs must always be decimal. I think we need to add toDatabase() or something like it and update the User Guide so developers aren't accidentally using localized strings for database input.

@datamweb

Copy link
Copy Markdown
Contributor

Friends just for information, the output of command toLocalizedString() is also UTF-8 string (33) "۲۰۲۲-۰۶-۱۳ ۱۷:۳۰:۱۳".

@paulbalandan

Copy link
Copy Markdown
Member

Is this related to here?
#4708

@kenjis

kenjis commented Jun 14, 2022

Copy link
Copy Markdown
Member Author

@paulbalandan Yes. The issue is because of the same bug.

ErrorException : DateTime::modify(): Failed to parse time string (۲۰۱۷-۰۳-۱۰ ۰۰:۰۰:۰۰) at position 0 (�): Unexpected character
 .../CodeIgniter4/system/I18n/Time.php:96
@kenjis

kenjis commented Jun 14, 2022

Copy link
Copy Markdown
Member Author

But the UG is still confusing

I sent a PR #6121

@kenjis kenjis mentioned this pull request Jun 14, 2022
3 tasks

@MGatner MGatner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, I am on board - this is definitely e right direction. 4.3 feature request: toDatabase()

@kenjis
kenjis merged commit 8e5360c into codeigniter4:develop Jun 14, 2022
@kenjis
kenjis deleted the fix-Time-setTestNow-bug branch June 14, 2022 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Verified issues on the current code behavior or pull requests that will fix them

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants