Skip to content

Concept: Utils\DateTime deprecation & replacement#82

Closed
Majkl578 wants to merge 2 commits intonette:masterfrom
Majkl578:datetime-replacement
Closed

Concept: Utils\DateTime deprecation & replacement#82
Majkl578 wants to merge 2 commits intonette:masterfrom
Majkl578:datetime-replacement

Conversation

@Majkl578
Copy link
Contributor

@Majkl578 Majkl578 commented Aug 24, 2015

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.)

@Majkl578 Majkl578 force-pushed the datetime-replacement branch from 244b179 to 3356dc4 Compare August 24, 2015 23:15
@pavelkouril
Copy link
Contributor

pavelkouril commented Aug 25, 2015

I definitely like the idea. 👍

@fprochazka
Copy link
Contributor

fprochazka commented Aug 25, 2015

Good job 👍

@dg
Copy link
Member

dg commented Aug 26, 2015

createMutable() vs createImmutable() makes users think…

@JanTvrdik
Copy link
Contributor

JanTvrdik commented Aug 26, 2015

👍 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

Majkl578 commented Aug 26, 2015

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.

@Majkl578 Majkl578 force-pushed the datetime-replacement branch from 3356dc4 to 0165413 Compare August 26, 2015 16:50
@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 Majkl578 mentioned this pull request Feb 1, 2017
Open
@dg dg force-pushed the master branch 4 times, most recently from ba38669 to bba41ff Compare February 10, 2017 12:52
@dg dg force-pushed the master branch 4 times, most recently from f6984a9 to eed59a3 Compare July 21, 2017 18:54
@Majkl578
Copy link
Contributor Author

Majkl578 commented Jul 23, 2017

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Majkl578 @pavelkouril @fprochazka @dg @JanTvrdik @enumag