Chronos\ChronosInterface usefulness

[nyamsprod]

function doSomething(\Cake\Chronos\ChronosInterface $chronos)
{
    return $chronos->year(2015);
}

If a Cake\Chronos\Chronos object is given to the function returns a new Cake\Chronos\Chronos because this object is immutable

If a Cake\Chronos\MutableDateTime object is given to the function will modify the object and return a reference to it because it is mutable

So you can not safely typehint against the Cake\Chronos\ChronosInterface and be safe regarding

• the input being changed or not
• the output being a new object or a reference to input

This means that you should typehint against a concrete implementation which reduce the interface usefulness

From a developer point this is a design issue. Of note, PHP's DateTimeInterface avoid this problem by only specifying methods which do not affect the object internal state.

[josegonzalez]

As I mentioned before:

ChronosInterface exists so we can typecheck as to whether or not the object coming in is a type of Date/DateTime object from the Chronos package, and we would use a more specific typehint where necessary.

Maybe we should have a MutableChronosInterface and ImmutableChronosInterface as well, so you can check both that the object inherits from the Chronos package as well as it's mutability?

[nyamsprod]

A simpler solution would be to copy DateTimeInterface (ie: to only put non modifying methods in the common interface). This way you get to safely typehint against it;

Each class modifying method behaves differently even if they share the same name so I see no added value to have them in the interface. (ie: Chronos::add != MutableDateTime::add != Date::add != MutableDate:add)

[markstory]

The main goal with the ChronosInterface was to allow separation between native DateTime objects and enriched ones like @josegonzalez mentioned.

I can see how the current interface hints don't allow you to ensure immutability though. What do you think of having a few more 'marker' interfaces:

• Cake\Chronos\ImmutableInterface - For the immutable variants.
• Cake\Chronos\MutableInterface - For the mutable variants.
• Cake\Chronos\DateInterface - For the date objects that lack time.

I think this gives enough typehints to give what you're looking for, but I might be missing something.

[nyamsprod]

I think that:

• Cake\Chronos\ChronosInterface should only contains non modifying methods
• Cake\Chronos\ChronosInterface should not extend DateTimeInterface
• A Cake\Chronos\DateInterface should be added that mimics DateTimeInterface methods

which results on:

• Chronos implementing DateTimeInterface and ChronosInterface
• MutableDateTime implementing DateTimeInterface and ChronosInterface
• Date implementing Cake\Chronos\DateInterface and ChronosInterface
• MutableDate implementing Cake\Chronos\DateInterface and ChronosInterface

To ease implementation, internally Date would represent its date as a Chronos object and MutableDate would represent its date as a MutableDateTime object.

This means that when typehinting against Cake\Chronos\ChronosInterface you expect the object to not being modified by whoever is using it. Otherwhise the developer should restrict the typehint to only concrete classes (ie: Cake\Chronos\Chronos, Cake\Chronos\Date, etc...).

Even thought I think that having a Cake\Chronos\ImmutableInterface or a Cake\Chronos\MutableInterface is not mandatory you may choose to add them.

This solution would even bug fix #76 as Cake\Chronos\Date would never be accepted as a possible argument for PHP's DatePeriod constructor. Of course someone can think of this as a trade-off.

[markstory]

I think that interface breakdown makes sense.

I was thinking more about interfaces for immutable/mutable differences earlier today, and I'm not sure it makes sense to have them. Even in a fully static type system the return types of both mutable and immutable objects would be the same, so there would be no violation of the interface methods. For example add() in both mutable/immutable cases would return an instance of self which would satisfy any interface definition.

I'm still not sold on Cake\Chronos\Date not extending DateTime. Converting that class into a proxy wrapper could cause existing code to break as that code could be relying on existing subclass type hints.

[nyamsprod]

@markstory the real question we should ask ourselves is what is the goal of the Cake\Chronos\Date class. Once you figure it out and layout its real public API (interface) then you will be able to determine if adding the DateTimeInterface to it make sense or not.

Currently I have the feeling it was conceive the other way around. DateTimeInterface and its implementations were taken for granted and Cake\Chronos\Date was build around it without questioning if this was the right choice. Hence the issues and the discussion we are having right now.

[markstory]

