Change the signatures to use \DateTimeInterface instead \DateTime

[burzum]

The current implementation makes it impossible to use any custom implementation of a date time object.

Also it would be good to be able to tell the lib which object it should construct.

$dtTmp = new \DateTime('now', $timezone);

There are a few places where this is hard coded. I've seen the DateTime object is used a lot in at least three classes. So changing this is pretty difficult without overloading lots of methods.

A possible solution to the second problem could be a trait:

namespace Recurr;

trait DateConstructorTrait {

    protected $dateTimeClass = '\DateTime';

    public function setDateTimeClass($class)
    {
        $this->classChecker($class);
        $this->dateTimeClass = $class;
    }

    public function getDateTime($time = 'now', \DateTimeZone $timezone = NULL)
    {
        $class = $this->dateTimeClass;
        return new $class($time, $timezone);
    }

    protected function classChecker($class)
    {
        if (!class_exists($class)) {
            throw new \RuntimeException(sprintf('Class %s does not exist!', $class));
        }
    }
}

And replace the new \DateTime() calls with $this->getDateTime().

[cmfcmf]

The current implementation makes it impossible to use any custom implementation of a date time object.

Well, changing to \DateTimeInterface doesn't make it much better, because according to http://php.net/manual/de/class.datetimeinterface.php, userland classes cannot implement the interface themselves.

[cmfcmf]

Also, this makes the library incompatible with PHP < 5.5.0.

[burzum]

OK, I agree, I haven't seen this when I looked at the docs for the interface.

What about the date constructor trait? This would allow me to use it with Chronos.

[simshaun]

Implementing DateTimeInterface would be the proper solution here. I'm a little hesitant to increase the PHP version requirement to 5.5 for what I consider to be a minor improvement. I'll think about it.

[burzum]

@simshaun you can't implement DatetimeInterface.

Trying to implement DateTimeInterface raises a fatal error now. Formerly implementing the interface didn't raise an error, but the behavior was erroneous.

But about the php version, check http://php.net/eol.php, 5.4 reached it's end of life 5 month ago.

[simshaun]

I should have worded that differently. I meant "integrate by type hinting". While userland classes can't implement the interface, it would still allow libraries that extend \DateTime or \DateTimeImmutable to be passed in to Recurr.

Also, I know 5.4 is end-of-life, but a large number of people are still

[simshaun]

Accidentally hit close button on phone...

Anyway, a large number of people are still on 5.4. I still don't think this an important enough feature to warrant raising the requirement.

[brianmuse]

This makes it pretty difficult to use this library with DateTimeImmutable, which is definitely preferred to the mutable DateTime. Perhaps you could merge the change but tag a major version bump so that those on 5.4 or earlier can still lock into their current version of this library?

You can see from the below link that the vast majority of projects are now 5.5 or greater, and official support for php 5.4 ended about a year ago (http://php.net/supported-versions.php). Even 5.5 is near EOL.

https://seld.be/notes/php-versions-stats-2016-1-edition

Also, I'm not sure using a trait is such a great idea. I think that #85 is a good path, but it may be even better to just switch over to DateTimeImmutable entirely instead of the interface.

[simshaun]

Version requirement will be raised and this will be implemented for 2.0.

[foaly-nr1]

Didn't make it into 2.0, did it? Would a PR be helpful at all?

[simshaun]

A PR would be welcome, thanks.

[simshaun]

Thanks to @foaly-nr1, this has been done. Due to the method signature changes, I've released this in v3.0.

https://github.com/simshaun/recurr/releases/tag/v3.0

[afoeder]

I'd like again to throw in that DateTimeImmutable thing because at the moment the annotations are to return DateTimeInterface which is unfortunately not very helpful, I'd say it "improved to the worse": now you don't know what exactly to expect.
I work with DateTimeImmutable all across my code and I can't event just DateTimeImmutable::createFromMutable() because I first have to check if the DateTimeInterface is actually a DateTime.
So, I would vote to replace everything with DateTimeImmutable, change the signature to DateTimeImmutable. This would also be b/c compatible because when adapting people expect DateTimeInterface, they would be satisfied with DateTimeImmutable.

[foaly-nr1]

@afoeder whenever you use interfaces you won't know what implementation to expect but there is rarely a need to know. The use of interfaces for type hints has widely been accepted as best practice.

[afoeder]

@foaly-nr1 still not sure whether I agree on return types being interfaces in concrete classes. In a concrete class one should know better what to return than an interface.
That way the only sane way to go for me is to

// $recurrence being \Recurr\Recurrence
new \DateTimeImmutable($recurrence->getStart()->format(\DateTime::ATOM));

[foaly-nr1]

@afoeder a good part of this library returns whatever concrete class you pass into it. So we'd end up with some return types being concrete and some being interfaces. That seems worse to me.

[afoeder]

that's kind of exactly why I think annotating interfaces is/was made too easy. There are mixed return types and annotating interfaces is just "slightly" better.
For the given case passing in DateTimeImmutable did nonetheless result in a DateTime; just look out for $dtTmp in \Recurr\Transformer\ArrayTransformer

And "some being concrete and some being interfaces" would still be better than all being interfaces, at least adopters know what to trust.

[foaly-nr1]

You can still use a DateTime exactly like you'd use a DateTimeImmutable can't you?

[afoeder]

not when my further code relies on (whatever DateTime or DateTimeImmutable)

[foaly-nr1]

I can't think of an example where your code relies on the concrete class apart from type hints. Can you give me an example?

[afoeder]

hmm I think I didn't get you… I meant "…when my further code relies on concrete DateTime or DateTimeImmutable"; doesn't matter which one, just: when my code does not rely on the interface type hint but the concrete type hint.

[foaly-nr1]

How can code rely on DateTimeImmutable and not work when it gets a DateTime? I think it'll always work with both, won't it?

[afoeder]

[sorry, forgot to reply]:

type hints: i.e. when prescribing DateTimeImmutable via type hints; that's what I mean: all my code expects DateTimeImmutable except when otherwise required; I don't have such a case though and I honestly also can't think of one.