Add native modify

[christian-kolb]

Change Log

Added

• Added native modify method

---

Description

I stumbled over the following issue:

In Germany there is a winter time to summer time switch on for example the 26. March 2023. When I work with modifications of a DateTime, the native DateTimeImmutable is aware of those switches. The current modify implementation internally uses seconds for all modifications and is able to solve issue like moving a month at the end of the month as we already touched upon here #294.

This means that when I'm using a DateTime and calling modify or subDays I get the result that at least differs from the expected date and might even be considered wrong.

For example:

$firstOfApril = DateTime::fromDateTime(
    new \DateTimeImmutable(
        '2023-04-01 00:00:00',
        new \DateTimeZone('Europe/Berlin'),
    )
);

echo $firstOfApril->modify('- 14 days')->format(\DateTimeInterface::ATOM);

It results in

2023-03-17T23:00:00+01:00

If it would be timezone aware it would be

2023-03-18T00:00:00+01:00

I don't think this is something that can be "fixed" in the current modify as the internal approach with working with seconds kind of prevents a timezone aware logic.

Therefore, I introduced a new nativeModify method that simply pipes through the modifier to the internal DateTimeImmutable.

[christian-kolb]

If the fact, that timezone awareness is missing, is on purpose, then we could also rename it to a timezoneAwareModify 🙂

[christian-kolb]

Unfortunately, I don't know how to setup a test that creates a false return on modify. All examples lead to an internal error instead of false. Do you know what's needed to get false there?

@norberttech Ok, it never returns false for PHP 8.0. Therefore I couldn't find it. I guess that's the same issue with the mutations? I can't find a way to prevent this. Any ideas?

[christian-kolb]

@norberttech The Static Analyzer seems to screw with me. When I don't put a @var array|false over the date_parts call, it complains that it will never be false at the $dateTimeParts === false part, but with it, it complains that it's unnecessary 🤦

[norberttech]

Hey, I'm still thinking about your proposition, on the one hand I definitely see the issue, but on the other hand I would like to avoid having 2 methods as it will impact the developer experience.

My current idea would be to keep one method but with the parameter that would define how the difference should be calculated.

[christian-kolb]

The more I think about the current setup the more I struggle to find a use case where the current behaviour works.

I think I found the issue:

Mutating a DateTime must be done in the TimeZone of the user. Otherwise mutating to move 14 days back would sometimes (depending on the summer/wintertime switch) be an hour off. Or other even stranger values depending on the TimeZone.

aeon-php want's to work exclusively in UTC. For example in DateTime::fromString it switches the default timezone to UTC and then creates an object. Data stored in the database therefore needs to be in the TimeZone UTC (which I don't object to) to work.

I understand from the other ticket that you want to "fix" the strange calculation that PHP (or the underlying OS library) uses, but I think to accomplish that, the logic of having a mutation being aware of a TimeZone got lost.

It looks to me as if mutations are "broken" in general if you're in a Timezone that has a winter/summertime switch.

I understand that this would have massive effect and impact on the design. So maybe we should take our time to discuss this in more detail.

Or I'm missing a crucial piece of information. I'm open to that idea.

[norberttech]

This is indeed problematic, which is why I believe the possibility of choosing the behavior of modify method would be a good fit here.
What I was trying to solve originally is the way PHP is subtracting/adding relative time units like for example +1 month to a given date - example, and this is the place where current behavior works.

But I actually missed the winter/summertime switch issue, but I'm not sure if I can clearly say the current behavior is wrong.

I get the result that at least differs from the expected date and might even be considered wrong.

That actually depends on what your expectations are here. I do not have here any strong evidence on how it should work, so it's only my opinion, but this behavior can be pretty much explained by the following code:

echo (new \DateTimeImmutable('2023-04-01 00:00:00 Europe/Berlin'))
  ->setTimeZone(new \DateTimeZone('UTC'))
  ->modify('-14 days')
  ->setTimeZone(new \DateTimeZone('Europe/Berlin'))
  ->format("Y-m-d H:i:s T"); // 2023-03-17 23:00:00 CET

https://3v4l.org/WTa6H#v8.1.11

That's why I would rather like to document both behaviors as I can see how both of them can be correct in certain scenarios and let the end user decide which one works better for him.
The native one, which works a bit non-intuitive when it comes to adding/subtracting relative time units but which supports winter/summer time switches.
Or the custom one that operates on the UTC timezone and that normalizes adding/subtracting month to a given date.

