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

Concept: Utils\DateTime deprecation & replacement #82

Closed
wants to merge 2 commits into from

Conversation

Majkl578
Copy link
Contributor

Hi,

this is an attempt to phase out Nette\Utils\DateTime and replace it by a new helper class.
The current DateTime makes date & time manipulation easier since it enhances \DateTime, yet it has some major flaws: mutability and inheritance.

Why should we want the new helper class?

  • current DateTime contains some useful methods to ease manipulation with DateTime instance, these are moved to this new class;
  • supports both mutable \DateTime and immutable \DateTimeImmutable variants, through \DateTimeInterface;
  • inheritance-free, does not extend mutable \DateTime;
  • does not provide magical __toString (which by the way drops the time zone information).

This PR is a draft, not a final proposal yet. It consists of new tests for existing DateTIme class as well as the actual replacement with tests as well.

I'd like to hear some feedback -- whether is it really what we want (I hope so) and what & how could be eventually changed.

(This depends on #79 which bumps composer.json & Travis versions. Hence Travis fails.)

@pavelkouril
Copy link
Contributor

I definitely like the idea. 👍

@fprochazka
Copy link
Contributor

Good job 👍

@dg
Copy link
Member

dg commented Aug 26, 2015

createMutable() vs createImmutable() makes users think…

@JanTvrdik
Copy link
Contributor

👍 For immutable only. The only good use-case for mutable DateTime is compatibility with old libraries which accepts DateTime only. Unfortunately there is no easy way in PHP core to convert DateTimeImmutable to DateTime.

@Majkl578
Copy link
Contributor Author

createMutable() vs createImmutable() makes users think…

Agreed, maybe it could be create() vs. createMutable() or something. But I'd like to keep both for the reasons below.

The only good use-case for mutable DateTime is compatibility with old libraries which accepts DateTime only.

Exactly. And I think that is very good reason for supporting it. As an example, whole Doctrine stack still does not natively support DateTimeImmutable/DateTimeInterface (or probably any other library following semver until they bump major version).

Unfortunately there is no easy way in PHP core to convert DateTimeImmutable to DateTime.

Actually, I really thought it was in (I used it in this PR, didn't even realize), but it was reverted afterwards, because Derick was against it. :( php/php-src#1145
This is imho yet another reason to have createMutable, which could handle this stupid boilerplate internally.

@dg dg force-pushed the master branch 13 times, most recently from 6c03b1f to a4bddc8 Compare September 9, 2015 07:16
@dg dg force-pushed the master branch 3 times, most recently from 2998062 to f6303c6 Compare September 28, 2015 14:20
@dg dg force-pushed the master branch 2 times, most recently from 46f5434 to ecbbdfc Compare December 3, 2015 12:21
@dg dg force-pushed the master branch 3 times, most recently from d830ff4 to 580569d Compare April 21, 2016 12:41
@Majkl578
Copy link
Contributor Author

What to do with this? Still doable for 3.0 where DateTime with inheritance sadly still exists, or should I just close it?

@dg dg force-pushed the master branch 7 times, most recently from 8b993d6 to 98975bf Compare July 24, 2017 15:01
@dg dg force-pushed the master branch 3 times, most recently from 8aa61b9 to fd48510 Compare February 19, 2018 14:44
@dg dg force-pushed the master branch 5 times, most recently from e2a373b to a316b52 Compare April 6, 2018 12:09
@Majkl578 Majkl578 closed this May 4, 2018
@Majkl578 Majkl578 deleted the datetime-replacement branch May 4, 2018 15:55
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

6 participants