@nyamsprod The intention/goal of Date was to provide the familiar DateTime interface but constrain the valid set of values to calendar dates. I wanted to keep as much compatibility as possible with DateTime so that dates and datetimes could be used interchangeably in most situations. I also had the larger goal of resolving a long standing issue in cakephp/orm where SQL date columns were mapped to DateTime objects which was sub-optimal.

[josegonzalez]

Closing as there isn't anything actionable here and I believe the question as to why it was built this way has been answered.

[nyamsprod]

@josegonzalez IMHO you should not close an issue which as not been resolved or without a solid argument for not fixing it. Indeed an historical explanation of the current situation is given but that's not a valid argument to keep a bug in any codebase.

FWIW, there are actions that can be taken to resolve this issue without breaking anything. For instance, you could added 2 new interfaces UpdatedChronosInterface and UpdatedDateInterface (naming things is hard, I'm sure we can come up with better names) that follow what I've suggested and it would resolve this current issue as well as at least one other issue. Of course, it would mean deprecated and then removing in the next major release the current ChronosInterface.

[lorenzo]

@nyamsprod that does not sound like a dead idea to me. Would you like to open a pull request with those additions?

[nyamsprod]

@lorenzo no offense but why is it not obvious that the ChronosInterface is buggy ? Using PHP7 return type hint this should be obvious to anyone:

<?php
use Cake\Chronos\ChronosInterface;
use Cake\Chronos\Chronos;
use Cake\Chronos\MutableDateTime;

function doSomething(Chronos $chronos): Chronos
{
    return $chronos->year(2015); // I know what I'm returning is immutable object so I can do whatever I want
}

function doSomething(MutableDateTime $chronos): MutableDateTime
{
    return $chronos->year(2015); // I know what I'm returning is mutable object so I can do whatever I want
}

function doSomething(ChronosInterface $chronos): ChronosInterface
{
    return $chronos->year(2015);  // I know I can use any getter 
                                  // I'm mislead into thinking I can rely on the setters methods
}

function doSomething(BugFixedChronosInterface$chronos): BugFixedChronosInterface
{
    return $chronos->year(2015);  // I know I can use any getter 
                                  // I know I can not rely on any setters
}

Adding this BugFixedChronosInterface which only exposes the getter methods would solve the issue. FWIW, the same issue was discussed during DateTimeInterface addition into PHP core and the solution reached was to remove any setter from the interface, which is what I'm suggesting.

Last but not least, closing this issue won't make the design flaw disappear. This is like ignoring the main issue this library is facing when integrating its interface in 3rd party codebase.

my 2c.

[lorenzo]

@nyamsprod I think we are past that debate. We have the interface today and cannot remove it in a patch release.

I think it is a good idea to break down the interface in smaller ones. Would you like to submit a pull request for it?

[nyamsprod]

@lorenzo I'm not suggesting to remove it now. Maybe in the next major release. I'm suggesting to add the correct interface now which in my book is not a BC break. Now you may do it or other may do it. I personnaly can't becasue I have other open source project to maintain.

[lorenzo]

@nyamsprod We both are restricted by time available then. Thanks for the suggestions, I do think they make sense :)

[dereuromark]

@nyamsprod Are you interested in making a PR with the suggested improvements? That would be great.

[MGatner]

This one is a bit tricky, as the long discussion validates. Two points I think should be incorporated into any solution (these are insights that have come from time, not a criticism of where we all were in years past):

1. Large interfaces are a bad design and should be avoided

2. Using an interface solely for union typing is mostly a hack

I realize DateTimeInterface is basically PHP-sanctioned version of no. 2 but they had the luxury of control:

It is not possible to implement this interface with userland classes.

I think the suggestions to break this interface up are good. I also don't think it is necessary to provide interfaces for every possible combination, because we can do this in PHP 8 & docblocks: Chronos|MutableDateTime.

If the goal is to have a central base class to branch all the divergent behavior from yet store things like constants then an abstract class is a better fit.

[ADmad]

Most likely in the next major we are only going to keep the immutable classes and drop the mutable ones, which should simply things a bit.

[MGatner]

I think that is a good plan. Is this planned work? Or tied to Cake v5? Or something contributors could help with now?

[ADmad]

Or tied to Cake v5?

This is a separate lib so not really, we were just hoping to get it done in time for Cake 5.

Or something contributors could help with now?

Yes contributor help is most welcome.

[othercorey]

Will be dropped in 3.x.