Project

General

Profile

Bug #696

The function api_get_local_time() has problematic design

Added by Ivan Tcholakov over 9 years ago. Updated over 9 years ago.

Status:
Bug resolved
Priority:
Normal
Assignee:
-
Category:
-
Target version:
Start date:
10/03/2010
Due date:
% Done:

100%

Estimated time:
Complexity:
Normal
SCRUM pts - complexity:
?

Description

See main_api.lib.php,
function api_get_local_time($time, $format=null, $to_timezone=null, $from_timezone=null) {
...
}

Let us call the function this way:
echo api_get_local_time($time, null, null, date_default_timezone_get());

Results:
for PHP 5.2.x: 2010-03-10 06:40:58
for PHP 5.3: 10 март 2010, сряда 06:40 - localized result

Code that uses this function this way work well for PHP 5.2, but fails for PHP 5.3. It is because code parses the returned from this function result and parsing localized string was not predicted (and it is not desireable).

I noticed this effect in "Dropbox" and "Assignments", and without much thinking I adapted code for PHP 5.3. Later I realized the problem.

I will make a workaround for these two tools, but please, rework this API function, it surprises badly as it is now.


Files

php_5_2_dates.png (60.2 KB) php_5_2_dates.png Ivan Tcholakov, 12/03/2010 01:18
php_5_2_dates_ok_now.png (57.3 KB) php_5_2_dates_ok_now.png Ivan Tcholakov, 13/03/2010 00:32

Related issues

Related to Chamilo LMS - Feature #272: UTF-8 native supportFeature implemented03/12/2009

Actions

History

#1

Updated by Ivan Tcholakov over 9 years ago

Here is the test statement.

echo api_get_local_time('2010-03-10 06:40:58', null, null, date_default_timezone_get());

Play with it on PHP 5.2.x and PHP 5.3 and see the difference.

#2

Updated by Ivan Tcholakov over 9 years ago

Here is the solution I intend to apply. The special PHP 5.3 case for date formatting will be disabled in api_get_local_time(), so it not to bring surprises. For that special formatting, call explicitly the function api_format_date_time_long() which uses date formatter from Intl for PHP 5.3, and for PHP 5.2.x provides a similar, close format.

I will be waiting for several hours before taking this task.

#3

Updated by Anonymous over 9 years ago

Hi Ivan,

In fact, this function does not work the same way in PHP 5.2 and PHP 5.3 and this is not right. It was an intent from me to handle both the localization and the conversion to the right timezone in the same function.

However, before doing any more changes and adding more functions, I think we should think a little bit more.

There are two things to handle concerning dates:
  • The conversion to the right timezone
  • The localization of the date

Concerning the conversion to the right timezone, I believe api_get_local_time handles it well, if we remove the block of code that formats the date. What I would like to do would be to modify api_get_local_time so that it takes care of the timezone conversion ONLY, and no more. The function would therefore always return a SQL datetime, and would have the following prototype:

function api_get_local_time($time, $to_timezone, $from_timezone)

Concerning the localization of the dates, we agreed with Yannick that we would support it only for users using PHP 5.3 or higher. The reason for this is that a full localization of the dates is hard to achieve in PHP 5.2 or lower, while it's just a matter of a few lines of code in PHP 5.3, using IntlDateFormatter. Concerning the localization of the date, instead of adding a new function api_format_date_time_long, I would opt for a refactoring of api_format_date, adding an if (IS_PHP_53) inside the function. Let me know what you think...

I also think it would be handy to create a function named for example api_convert_and_format_date, which would do both the timezone conversion and the localization, and would have the following prototype:

function api_convert_and_format_date($time, $format, $from_timezone)

When simply displaying a datetime coming from mysql, developers would use this function...

Finally, one last thing which concerns date functions such as date_to_str_ago. These functions will probably need to be rewritten, as they use date(), which uses the default timezone of the server... I'll investigate more on this... Do you know of any other api functions that handle dates ?

Thank you and let me know what you think (Yannick as well)

#4

Updated by Anonymous over 9 years ago

  • Assignee set to Anonymous
#5

Updated by Ivan Tcholakov over 9 years ago

Hello Guillaume,

Maybe it is not a good time for radical changes, the beta is coming soon. I am a little bit tired...

I think that separate implementation of timezone converting and date/time formatting as different functions is the best idea at all. Complex and combined functions are difficult to be used, to be controlled by the developers and to be tested.