One more solution here would be to actually check in the code if there was winter/summer time switch and add/sub and hour to the final result.

Mutating a DateTime must be done in the TimeZone of the user.

It's irrelevant to the problem but technically speaking DateTime is immutable, so there is no mutation here - just the creation of a new instance.

[christian-kolb]

Maybe it's useful to go back to the very beginning. You've changed an unintuitive behaviour of PHP when modifying a date with one or more months. In general PHP always tries to recover from an invalid function call and still give back a "correct" result instead of failing. Here it's that its adding days to a date if the month doesn't have as many days as the one from where the modification happens from. I'm a big fan of letting those calls fail, because without knowledge of the current context you can't know what the programmer meant. PHP thinks it should try to return an existing date (and adds additional days), you in your implementation say: It should always be the last day of the month if there are too many days.

Depending on the use case both can be right or wrong. Both aren't well documented. Both might lead to wrong results. So in my opinion, the changed variant doesn't add much. It's just different taste.

On the flip side, a custom implementation means room for additional errors. While only using this logic for a month, I already run into 2 bugs. One was found in production, the other was accidentally caught by a test. And there might be a lot more.

Please don't get me wrong, it's not that I don't think you're smart enough to know how to handle modification. I rather think that there is no single person alive that is able to know about and correctly implement the whole complexity embedded in date handling. Only a bigger group of people over a longer time frame might be able to. The main problem here is that we don't know what we don't know. And therefore can't handle the things we don't know by definition.

I wouldn't say that there can't be any custom implementation, only that this then should be very clear to the API user so that they can decide to take the risk for the additional benefit.

But maybe I just have completely different expectations than you and other people using the library.

What I was searching for was a library that has a better way to:

• compare a time of a day to a point in time.
• compare a day to a point in time.

Both of them are covered perfectly by aeon-php and it includes a lot other tools and DX improvements. But what I also get is changed modification behaviour. And that's something I really don't want and the more I think about it (I'm thinking on paper here), the more I think the philosophy of the library just doesn't match my own.

All I really need is a very thin wrapper over a DateTimeImmutable that is able to return a Day or Time utility object that can be stored to a database, be normalized and have a few comparison functions. And for modification, just pipes them through to the underlying object.

With the amount of stars and downloads you've got on the package, there's obviously a need for the way you build the package. So don't want to impose by priorities and values on it, and will just try to build that on my own and see if it's as straightforward as I think it is.

[norberttech]

Let me just to summarize my thoughts and some facts about "native modify" (as I believe we are both talking about the same thing).

Opinions:

• I do believe aeon needs to support native modify
• I do believe current implementation is also needed
• I'm not the biggest fan of adding new "modifyNative" but instead I would rather add a parameter to existing modify, like: DateTime::modify("", Mode::Native)
• there is no "right" or "wrong" solution in this discussion

Facts:

• default behavior must stay as is, at least in version 1.x in order to not break more production systems
• there was a bug in the current implementation that you found (sorry for missing it) and there might be some more edge cases I misses since as you noticed, date&time is an extremely complex problem to solve but as you also noticed, PHP native class is not the best when it comes to Developer Experience.

If for any reason you can't or don't want to add a Mode parameter to the existing modify method that would allow users to change the behavior of modify (and let you use the one relying on the timezones) I'm going to add it. I just don't know when since I'm a bit busy recently.

I honestly and highly appreciate your input and all the thoughts you shared with me about this project, it's the biggest contribution I got so far, which is also a great motivation for me. I totally understand that you might be looking for something different and I encourage you to build your own lib if aeon is not aligned with your vision. One way or another, you made this project better, thanks!

[christian-kolb]

Highly appreciate the feedback. The flag sounds like a viable solution to me.

I'd suggest adding it to all the sub* and add* methods as well, as it's the same there.

I'm not opposed to doing this myself as part of this PR. I will concentrate on trying to build a lib first though to see if it's viable (I'm not sure about that at the moment). But I would adapt it afterwards (as well as work on the serializer package). If it takes to long for you or you find time first, don't hesitate to take over this PR or close this one and create a new one 🙂

[norberttech]

I'd suggest adding it to all the sub* and add* methods as well, as it's the same there.

Yes, it's the same.

I'm not opposed to doing this myself as part of this PR. I will concentrate on trying to build a lib first though to see if it's viable (I'm not sure about that at the moment). But I would adapt it afterwards (as well as work on the serializer package). If it takes to long for you or you find time first, don't hesitate to take over this PR or close this one and create a new one 🙂

Sounds like a plan then!