addMinutes() across daylight savings going out gets the wrong time

[joshbmarshall]

In my project I use diffInMinutes to get the start of day, then round that minutes down to a quarter hour, then add the minutes to the start of day to get the appropriate time.

But when the day has daylight savings going out, it calculates the hour incorrectly.

I created PR #518 to show the issue.

[dereuromark]

Can you check if #520 solves this?

[joshbmarshall]

It might fix the issue, however it means that we need to refactor all our code to use the new (crazy long function name) functions.

The issue we have here is that I could understand if the addMinutes etc functions added wall clock time, as long as the time zone offset didn't change.

For example it goes from 00:00:00+11 to 10:00:00+10 when adding 10 hours. The clock doesn't understand zone offset, so it should stay at +11 (which would be correct)

Is there a reason why addMinutes() would be used across a DST transition and expect there to not be a clock shift at the DST transition point? Maybe the consideration would be whether the current functionality would warrant a new function name and the one that takes timezone and DST into account be the default.

[joshbmarshall]

If we are forced to refactor code, it might be nicer to have shorter function names - the ones copilot suggested are a mouthful. addUTCMinutes() makes more sense than addMinutesWithTimestamp()

I still think that devs would expect that having a point in time with a timezone, if they add a minute then they'll get the time in one minute time, with that timezone. If it's across a DST transition you'd expect it to handle it. So the timestamp code would be the default (my opinion) - I know this would be considered breaking change :(

[dereuromark]

BC reasons: Existing apps silently getting different behavior is worse than having verbose method names. The safe path is new methods.

[joshbmarshall]

Gotcha. Can we have a nicer method name that actually makes sense? addMinutesWithTimestamp() doesn't indicate that it's moving universe time along.

[joshbmarshall]

Some examples of alternates that make more sense (to me):

• addElapsedMinutes() * my preference
• addRealMinutes()
• addDurationInMinutes()

[dereuromark]

I agree, that there could be other alternatives.

Option	Methods	Pros	Cons
A. Elapsed	addElapsedMinutes()	Clear semantic meaning - "elapsed time"	17 chars
B. UTC prefix	addUtcMinutes()	Short, matches Carbon's naming	Slightly misleading - not actually converting to UTC
C. Real	addRealMinutes()	Short, familiar to Carbon users	"Real" is vague, Carbon deprecated this naming
D. Actual	addActualMinutes()	Plain English	Could be unclear

My preference is Option A: addElapsedMinutes() - it directly describes the semantic difference: you're adding elapsed (real) time rather than wall clock time. This makes the distinction clear when reading code.

Option B (addUtcMinutes()) is shorter and matches Carbon, but "UTC" is somewhat misleading since the method doesn't actually convert to UTC - it just uses timestamp arithmetic.

Thoughts?

[joshbmarshall]

Yes I'd be more than happy with addElapsedMinutes - it's instantly recognized for what it does

Thanks for being open to suggestions/feedback!

[dereuromark]

There is a new major 4.x coming up together with Cake v6 (see 4.x branch).
In that we could revise the default behavior and document the other methods to be the alternatives then.

[joshbmarshall]

That would be awesome if you would. I don't know how many people it would negatively affect, but they would be able to know in advance before upgrading.

[dereuromark]

Updated PR #520 with the addElapsed* naming. Methods are now:

• addElapsedHours() / subElapsedHours()
• addElapsedMinutes() / subElapsedMinutes()
• addElapsedSeconds() / subElapsedSeconds()

[dereuromark]

https://github.com/cakephp/chronos/releases/tag/3.5.0