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

int|float for DateTime::setTimestamp #13383

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

divinity76
Copy link
Contributor

just like the constructor accepts

new DateTime("@0.123456"); // 1970-01-01 00:00:00.123456
new DateTime("@".microtime(true));

now setTimestamp accepts it too:

$dt->setTimestamp(0.123456); // 1970-01-01 00:00:00.123456
$dt->setTimestamp(microtime(true));

resolves #13376

@SakiTakamachi
Copy link
Member

The changes look good to me.

But the following code:

<?php
var_dump((new DateTime)->setTimestamp(microtime(true)));

This is interpreted as an int and works, albeit with a warning. So, strictly speaking, this is a BC break.

I honestly don't know if an RFC is necessary for this.

@divinity76
Copy link
Contributor Author

divinity76 commented Feb 13, 2024

nah it's all good, it will still work, just with higher accuracy and less warnings.
the BC break is inconsequential and borderline academic

I would be more worried about changing DateTime::getTimestamp to return float (that one might need a bool $as_float=false; argument)

@SakiTakamachi
Copy link
Member

In the case of the following code, the comparison result will change:

<?php

$d1 = (new DateTime)->setTimestamp(microtime(true));
$d2 = (new DateTime)->setTimestamp(microtime(true));

if ($1 < $d2) {
    // any
}

I don't know if this kind of code is actually used....

@divinity76
Copy link
Contributor Author

@SakiTakamachi that code is bugged and will randomly return true/false even today, the code:

$t1 = new DateTime();
$t2 = new DateTime();
$attempts = 0;
echo "it usually returns false\n";
while(!($t1->setTimestamp(time()) < $t2->setTimestamp(time()))) {
    ++$attempts;
}
echo "but after $attempts attempts it returns true\n";

outputs

it usually returns false
but after 2475192 attempts it returns true

@SakiTakamachi
Copy link
Member

Yes, that's right. So I'm not sure if I should treat this as a BC Break. I'd like to wait for other people's opinions.

@divinity76
Copy link
Contributor Author

divinity76 commented Feb 14, 2024

¯\_(ツ)_/¯ i'll ask on the mailing list

Copy link
Member

@kocsismate kocsismate left a comment

Choose a reason for hiding this comment

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

the PR looks good to me.

static void php_date_timestamp_set(zval *object, zval *timestamp, zval *return_value) /* {{{ */
{
php_date_obj *dateobj;
timelib_sll ts;
Copy link
Member

Choose a reason for hiding this comment

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

nit: the same spaces vs tab "issue" here as well

@derickr
Copy link
Contributor

derickr commented Mar 18, 2024

Please address @kocsismate's coding standard comments. Indentation is with spaces in .c files. (It also now conflicts, so please rebase on master too).

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.

DateTime::setTimestamp should accept int|float $timestamp
4 participants