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

Fix copying of DateInterval objects #87

Merged
merged 2 commits into from
Oct 19, 2017

Conversation

theofidry
Copy link
Collaborator

@theofidry theofidry commented Oct 15, 2017

Closes #75, #76.

@theofidry
Copy link
Collaborator Author

Kinda weird, it passes when debugging step by step but not when running the test without breakpoints.

@mnapoli any idea?

@theofidry theofidry mentioned this pull request Oct 15, 2017
@mnapoli
Copy link
Member

mnapoli commented Oct 16, 2017

Not really :/ Maybe when you are debugging the date changes (seconds are passing) and when not debugging the test runs fast enough? Or maybe you are touching some weird behavior of PHP's internals ;)

@theofidry
Copy link
Collaborator Author

Going back on my words: we're may be introducing a BC here but more likely fixing a broken case.

@theofidry
Copy link
Collaborator Author

@mnapoli as there may be BC I would like you to review that one if you don't mind :)

@mnapoli
Copy link
Member

mnapoli commented Oct 17, 2017

I'm reading the diff and the other tickets but I'm not sure I understand the original problem. If I understand correctly what happens here is:

  • DateTimeInterval are handled with an exception (like the other DateTime classes) -> good to me
  • subclasses of DateTimeInterval is treated differently: why is that?

@theofidry
Copy link
Collaborator Author

Actually as the type filter is registered, DateInterval are handled by DateIntervalFilter. The change applied L192 is useless and needs to be reverted.

There is an inconsistency here, for DateTime and DateTimeZone we do a shallow copy with clone, which let the user define his kind of custom cloning if he wants to if he is inheriting from DateTimeZone. However a clone doesn't work on <PHP7.1 on DateInterval...

@theofidry theofidry merged commit 3b8a3a9 into myclabs:1.x Oct 19, 2017
@theofidry theofidry deleted the bugfix/dateinterval branch October 19, 2017 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants