Skip to content

Adds DateTime[Immutable]::createFromTimestamp #12413

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

Closed
wants to merge 5 commits into from

Conversation

marc-mabe
Copy link
Contributor

@marc-mabe marc-mabe commented Oct 11, 2023

This creates the following functions:

DateTime::createFromTimestamp(int|float $timestamp): static
DateTimeImmutable::createFromTimestamp(int|float $timestamp): static

The reason for these functions is the very common case of creating a DateTime[Immutable] object from a unix timestamp.
This currently only works by:

  • special timestamp constructor format @<timestamp>
  • createFromFormat with U / U.u
    • same as @<timstamp> but adds additional differences on integer vs. float
  • (new DateTime(...))->setTimestamp()
    • needs instance beforehand which does a current time lookup
    • setTimestamp supports integers only (This probably should allow floats as well in a separate PR)

The new functions avoid the above issues, are self explained and are much faster for this common case as well.

The functions will throw a DateRangeError in case of special floating point numbers or out-of-range of signed long long of timelib.

Internals: https://marc.info/?l=php-internals&m=169850186312326

Bench (doesn't even handle scientific notation of floating points):

<?php

$calls = 10000;
$time = time();
$microtime = microtime(true);
$utc = new DateTimeZone('+00:00');

function p(float $start, float $end, int $calls) {
    printf(
        "\t%ss for %s calls (%Fns per call)\n",
        ($end - $start) / 1000000000,
        $calls,
        ($end - $start) / $calls,
    );
}

foreach ([$time, $microtime] as $t) {
    echo '$t = ' . var_export($t, true) . "\n";

    echo 'new DateTime("@" . $t, $utc): ' . "\n";
    $start = hrtime(true);
    for ($i = 0; $i < $calls; $i += 1) {
        $tmp = new DateTime("@" . $t, $utc);
    }
    $end = hrtime(true);
    p($start, $end, $calls);
    //var_dump($tmp);

    if (is_float($t)) {
        echo 'DateTime::createFromFormat("U.u", $t, $utc): ' . "\n";
        $start = hrtime(true);
        for ($i = 0; $i < $calls; $i += 1) {
            $tmp = DateTime::createFromFormat("U.u", $t, $utc);
        }
        $end = hrtime(true);
        p($start, $end, $calls);
    } else {
        echo 'DateTime::createFromFormat("U", $t, $utc): ' . "\n";
        $start = hrtime(true);
        for ($i = 0; $i < $calls; $i += 1) {
            $tmp = DateTime::createFromFormat("U", $t, $utc);
        }
        $end = hrtime(true);
        p($start, $end, $calls);
    }
    //var_dump($tmp);

    echo 'DateTime::createFromTimestamp($t): ' . "\n";
    $start = hrtime(true);
    for ($i = 0; $i < $calls; $i += 1) {
        $tmp = DateTime::createFromTimestamp($t);
    }
    $end = hrtime(true);
    p($start, $end, $calls);
    //var_dump($tmp);
}
$t = 1697016217
new DateTime("@" . $t, $utc): 
        0.012929698s for 10000 calls (1292.969800ns per call)
DateTime::createFromFormat("U", $t, $utc): 
        0.009363419s for 10000 calls (936.341900ns per call)
DateTime::createFromTimestamp($t): 
        0.004168669s for 10000 calls (416.866900ns per call)
$t = 1697016217.492358
new DateTime("@" . $t, $utc): 
        0.013072768s for 10000 calls (1307.276800ns per call)
DateTime::createFromFormat("U.u", $t, $utc): 
        0.012962979s for 10000 calls (1296.297900ns per call)
DateTime::createFromTimestamp($t): 
        0.00396879s for 10000 calls (396.879000ns per call)

@marc-mabe marc-mabe force-pushed the DateTime-createFromTimestamp branch from 1ac0743 to c7cc823 Compare October 18, 2023 20:26
@marc-mabe marc-mabe force-pushed the DateTime-createFromTimestamp branch from d517b20 to c7cc823 Compare October 28, 2023 10:07
@@ -362,6 +368,12 @@ public static function createFromInterface(DateTimeInterface $object): DateTime
*/
public static function createFromFormat(string $format, string $datetime, ?DateTimeZone $timezone = null): DateTime|false {}

/**
* @tentative-return-type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tentative-return-type makes Carbon compatible with deprecation message

@kamil-tekiela
Copy link
Member

The functions can only return false in case of special floating point numbers or not within range of zend_long.

The signatures you listed do not contain false as a possible return type. Why do these functions return false at all? Why not just throw an exception?

@marc-mabe
Copy link
Contributor Author

The functions can only return false in case of special floating point numbers or not within range of zend_long.

The signatures you listed do not contain false as a possible return type. Why do these functions return false at all? Why not just throw an exception?

@kamil-tekiela Thanks for pointing this out. It has been updated already to throw a DateRangeError as visible in the test. Updated the PR description accordingly.

@marc-mabe
Copy link
Contributor Author

@derickr I have updated the PR according to your review

Copy link
Member

@derickr derickr left a comment

Choose a reason for hiding this comment

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

I think this looks good, but I have one question left.

["timezone"]=>
string(6) "+00:00"
}
DateTime::createFromTimestamp(%f): DateRangeError: Seconds must be a finite number between %i and %i, %f given
Copy link
Member

Choose a reason for hiding this comment

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

Why do you say finite here, and not integer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the word "finite" to cover NAN, +/-INF as well and as a FP is allowed as well "integer" would not be fully correct.
If you feel it's confusing it could also be left out -> "... must be a number between ..."

derickr added a commit that referenced this pull request Nov 22, 2023
@derickr
Copy link
Member

derickr commented Nov 22, 2023

I have merged this after a rebase, squash, and the addition of the NEWS entry. Thanks!

@derickr derickr closed this Nov 22, 2023
@marc-mabe marc-mabe deleted the DateTime-createFromTimestamp branch November 22, 2023 15:53
@derickr
Copy link
Member

derickr commented Nov 23, 2023

@marc-mabe Turns out that this failed tests on x32, can you have a look? https://github.com/php/php-src/actions/runs/6964523973/job/18951903197

@marc-mabe
Copy link
Contributor Author

@marc-mabe Turns out that this failed tests on x32, can you have a look? https://github.com/php/php-src/actions/runs/6964523973/job/18951903197

I'll have a look ... Could you give me a hint for how I can test this without x32 arch 🙏

@derickr
Copy link
Member

derickr commented Nov 23, 2023

I run compile my 32-bit build like this:

https://github.com/derickr/php-build/blob/master/build#L68
https://github.com/derickr/php-build/blob/master/build#L138

You can probably get away with just --disable-all.

I guess, this would be minimal:

export CFLAGS="-m32"
export PKG_CONFIG_PATH="/usr/lib/i386-linux-gnu/pkgconfig"
./buildconf
./configure --build=i686-pc-linux-gnu --disable-all
make
make test TESTS=ext/date

marc-mabe added a commit to marc-mabe/php-src that referenced this pull request Dec 17, 2023
nielsdos pushed a commit that referenced this pull request Dec 17, 2023
nicolas-grekas added a commit to symfony/symfony that referenced this pull request Apr 9, 2024
…omTimestamp() (derrabus)

This PR was merged into the 7.1 branch.

Discussion
----------

[Clock] Add a polyfill for DateTimeImmutable::createFromTimestamp()

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | N/A
| License       | MIT

This PR adds the new `createFromTimestamp()` method introduced in php/php-src#12413 to our `DatePoint` class. It comes with a polyfill implementation for PHP 8.2/8.3.

Commits
-------

9e48e0d [Clock] Polyfill DateTimeImmutable::createFromTimestamp()
@steffenweber
Copy link

I'm late to the party but I'm wondering why in contrast to createFromFormat, createFromTimestamp doesn't have a second parameter to specify the timezone. Of course, unix timestamps don't have a timezone but when I'm using the DateTime class to format a date I'll have to call setTimezone now. This feels a little inconvenient (and seems to be a relatively expensive call).

@steffenweber
Copy link

To answer my own question, createFromFormat actually doesn't use the timezone parameter either in this case: "The $timezone parameter and the current timezone are ignored when the $datetime parameter either is a UNIX timestamp [...]" – https://www.php.net/manual/en/datetime.construct.php

@kamil-tekiela
Copy link
Member

I'm late to the party but I'm wondering why in contrast to createFromFormat, createFromTimestamp doesn't have a second parameter to specify the timezone. Of course, unix timestamps don't have a timezone but when I'm using the DateTime class to format a date I'll have to call setTimezone now. This feels a little inconvenient (and seems to be a relatively expensive call).

Because a UNIX timestamp already contains a time zone. Specifying a time zone would be redundant as you have found out yourself.

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

Successfully merging this pull request may close these issues.

4 participants