What I proposed is the most economical approach, it saves efforts. We have all we need, the formatting function and the function api_get_local_time() for the timezones, let us deactivate the formatting code for PHP 5.3. I moved this special code for formatting (because I like it) in the function api_format_date_time_long(), which gives nice formatted output for PHP 5.2.x and PHP 5.3. You only add there your name as a co-author.

Please, don't touch the functions api_format_date_time_long() and api_format_date(). Adding new functions is not necessary too. We have the promises about UTF-8, timezones, let us focus on them only. The solution might look dirty for now, but we have enough dirty code anyway. :-)

For 1.8.7.1 I propose to make clean code, well designed functions and to remove some garbage. It will be more calm situation, we'll see how popular PHP 5.3 will be and decisions will be easier.

I have very clear idea for a swift adaptation right now, let me do it. Please, give this task to me, you will see what I mean.

Ivan.

#6

Updated by Anonymous over 9 years ago

Hi Ivan,

What I don't like about the function api_format_date_time_long is that it's a new function that manages dates, and we already have one function that's supposed to do the same as api_format_date_time_long, which is api_format_date... So I'm still advocating the idea of removing the api_format_date_time_long and modifying api_format_date instead...

I would also like to add a new function, called api_convert_and_format_date, because currently, if you just want to display a date that comes out of the database, you need to call two functions: api_get_local_time first, and then api_format_date or api_format_date_time_long. So, just to make it easier, it would be nice to have a function that does both (convert the time to the right timezone and localize the date).

Finally, are you sure there is a need for a call to api_to_system_encoding before returning the formatted date in api_format_date_time_long ? I'm pretty sure that the result returned by datefmt_format is already UTF-8...

#7

Updated by Yannick Warnier over 9 years ago

Hi Ivan,

I discussed this a little with Guillaume, who seems keen to improve his current function with the code from the _long() version. I have no particular problem with that, he'll take in charge changing the function and the places where the _long() version has been used.
Personally, I highly approve your efforts to make it 5.2 and 5.3 compatible, and I think Guillaume does too. However, we've already started to have problems with other developers not following the recommendations in terms of date usage, so I think it's best to reduce the possibility of misinterpretation and provide only one function that provides the features of both. As I know now that you don't like arguing much (I don't either), I'll have Guillaume take that in charge right away.

On another related problem, what do you think of moving all database times and dates to UTC? Any particular vision on that? I'm hesitant because this will probably cause a lot of processing when migrating to 1.8.6.2 to 1.8.7, but the more I think of it, the less I find it would be a better idea to wait for the move between 1.8.7 and 1.8.7.1 to do it...

#8

Updated by Anonymous over 9 years ago

Hi Ivan,

I'm going to modify the format of the function api_get_local_time so that it only does the timezone conversion and no formatting at all (because I think we both agree on that). I will also, obviously, modify the calls to this function so that they match the new prototype. The new prototype of the function will be like this:

function api_get_local_time($time, $to_timezone=null, $from_timezone=null)

Concerning the date formatting part, I'll wait for your green light before changing anything...

#9

Updated by Ivan Tcholakov over 9 years ago

Guillaume: Finally, are you sure there is a need for a call to api_to_system_encoding before returning the formatted date in api_format_date_time_long ? I'm pretty sure that the result returned by datefmt_format is already UTF-8...

Of course, we will encourage the adopters to go UTF-8, but it is still their choice. The period is transitional, old habits die hard, we don't force people to make the encoding switch for this release.

Make an experiment with a UTF-8 system. Create a French or Spanish course. Go to the administrator panel, "Languages" tab and change the platform charset as ISO-8859-15. Go back to your course, its text should be Ok. In the case you are asking about api_to_system_encoding() just passes the string for UTF-8 system, but for non-UTF-8 system it does really encoding conversion.

Similar logic you may find in many places. In fact, this technique makes the system is not just UTF-8 ready, the system becomes, how to say, a "multi-encoding" capable. This capability is for serving the encoding transition. The little experiment I described may be done in the opposite direction: from ISO-8859-15 (an upgraded system for example, it may work this way some time) to UTF-8 (our goal, the system should continue to work without problems).

Technically saying: The system may be UTF-8, it may be non-UTF-8 too, so code should manage both cases.

Yannick: On another related problem, what do you think of moving all database times and dates to UTC? Any particular vision on that? I'm hesitant because this will probably cause a lot of processing when migrating to 1.8.6.2 to 1.8.7, but the more I think of it, the less I find it would be a better idea to wait for the move between 1.8.7 and 1.8.7.1 to do it...

Storing UTC-times in database is a good idea and as far as I know, a promise has been given. I think, this should be done for 1.8.7 release, even with the risk of some delay. The momentum now is good.

