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

date_diff() bug #65768 #512

Closed
wants to merge 4 commits into from
Closed

Conversation

@nikita2206
Copy link
Contributor

nikita2206 commented Oct 29, 2013

Fixed the bug when DateTimeInterface::diff() (and date_diff()) was declared as diff(DateTimeInterface, DateTimeInterface), but the implementations were always diff(DateTime, DateTime), even when working with DateTimeImmutable (which gave you a fatal in any case)

Here's a link to bugtracker https://bugs.php.net/bug.php?id=65768

…eTimeInterface, DateTimeInterface), but the implementations were always diff(DateTime, DateTime), even when working with DateTimeImmutable (which gave you a fatal in any case)
@nikic

This comment has been minimized.

Copy link
Member

nikic commented Oct 29, 2013

Would you mind adding a test for this change?

@glenjamin

This comment has been minimized.

Copy link

glenjamin commented Oct 29, 2013

Does this fix calling on both DateTime and DateTimeImmutable instances?

@nikita2206

This comment has been minimized.

Copy link
Contributor Author

nikita2206 commented Oct 29, 2013

@nikic can't do it now (from home), will do tomorrow
@glenjamin Yes, as docs say, you can pass any DateTimeInterface instance

@nikita2206

This comment has been minimized.

Copy link
Contributor Author

nikita2206 commented Oct 30, 2013

@nikic Hmm, while writing tests I just came to a conclusion that we can't use date_ce_interface to make sure the object is suitable for us here, because then you can do something like

$d = new DateTimeImmutable("now");

class dt implements DateTimeInterface {
  function diff($d, $a = false){}
  function format($f){}
  function getOffset(){}
  function getTimestamp(){}
  function getTimezone(){}
  function __wakeup(){}
}
$d2 = new dt();

$d->diff($d2);

and it will break things badly...
So, I assume we should check if this object is a child of DateTime or DateTimeImmutable. But it can become a mess...
Maybe we can go for AbstractDateTime? Or should I just create a macros, say DATE_CHECK_INTERNAL?

@nikita2206

This comment has been minimized.

Copy link
Contributor Author

nikita2206 commented Oct 30, 2013

@nikic But on the other side it would be reasonable to support diffing even fully user-defined dates... Just, if passed object is not an internal date, then we call its diff method and pass it the other object (which can be internal or not, we shouldn't care about that from that point). Yeah, I will try to implement it.

@glenjamin

This comment has been minimized.

Copy link

glenjamin commented Oct 30, 2013

The problem arises because while diff checks for DateTimeInterface, it is implemented using something that is not specified in the interface.

There'd be a performance penalty to creating a timelib struct using only the public interface, but perhaps you could have DateTime and DateTimeImmutable special-cased, otherwise use getTimestamp to create a timelib of the argument object, then proceeed as normal?

@nikita2206

This comment has been minimized.

Copy link
Contributor Author

nikita2206 commented Oct 30, 2013

@glenjamin
There's always some trade-off, as I see it:
First, we could call user-defined diff, as I said before. But it is bad because: we are calling user-space function from engine - that's not good I suppose, also we must take care of the returned value (maybe typecheck), and we will grow call-stack...
And second, we could convert non-internal dates to timelib_time, as you suggested, but there's one thing I think will mess us up - getTimestamp returns long (php's long), which can be of size of 32-bit on 32-bit systems, while timelib_time's sse member is of type signed long long. So I suppose we can't use timestamps here. Then we could use format method here - but the DateTimeInterface doesn't explicitly declare how it should work. I mean - we use i for minutes and m for months, but what if user, who implemented DateTimeInterface, wants to use m for minutes and i for months?

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Oct 30, 2013

@nikita2206 That issues doesn't seem specific to date_diff, but also applies to all the other methods accepting date_ce_interface. To avoid special handling all over the place I'd suggest to disallow implementation of DateTimeInterface by userland classes. So add a handler along the lines of:

static int implement_date_interface(zend_class_entry *interface, zend_class_entry *ce TSRMLS_DC)
{
    if (ce->type == ZEND_USER_CLASS) {
        zend_error(E_ERROR, "DateTimeImmutable can't be implemented by user classes");
    }

    return SUCCESS;
}

Might need a bit of tweaking (maybe it needs to check whether a parent if DateTime/DateTimeImmutable instead, don't remember the invokation point just now).

@nikita2206

This comment has been minimized.

Copy link
Contributor Author

nikita2206 commented Oct 30, 2013

@nikic Hi, I commited a test-case and made DateTimeInterface not implementable, as you said. I also have an implementation of what I was talking about a few posts above, actually there are pretty few of functions that take DateTimeInterface as an argument

@nikita2206

This comment has been minimized.

Copy link
Contributor Author

nikita2206 commented Oct 30, 2013

Btw, travis reported failure due to date timezone not being set. How we usually deal with this kind of fails?

@nikita2206

This comment has been minimized.

Copy link
Contributor Author

nikita2206 commented Nov 18, 2013

I fixed a test, now it should pass on travis.

@hikari-no-yume

This comment has been minimized.

Copy link
Contributor

hikari-no-yume commented Nov 29, 2013

DateTimeInterface not being implementable, shouldn't that go in UPGRADING or something like that? (Sorry if I'm confused here and wrong)

@nikita2206

This comment has been minimized.

Copy link
Contributor Author

nikita2206 commented Nov 29, 2013

@TazeTSchnitzel yeah, I think it should. Didn't think about it at the time when I committed it, I will add this note in UPGRADING file tomorrow

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Nov 29, 2013

Merged via 5f09944 for 5.5.8, thanks for the patch!

Could you please close this PR yourself? Our tool for that seems to be currently broken...

@nikita2206

This comment has been minimized.

Copy link
Contributor Author

nikita2206 commented Nov 29, 2013

Ok, thanks!

@nikita2206 nikita2206 closed this Nov 29, 2013
@nikic

This comment has been minimized.

Copy link
Member

nikic commented Nov 29, 2013

One more note: Usually it's best to do code changes with --enable-maintainer-zts, otherwise you'll likely forget a TSRMLS_CC somewhere. E.g. in this case instanceof_function needed one.

Maybe we should make travis builds in zts to catch this.

@nikita2206

This comment has been minimized.

Copy link
Contributor Author

nikita2206 commented Nov 29, 2013

@nikic oops, ok I will use it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.