fix: make Time immutable

[kenjis]

Description
Fixes #6762

• Time will extend DateTimeImmutable
• rename the current Time to TimeLegacy

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[MGatner]

This is another situation of "do we break it to fix it?" Let's get some discussion on #develop and maybe even the forums before we jump in.

Also: instead of TimeLegacy I would like to see a TimeMutable class. We could discuss whether that includes changing methods like setTimezone to be mutable.

[kenjis]

Also: instead of TimeLegacy I would like to see a TimeMutable class.

If we have TimeMutable, we need to change all write methods to be mutable in the current Time.
Then, since TimeMutable is incompatible with the current Time, we still need TimeLegacy (or Time).
It is questionable whether TimeMutable is necessary now.

Another option is to make the current Time deprecated and add a new TimeImmutable.

[paulbalandan]

I would like to see the second option. Also, we can extract the methods which do not change in implementation into a trait/traits so that we minimize code duplication.

[MGatner]

Deprecation and adding TimeImmutable seems the conservative path. With so many other big changes happening I think that is the way to go. In v5 we can kill the mutable class - IMO DateTime should never have had a mutable version, and there's even been some talk of removing it: https://www.reddit.com/r/PHP/comments/w7v4pb/deprecating_the_mutable_datetime_class/

[lonnieezell]

I'm personally not a fan of two classes for this honestly. I know I've been the one arguing against breaking changes, but in this case I think it's justified because it has always been billed as an immutable class and the fact that we determined it wasn't I think definitely qualifies as a bug, personally.

[michalsn]

Usually, I'm like... if there is a bug - fix it, and that's it, but in this case, I'm not sure.

Without a doubt, people have already created a lot of code that depends on the current implementation, and manipulating dates can be challenging and tricky the more operations we're doing.

I would keep the TimeLegacy class just for these reasons. But without separate service available.
In upgrade instructions, we can inform users that if their code relies heavily on the previous implementation of the Time class, they should override the current definition of a service to use TimeLegacy class.

I know it's not ideal, but it would help keep everyone happy without additional maintenance.

TimeLegacy class would disappear with v5.

[lonnieezell]

@michalsn I think that sounds like a fair compromise.

[kenjis]

In upgrade instructions, we can inform users that if their code relies heavily on the previous implementation of the Time class, they should override the current definition of a service to use TimeLegacy class.

What is the service? Services::time() does not exist.
And Time has static factory methods.

[michalsn]

What is the service? Services::time() does not exist. And Time has static factory methods.

Lol... I guess I don't use this class "often". My advice is more or less useless then.

[kenjis]

@paulbalandan
Moved common code to TimeTrait.

Before refactoring:

--- Time.php	2022-10-29 17:26:50.000000000 +0900
+++ TimeLegacy.php	2022-10-29 17:28:38.000000000 +0900
@@ -24,14 +24,17 @@
 use ReturnTypeWillChange;
 
 /**
- * A localized date/time package inspired
- * by Nesbot/Carbon and CakePHP/Chronos.
+ * Legacy Time class.
  *
- * Requires the intl PHP extension.
+ * This class is only for backward compatibility. Do not use.
+ * This is not immutable! Some methods are immutable,
+ * but some methods can alter the state.
  *
  * @property string $date
+ *
+ * @deprecated Use Time instead.
  */
-class Time extends DateTimeImmutable
+class TimeLegacy extends DateTime
 {
     /**
      * @var DateTimeZone
@@ -118,7 +121,7 @@
      * Returns a new Time instance while parsing a datetime string.
      *
      * Example:
-     *  $time = Time::parse('first day of December 2008');
+     *  $time = TimeLegacy::parse('first day of December 2008');
      *
      * @param DateTimeZone|string|null $timezone
      *
@@ -314,7 +317,7 @@
 
     /**
      * Creates an instance of Time that will be returned during testing
-     * when calling 'Time::now' instead of the current time.
+     * when calling 'TimeLegacy::now()' instead of the current time.
      *
      * @param DateTimeInterface|self|string|null $datetime
      * @param DateTimeZone|string|null           $timezone

[kenjis]

Deprecation and adding TimeImmutable seems the conservative path.

Yes.

Good news is there seems many users who don't use Time.
Because Time was introduced in v4. It is a new class.

And all the description in the User Guide is immutable. Only some inherit methods are mutable.
So if we fix Time, that might not break existing applications.

If fixing Time does not affect many users, it would be better to fix Time.
I don't think it is a good idea to leave only TimeImmutable in the future.

[MGatner]

Looking good!

[kenjis]

Let's get some discussion on #develop and maybe even the forums before we jump in.

I posted. https://forum.codeigniter.com/showthread.php?tid=84677
Also asked in Slack #codeigniter4.

[MGatner]

It seems to be that current sentiment is leaning towards "just fix it". I think the proposed solution (TimeLegacy plus switch parent) is a good fit.

[kenjis]

Added the docs.

[paulbalandan]

So, the difference of Time and TimeLegacy, although both using TimeTrait, is the extended parent. Very nice!

[kenjis]

🎉