Guillaume: ... Concerning the date formatting part, I'll wait for your green light before changing anything...

Change everything at your will. I have too strong habit to trust myself only. :-) Let me break it this time.

#10

Updated by Anonymous over 9 years ago

Ivan > concerning UTF-8, you're right, I had not thought about the fact that some systems needed to go from UTF-8 to ISO-8859-1, but then it means that the same thing is needed for the api_format_date function right ?...

Otherwise, here is what I'm planning to do:

function api_get_local_time($time, $to_timezone=null, $from_timezone=null) {
    // Determining the timezone to be converted from
    if ($from_timezone === null) {
        $from_timezone = 'UTC';
    }
    // Determining the timezone to be converted to
    if ($to_timezone === null) {
        $to_timezone = _api_get_timezone();
    }
    // If time is a timestamp, convert it to a string
    if (is_int($time)) {
        $time = date('Y-m-d H:i:s', $time);
    }
    try {
        $date = new DateTime($time, new DateTimezone($from_timezone));
        $date->setTimezone(new DateTimeZone($to_timezone));
        return $date->format('Y-m-d H:i:s');
    } catch (Exception $e) {
        return null;
    }
}

function api_format_date($time, $format = null, $language = null) {

    if (is_string($time)) {
        $time = strtotime($time);
    }

    if ($format === null) {
        $format = DATE_TIME_FORMAT_LONG;
    }

    $datetype = null;
    $timetype = null;
    if(is_int($format)) {
        switch ($format) {
            case TIME_NO_SEC_FORMAT:
                $date_format = get_lang('timeNoSecFormat', '', $language);
                if (IS_PHP_53) {
                    $datetype = IntlDateFormatter::NONE;
                    $timetype = IntlDateFormatter::SHORT;
                }
                break;
            case DATE_FORMAT_SHORT:
                $date_format = get_lang('dateFormatShort', '', $language);
                if (IS_PHP_53) {
                    $datetype = IntlDateFormatter::LONG;
                    $timetype = IntlDateFormatter::NONE;
                }
                break;
            case DATE_FORMAT_LONG:
                $date_format = get_lang('dateFormatLong', '', $language);
                if (IS_PHP_53) {
                    $datetype = IntlDateFormatter::FULL;
                    $timetype = IntlDateFormatter::NONE;
                }
                break;
            case DATE_TIME_FORMAT_LONG:
                $date_format = get_lang('dateTimeFormatLong', '', $language);
                if (IS_PHP_53) {
                    $datetype = IntlDateFormatter::FULL;
                    $timetype = IntlDateFormatter::SHORT;
                }
                break;
            case DATE_FORMAT_LONG_WITHOUT_DAY:
                $date_format = get_lang('DateFormatLongWithoutDay', '', $language);
                break;
            default:
                $date_format = get_lang('dateTimeFormatLong', '', $language);
                if (IS_PHP_53) {
                    $datetype = IntlDateFormatter::FULL;
                    $timetype = IntlDateFormatter::SHORT;
                }
        }
    }

    if (IS_PHP_53 && INTL_INSTALLED && $datetype !== null && $timetype !== null) {
        // Use ICU
        if ($language == null) {
            $language = api_get_language_isocode();
        }
        $date_formatter = datefmt_create($language, $datetype, $timetype);
        return api_to_system_encoding(datefmt_format($date_formatter, $time), 'UTF-8');
    } else {
        // We replace %a %A %b %B masks of date format with translated strings.
        $translated = &_api_get_day_month_names($language);
        $date_format = str_replace(array('%A', '%a', '%B', '%b'),
        array($translated['days_long'][(int)strftime('%w', $time_stamp)],
            $translated['days_short'][(int)strftime('%w', $time_stamp)],
            $translated['months_long'][(int)strftime('%m', $time_stamp) - 1],
            $translated['months_short'][(int)strftime('%m', $time_stamp) - 1]),
        $date_format);
        return api_to_system_encoding(strftime($date_format, $time_stamp), 'UTF-8');
    }
}
function api_convert_and_format_date($time, $format = null, $from_timezone = null) {
    // First, convert the datetime to the right timezone
    $datetime = api_get_local_time($time, null, $from_timezone);
    // Second, localize the date
    return api_format_date($time, $format);
}

I'm also planning, obviously, to change all the current calls to api_format_date so that they fit the new prototype, and probably remove the old deprecated format_locale_date function as well... Let me know what you think...

#11

Updated by Ivan Tcholakov over 9 years ago

... but then it means that the same thing is needed for the api_format_date function right ?...

The function yo are asking about returns human-language formatted string using the system encoding.

About code you presented I have one request only:

Replace constructs as

if ($format === null) {

or

if ($language == null) {

with

if (is_null($format)) {
if (is_null($language)) {
...

#12

Updated by Anonymous over 9 years ago

Please see http://code.google.com/p/chamilo/source/detail?r=85aeef470108935650fecdcf36cedd73eea96bd3&repo=classic

On saturday, I will:
- write a tutorial on the wiki to explain how the date functions should be used
- get rid of format_locale_date
- write tests for the date functions

#13

Updated by Ivan Tcholakov over 9 years ago

I examined how new code works in "Assignments" tools. For PHP 5.3 dates are Ok, but
when I atificially simulated PHP 5.2, dates about 40 years ago are shown. See the picture.
I think, some minor correction is needed for the PHP 5.2 branch of code.

As a concept and definitions the API looks well. You prefer, let me call it
"swiss-army-knife" design for the functions which might have its ground too.

Especially I like the part to "get rid of format_locale_date()".

Some ideas (if you like them):
- yet another constant to be defined, for example DATE_FORMAT_INTEGER
for conversion from 'Y-m-d ....' to PHP's integer;
- the function date_to_str_ago() to be moved in the intednationalization
library, within "Date and time formats" section, this is the natural place;
- In a separate section in the internationalization library you may move your
timezone conversion functions. Why? In the past I had two libraries called
"multibyte_string_functions.lib.php" and "internationalization.lib.php". Later
I realized that it is all about internationalization, so I merged these libraries.
- the function format_locale_date() to be declared in the code as deprecated.

#14

Updated by Ivan Tcholakov over 9 years ago

For the tests there is a file ....chamilo/tests/main/inc/lib/internationalization.lib.test.php . The test functions are written by me. If you wonder how to use some of the internationalization functions, the correspondent test is a good example, an illustration.

#15

Updated by Ivan Tcholakov over 9 years ago

I can see that Christian Fasanando has fixed the bug I've mentioned. Dates under PHP 5.2 are Ok now.
http://code.google.com/p/chamilo/source/detail?r=b4c4f29df98182d06387f77fd7e3d2eb6e1a76b2&repo=classic

#16

Updated by Anonymous over 9 years ago

The tutorial on the wiki is available hereÑ
http://support.chamilo.org/projects/chamilo-18/wiki/Date_and_time_management

Ivan > I don't think the idea of declaring a new format DATE_FORMAT_INTEGER is a good one. You will understand why when reading the tutorial on the wiki. Instead, I created a function called api_strtotime, which is essentially a "timezone safe" strtotime.
I will move the functions to the right places, as you advise.
I'm planning on not only declaring format_locale_date as deprecated, but completely getting rid of it (i. e. replacing all the calls to format_locale_date by a call to api_format_date) in the code.

#17

Updated by Anonymous over 9 years ago

Note for myself: there are dates under the int format in mysql in the gradebook tables

#18

Updated by Anonymous over 9 years ago

Ivan > I got rid of all the format_locale_date calls (I obviously replaced them by calls to api_format_date or api_convert_and_format_date). All the changes I made, however, might have caused some bugs (in theory no, but practice is always different from theory ;-)). So, if you notice any dates that are badly displayed, let me know... I will inform the peruvian team of that as well.

#19

Updated by Ivan Tcholakov over 9 years ago

Ok.

There is a small detail to be discussed.

api_ucfirst(api_convert_and_format_date($myrow['end_date'], ......

In the past it was ucfirst(...), later it became api_ucfirst(...). Somebody decided that it would be the easy way for fixing spelling about English (or Spanish?), but this in not the correct way for other languages. The better approach is that api_convert_and_format_date() "knows" how to spell correctly dates for different languages.

I suggest this surrounding api_ucfirst() function call to be removed. If you are agreed, I can do that, you needn't waste your time.

#20

Updated by Anonymous over 9 years ago

Hi Ivan,

I totally agree :-)

#21

Updated by Ivan Tcholakov over 9 years ago

10975:69f584709037 Task #696 - Removing api_ucfirst() calls that affect formatting dates for different languages.
http://code.google.com/p/chamilo/source/detail?r=69f5847090372337164cf4a4f754c35a88ed322a&repo=classic

I have nothing else to say. Have a nice weekend.

#22

Updated by Yannick Warnier over 9 years ago

  • Status changed from New to Bug resolved
  • Target version set to 1.8.7 beta
  • % Done changed from 0 to 100

I guess this can be closed then...

Also available in: Atom PDF