final or not final #54

nyamsprod · 2017-09-07T07:31:10Z

Issue summary

Period is a value object. as such until version 3.0 the Period class was marked as final. The final keyword was removed as a requested from @shadowhand link to the relevant discussion

I am in the process of wrapping up the code for a v4 version which will be PHP7.1+ only. One of the last question I face is should I revert this change and make Period final again ?

Any input from the community is welcomed @Ocramius , @mathiasverraes, @frankdejonge you are welcome to participate. I should note that I never felt the need to extend the Period class. But They are some use cases I may not be taking into account.

thanks in advance

The text was updated successfully, but these errors were encountered:

Ocramius · 2017-09-07T08:43:15Z

@nyamsprod I don't know how the period class is supposed to be used, but it looks like a value object to me.

values don't need to be "extended" unless a child class adds more meaning to the value. Specifically, using instanceof against it provides more context at runtime. Another way to see it is "does it make sense to have an interface? And would anybody extend it?"
if it's not designed to be extended, don't allow for it to be extended
if this is about mocking, then bad news for who is mocking: this is a value, and it shouldn't be mocked.

mathiasverraes · 2017-09-07T09:06:14Z

I'm afraid I don't have time to read the original discussion, so maybe there are actual valid reasons I'm not aware of, but, to be honest, I doubt it. I never feel the need to extend or mock Value Objects. I almost never feel the need to extend _any_ object anymore. All the smart people have been telling us to use composition over inheritance FOR AT LEAST 2 DECADES. Inheritance is dead. Get over it.

…

On 7 September 2017 at 10:43, Marco Pivetta ***@***.***> wrote: @nyamsprod <https://github.com/nyamsprod> I don't know how the period class is supposed to be used, but it looks like a value object to me. 1. values don't need to be "extended" unless a child class adds more meaning to the value. Specifically, using instanceof against it provides more context at runtime. Another way to see it is "does it make sense to have an interface? And would anybody extend it?" 2. if it's not designed to be extended, don't allow for it to be extended 3. if this is about mocking, then bad news for who is mocking: this is a value, and it shouldn't be mocked. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#54 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAd4LHYZIYswBUXrytkbnVAgxdfP5Xh9ks5sf6ylgaJpZM4PPZrd> .

mathiasverraes · 2017-09-07T09:08:35Z

PS If people have use cases that Period doesn't support, the answer is not to open up Period or bloat it with features. The answer is to fork it and scratch your own itch.

…

On 7 September 2017 at 11:05, Mathias Verraes ***@***.***> wrote: I'm afraid I don't have time to read the original discussion, so maybe there are actual valid reasons I'm not aware of, but, to be honest, I doubt it. I never feel the need to extend or mock Value Objects. I almost never feel the need to extend _any_ object anymore. All the smart people have been telling us to use composition over inheritance FOR AT LEAST 2 DECADES. Inheritance is dead. Get over it. On 7 September 2017 at 10:43, Marco Pivetta ***@***.***> wrote: > @nyamsprod <https://github.com/nyamsprod> I don't know how the period > class is supposed to be used, but it looks like a value object to me. > > 1. values don't need to be "extended" unless a child class adds more > meaning to the value. Specifically, using instanceof against it > provides more context at runtime. Another way to see it is "does it make > sense to have an interface? And would anybody extend it?" > 2. if it's not designed to be extended, don't allow for it to be > extended > 3. if this is about mocking, then bad news for who is mocking: this > is a value, and it shouldn't be mocked. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#54 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAd4LHYZIYswBUXrytkbnVAgxdfP5Xh9ks5sf6ylgaJpZM4PPZrd> > . >

bpolaszek · 2017-09-07T09:34:24Z

PS If people have use cases that Period doesn't support, the answer is not
to open up Period or bloat it with features. The answer is to fork it and
scratch your own itch.

Hello @mathiasverraes, I understand your point, but I think it's too extreme. PHP libraries, open-sourced on github, packaged on packagist, are made for helping the developers lives, not to complicate them.

One day I had to fork a library to add some additions of 2 other forks. I could not submit a PR because the library's conception made that I need some hard refactorings to enable the features I needed. Now it's a real mess to maintain. I don't want this to be the rule on software engineering.

In my opinion, creating open source libraries on github is to solve common use cases, and not to give a draft to every developer who would have to fork everything to fit his needs.

If people have use cases that Period (or any library) does not support, before giving a ready-made "go fuck yourself and click on fork" answer, maybe it should be interesting to ask some questions:

Is my own need something common?
Does it solve an actual problem?
Is it easy to add this feature in the library without breaking everything?
Maybe the author would be interested in adding these features?

I'm really happy that Period has methods such as moveEndDate or add and that I didn't need to code them myself on my own fork. From my point of view, "bloating" a library of features is relevant as soon as these features are relevant, isn't it?

nyamsprod · 2017-09-07T09:51:38Z

@bpolaszek I get your reasons but they do not invalidate the fact that the class can be made final . If

Your need is something common
Solve an actual problem
Is easy to add without breaking everything

Then the package author/maintainer will add it. Even if the class is final. The final part will also guarantee less BC breaking.

mathiasverraes · 2017-09-07T10:02:22Z

> In my opinion, creating open source libraries on github is to solve > common use cases, and not to give a draft to every developer who would have > to fork everything to fit his needs.

This is the prevailing opinion. DRY all the things, one library that solves all the use cases, bloat bloat bloat. But why? I'm arguing the very opposite. The point of Open Source, and GitHub, is to fork. Use it as is when you can, contribute if it makes sense, but otherwise, learn from it, fork it, adapt it, rebuild it, make something new or better or different or niche. Sure, it will be messy. There will be multitudes of options, some terrible, some wonderful, and various forces will make the most useful ones float to the top. Sounds familiar? Natural selection works like this. Nature forks.

…

On 7 September 2017 at 11:51, ignace nyamagana butera < ***@***.***> wrote: @bpolaszek <https://github.com/bpolaszek> I get your reasons but they do not invalidate the fact that the class can be made final . If - Your need is something common - Solve an actual problem - Is easy to add without breaking everything Then the package author/maintainer will add it. Even if the class is final. The final part will also guarantee less BC breaking. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#54 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAd4LP4FYehi642QMhWMJBB6W2Ynurhyks5sf7yrgaJpZM4PPZrd> .

turanct · 2017-09-07T11:58:12Z

It's intriguing that people still think in terms of extending classes so much.

I've some thoughts to add:

inheritance is something that you mostly don't see in SOLID code, because the extra functionality that the child class adds to that base class is a violation of the Single Responsibility Principle, most of the time.
as a package maintainer, not making a class as final means that you declare the public interface of that class to include extending. From that moment on, (some) users of the package will try to use that functionality, and making the class final again will be equivalent to a BC break for them. Therefore, mark the class as final from the beginning and you can rethink that decision afterwards if you really have to.
as said before, if the object represents a value, mocking it makes no sense at all, it's like saying "pretend that this is 3" and add "pretend that this is 4" to it. Values are to be used as values. So if the reason for removing final is to mock a value, then it's not a valid reason.

mathiasverraes · 2017-09-07T12:17:47Z

"pretend that this is 3"

I'm going to use that one :-D

bpolaszek · 2017-09-07T12:56:09Z

Hello everyone,

At first I did not intend to take part of this topic since I'm not familiar enough with immutability.
But maybe my newbie point of view can help :)

Your replies are really interesting, because PHP practices are changing (I never heard about immutability before PSR-7 or DateTimeImmutable, and I've been implied for years in many legacy PHP4 projects to maintain and upgrade, even now), and that's interesting to see how things evolve. And what I see is that things evolve towards reliability and sustainability, and that's pretty positive.

To my eyes, yes it's a value object, but its role is not only to store a value, it provides factories, functionnalities that compute things and return a result, and given everything we can do with periods in any PHP project, I'm pretty sure that these functionnalities aren't exhaustive.

For years we've been taught to "favor composition over inheritance". I fully aknowledge that making the class final will prevent inheritance and thus possible BC breaks and I agree this is a good thing.
What I don't understand is, how do you compose if you want to extend Period's functionnality (with the Decorator pattern, for example) if there's no PeriodInterface? You just can't? You fork the package as @mathiasverraes suggests?

Thank you,
Ben

Ocramius · 2017-09-07T13:10:42Z

how do you compose if you want to extend Period's functionnality (with the Decorator pattern, for example)

You don't use decoration for this, especially since it's a value.

An example is final class HolidaySeason extends Period { ... } versus final class HolidaySeason { public static function fromPeriod(Period $period) : self { ... } ... }

Fairly sure you're never gonna go with the first one without acutely regretting it later.

bpolaszek · 2017-09-07T13:15:31Z

Of course, but with final class HolidaySeason { public static function fromPeriod(Period $period) : self { ... } ... } you can't inject HolidaySeason in a withPeriod(Period $period) method because HolidaySeason and Period have no relationship at all despite they carry similar values?

Ocramius · 2017-09-07T13:20:28Z

You just add a public function toPeriod() : Period { ... }

Since PHP has no type classes, we are forced into inheritance models: if you inherit from Period, according to the LSP you are also supposed to provide the same behaviour as Period in your child classes. As soon as you have a behaviour/domain mismatch, you got trouble.

shadowhand · 2017-09-07T13:20:56Z

Honestly, I don't remember why I argued against final. There was probably some kind of work-related need for Period extension that made it seem necessary. 🤔

The only problem with the "composition over inheritance" argument is that if you start to write code that type hints against Period and then you need to decorate it, all type hints need to be updated. If Period is going to go back to using final, I would request that an interface be defined as well, so that decorated instances (and return typing, as @Ocramius points out) can be maintained. This would follow the LSP (of SOLID) and allow decoration or proxying Period instances consistently.

Ocramius · 2017-09-07T13:23:09Z

I would request that an interface be defined as well, so that decorated instances (and return typing, as @Ocramius points out) can be maintained.

Except that this is a value with a very well defined domain, whereas other implementations will not respect that

bpolaszek · 2017-09-07T13:43:48Z

@Ocramius I understand but I agree with @shadowhand. I just don't understand the danger of violating LSP if the common point of inheritance (the class or an interface) is only the getters.
What I understood of LSP is that you cant have a Square extend Rectangle in a Rectangle type-hint since you can't set a height on a square without breaking its width.

But what's the possible danger of allowing this?

namespace League\Period;

use DateInterval;
use DatePeriod;
use DateTimeImmutable;
use DateTimeInterface;

// Only read-only methods, no setters
interface PeriodInterface
{

    /**
     * @return DateTimeImmutable
     */
    public function getStartDate(): DateTimeImmutable;

    /**
     * @return DateTimeImmutable
     */
    public function getEndDate(): DateTimeImmutable;

    /**
     * @return float
     */
    public function getTimestampInterval(): float;

    /**
     * @return DateInterval
     */
    public function getDateInterval(): DateInterval;

    /**
     * @param     $interval
     * @param int $option
     * @return DatePeriod
     */
    public function getDatePeriod($interval, int $option = 0): DatePeriod;

    // ...

    /**
     * @param Period $period
     * @return bool
     */
    public function durationGreaterThan(Period $period): bool;

    /**
     * @param Period $period
     * @return bool
     */
    public function durationLessThan(Period $period): bool;
}

namespace League\Period;

final class Period implements PeriodInterface {
// ...
}

namespace My\App;

use League\Period\PeriodInterface;

final class HolidaySeason implements PeriodInterface {
// ...
}

turanct · 2017-09-07T14:06:21Z

As far as I'm concerned, HolidaySeason is a concept that spans a Period, but it's more than a Period, it has meaning, it has context. Declaring it a Period instance would be weird. Let's look at a different example to illustrate:

Say you have a Value Address. You could say that it's a String, and manipulate it as such, but doing that would possibly violate the rules of the concept of an Address. It would not make sense to have a ->toLower() method on the Address. In this case, I would make an Address object that encapsulates a string.

The same goes for the HolidaySeason. There might be rules (e.g. everyone gets 6 consecutive weeks of holidays, so changing the startdate will also change the enddate), thus having methods like ->moveStartDate() which shorten the Period would not make sense in this case. Your HolidaySeason domain object would be better off encapsulating a Period instance and adding meaning and behavior to it, rather than being an instance of Period.

but that's just me probably 👀

bpolaszek · 2017-09-07T14:28:14Z

Hi @turanct, indeed the semantics of the example was not the better.
Let's try something completely different, imagine I need all the same functionnality and period manipulation, except that I want a period of 1 day not to be 2017-09-07 00:00:00 -> 2017-09-08 00:00:00 but 2017-09-07 00:00:00 -> 2017-09-07 23:59:59, then I need to implement my own logic:

namespace League\Period;

use DateInterval;
use DatePeriod;
use DateTimeImmutable;
use DateTimeInterface;

interface PeriodInterface
{
    // same as above
}

namespace League\Period;

final class Period implements PeriodInterface {
    // same as above
}

namespace My\App;

use DateTimeImmutable;
use League\Period\PeriodInterface;

final class MyPeriod implements PeriodInterface {
    
    // ...
    
    /**
     * @return DateTimeImmutable
     */
    public function getEndDate(): DateTimeImmutable
    {
       // I would not really do it this way, but you know what I mean
        return $this->endDate->modify('-1 second'); 
    }

    // etc.

}

My object still represent a period and can be manipulated the same way.

This may be a better example?

nyamsprod · 2017-09-07T14:29:49Z

@bpolaszek If you do that you've already broken Period contract 👎 having methods with the same name does not validate creating an interface if the meaning behind those methods is already different.

shadowhand · 2017-09-07T14:33:46Z

@nyamsprod I don't agree. The result is the same: a DateTimeImmutable. The interface is the contract, and the contract is enforced. I think the example @bpolaszek gave is entirely valid, if the business requirements state that "a day" needs to end at 59:59 instead of 00:00.

nyamsprod · 2017-09-07T14:37:51Z

@shadowhand no it is not as stated in the documentation website

A period consists of a continuous portion of time between two positions in time called datepoints. This library assumes that the starting datepoint is included into the period. Conversely, the ending datepoint is excluded from the specified period. The starting datepoint is always less than or equal to the ending datepoint. The datepoints are defined as DateTimeImmutable objects.

So what he suggests invalidate his implementation of being a true Period object in my book. just returning DateTimeImmutable object does not satisfy the contract as he deliberatly redefined what a Period is. If his code interacts with mine then we will have an issue because we are not talking about the same thing.

shadowhand · 2017-09-07T14:42:00Z

Except that this is a value with a very well defined domain, whereas other implementations will not respect that

@Ocramius whether or not other implementations respect the domain is not a forgone conclusion. Providing the interface is saying "this is how Period works" not "this is what Period does". The "doing" is in the implementation, and a good implementation will respect both the contract (interface) and the spirit (documentation, existing implementation) of Period.

The OCP says that a class must be open to extension. The LSP says that two implementations should be interchangeable. This can either be done by inheritance (👎🏼) or decoration (👍🏼) but LSP would be violated unless there is an interface that can be shared between the decorator and the decorated.

Philosophy aside, if Period becomes final it must have a corresponding interface or it will violate LSP. IMO.

shadowhand · 2017-09-07T14:45:19Z

@nyamsprod sure, fine. The examples given violate some aspect of the "implied contract" of Period. That doesn't mean every implementation will. Throwing out the possibility of using the decorator pattern is, in my opinion, not acceptable.

Ocramius · 2017-09-07T14:49:49Z

You don't need to design to support every pattern: that brings no value at all.

…

On 7 Sep 2017 16:45, "Woody Gilk" ***@***.***> wrote: @nyamsprod <https://github.com/nyamsprod> sure, fine. The examples given violate some aspect of the "implied contract" of Period. That doesn't mean *every* implementation will. Throwing out the possibility of using the decorator pattern is, in my opinion, not acceptable. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#54 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakEU2a5i1FICdzxtCgNjqxz5USuXTks5sgAGAgaJpZM4PPZrd> .

bpolaszek · 2017-09-07T14:59:27Z

You don't need to design to support every pattern: that brings no value at all.

The point is not about the pattern, it is about OCP and LSP. Setting the class as final without interface means that there should be only 1 implementation of a Period, that means it won't be extensible nor interchangeable.

Ocramius · 2017-09-07T15:02:52Z

Which is OK 😋

…

On 7 Sep 2017 16:59, "Beno!t POLASZEK" ***@***.***> wrote: You don't need to design to support every pattern: that brings no value at all. The point is not about the pattern, it is about OCP and LSP. Setting the class as final without interface means that there should be only 1 implementation of a Period, that means it won't be extensible nor interchangeable. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#54 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakJuVvJQqivnsJsjJwZdTvhPEfK_1ks5sgATRgaJpZM4PPZrd> .

turanct · 2017-09-07T15:09:34Z

Also, we're talking about a Value here. I've never seen values being "decorated", simply because there's no use doing it.

Compare it to a Coordinate(x, y), which is a value composed out of two other values. Now, you're saying that you want a Coordinate(2, 3) that when inspected has in fact x = 2 and y = 4! that's absurd. Why not instantiate it as Coordinate(2, 4) then instead?

Same goes for the Period of 1 day, that is actually 1 second less than one day. Just instantiate it differently. This is just a composed Value out of two DateTimeImmutables. There is no way to implement it differently in a sane way. There is no contract. Contracts talk about behavior, while this is a value, like 4 is a value. Values can be compared and worked with by other actors but they don't have any real behavior of their own which can be extracted to an interface.

bpolaszek · 2017-09-07T15:31:06Z

@turanct the problem is not "modifying" the values (Period setters are immutable), the problem is to override the factories.

Simple example:

$period = new Period(new DateTimeImmutable('2017-09-01'), new DateTimeImmutable('today -1 second'));
foreach ($period->split('1 day') as $subperiod) {
    printf('%s => %s' . PHP_EOL, $subperiod->getStartDate()->format('Y-m-d H:i:s'), $subperiod->getEndDate()->format('Y-m-d H:i:s'));
}

Currently outputs:

2017-09-01 00:00:00 => 2017-09-02 00:00:00
2017-09-02 00:00:00 => 2017-09-03 00:00:00
2017-09-03 00:00:00 => 2017-09-04 00:00:00
2017-09-04 00:00:00 => 2017-09-05 00:00:00
2017-09-05 00:00:00 => 2017-09-06 00:00:00
2017-09-06 00:00:00 => 2017-09-06 23:59:59

What if I want this result:

2017-09-01 00:00:00 => 2017-09-01 23:59:59
2017-09-02 00:00:00 => 2017-09-02 23:59:59
2017-09-03 00:00:00 => 2017-09-03 23:59:59
2017-09-04 00:00:00 => 2017-09-04 23:59:59
2017-09-05 00:00:00 => 2017-09-05 23:59:59
2017-09-06 00:00:00 => 2017-09-06 23:59:59

3 options:

I override a non-final Period in a new child class
I create a new MyBusinessPeriod that implement PeriodInterface because final class Period implements PeriodInterface
I fork the Period library

Which one should I choose?

Ocramius · 2017-09-07T15:35:04Z

You know that a factory is generally not an instance method, and therefore doesn't need to be overridden, ever, right?

…

On 7 Sep 2017 17:31, "Beno!t POLASZEK" ***@***.***> wrote: @turanct <https://github.com/turanct> the problem is not "modifying" the values (Period setters are immutable), the problem is to override the factories. Simple example: $period = new Period(new DateTimeImmutable('2017-09-01'), new DateTimeImmutable('today -1 second'));foreach ($period->split('1 day') as $subperiod) { printf('%s => %s' . PHP_EOL, $subperiod->getStartDate()->format('Y-m-d H:i:s'), $subperiod->getEndDate()->format('Y-m-d H:i:s'));} Currently outputs: 2017-09-01 00:00:00 => 2017-09-02 00:00:00 2017-09-02 00:00:00 => 2017-09-03 00:00:00 2017-09-03 00:00:00 => 2017-09-04 00:00:00 2017-09-04 00:00:00 => 2017-09-05 00:00:00 2017-09-05 00:00:00 => 2017-09-06 00:00:00 2017-09-06 00:00:00 => 2017-09-06 23:59:59 What if I want this result: 2017-09-01 00:00:00 => 2017-09-01 23:59:59 2017-09-02 00:00:00 => 2017-09-02 23:59:59 2017-09-03 00:00:00 => 2017-09-03 23:59:59 2017-09-04 00:00:00 => 2017-09-04 23:59:59 2017-09-05 00:00:00 => 2017-09-05 23:59:59 2017-09-06 00:00:00 => 2017-09-06 23:59:59 3 options: - I override a non-final Period in a new child class - I create a new MyBusinessPeriod that implement PeriodInterface because final class Period implements PeriodInterface - I fork the Period library Which one should I choose? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#54 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakPpk8yuqLMT4Pyj4QbrDjFl_ajMqks5sgAw9gaJpZM4PPZrd> .

nyamsprod · 2017-09-07T15:37:44Z

@bpolaszek you are just wrongly using the split method which was not made for that. instead you should use a factory like @Ocramius suggested

mathiasverraes · 2017-09-07T15:37:55Z

@nyamsprod <https://github.com/nyamsprod> sure, fine. The examples given

violate some aspect of the "implied contract" of Period. That doesn't mean *every* implementation will. Throwing out the possibility of using the decorator pattern is, in my opinion, not acceptable. Every implementation that does not return the same values for the same inputs is wrong. Any implementation that returns the exact same values for the same inputs is correct, but has no reason to exist(*) as it's doing the exact same thing. Therefore, value objects do not have commonly offer extensibility or interfaces(***). (*) Except for non-functional concerns, such as better performance under some conditions. Ideally, the one implementation that gets accepted gives the best performance under all conditions. (**) An "implied contract" is no longer implied if it's documented. Ideally of course, it's enforceable. PHP lacks features to enforce this kind of contract, interfaces are very crude. (***) Typing by value can be a fun use case for interfaces with Value Objects: final class Saturday implements Weekday {public function isWeekend():bool{return true;}) (****) (****) Before all the DRY lovers start shouting AbstractWeekendDay: trait Weekend solves this without inheritance, but seriously, in this specific example, don't. DRY is coupling, and you should only trade off coupling against reduced maintenance. Value Objects raraely have a maintenance problem, they tend to be very stable. In other words: final class Saturday implements Weekday {public function isWeekend():bool{return true;}) final class Sunday implements Weekday {public function isWeekend():bool{return true;}) THIS CODE WILL NEVER CHANGE, because if Saturday is not Weekend anymore, the rebellion will rise! Therefore, no maintenance problem, therefore, don't DRY.

…

On 7 September 2017 at 17:09, turanct ***@***.***> wrote: Also, we're talking about a Value here. I've never seen values being "decorated", simply because there's no use doing it. Compare it to a Coordinate(x, y), which is a value composed out of two other values. Now, you're saying that you want a Coordinate(2, 3) that when inspected has in fact x = 2 and y = 4! that's absurd. Why not instantiate it as Coordinate(2,4) then instead? Same goes for the Period of 1 day, that is actually 1 second less than one day. Just instantiate it differently. This is just a composed Value out of two DateTimeImmutables. There is no way to implement it differently in a sane way. There is no contract. Contracts talk about behavior, while this is a value, like 4 is a value. Values can be compared and worked with by other actors but they don't have any real behavior of their own which can be extracted to an interface. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#54 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAd4LLYcL448MLkDbIUg5Ju73YB86df8ks5sgAcwgaJpZM4PPZrd> .

colinodell · 2017-09-07T15:39:11Z

I've never used Period before, but I have implemented similar functionality on my own.

In one particular instance I needed a way to generate different types of periods that shared a common interface:

Periods starting on the 1st of the month and ending on the last day
Periods that are exactly X days long
Periods that are exactly X months long
Periods that start on the US Federal Election date and end 1 day before the next one

The last one is a little complex - US Federal Elections occur on "the first Tuesday following the first Monday in November". I could be wrong, but it seems like Period::next() would not give the proper results.

If I were to use league/period for this particular use case, what's the best way to create something that looks and acts like a Period? I could either code against a PeriodInterface or extend Period - both have their pros and cons, but at least one of these options should be made available for me.

nyamsprod · 2017-09-07T15:48:09Z

@colinodell a Period consists of 2 DateTimeImmutable objects and that's it. How you calculate those 2 datepoints is out of scope. I could have shipped the Period object without any named constructor if I wanted. It's the developper responsability to correclty compute those endpoints. I don't see how extending the Period object would solve this issue ?

mathiasverraes · 2017-09-07T15:52:32Z

I could either code against a PeriodInterface or extend Period - both

have their pros and cons, but at least one of these options should be made available for me. False dichotomy!

…

On 7 September 2017 at 17:48, ignace nyamagana butera < ***@***.***> wrote: @colinodell <https://github.com/colinodell> a Period consists of 2 DateTimeImmutable objects and that's it. How you calculate those 2 datepoints is out of scope. I could have shipped the Period object without any named constructor if I wanted. It's the developper responsability to correclty compute those endpoints. I don't see how extending the Period object would solve this issue ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#54 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAd4LC2gaXrdb1rH98KKkb74UWzXI49fks5sgBA6gaJpZM4PPZrd> .

colinodell · 2017-09-07T15:55:27Z

How you calculate those 2 datepoints is out of scope.

I understand that. However, there's also a next() method which performs exactly that calculation without any way to override it. While it is convenient for some cases, it does not work properly for all of them.

False dichotomy!

That's a fair point. I could create a factory which returns new Period objects. But that doesn't change the fact that a factory already exists which doesn't do what I expect/need and whose behavior I could not change without extending Period or implementing an interface.

colinodell · 2017-09-07T15:57:07Z

FWIW I don't have a strong opinion on whether final is used - just trying to provide an additional viewpoint to further the discussion :)

bpolaszek · 2017-09-07T16:00:23Z

@nyamsprod @colinodell If a Period only consisted of 2 DateTimeImmutable objects, I wouldn't have used your library, I would have used... 2 DateTimeImmutable objects.

What makes your class great is that it can read the combination of these 2 objects under the prism of a date range. next(), split(), add(), etc, are handled with the comfort of validating that the end date will always be greater than the start date: we don't need to code all this stuff and this is why your library is wonderful.

Apart from that, even if your definition of a period is definitely the most common, @colinodell proved this is not the only one. You can have a different way to read 2 DateTimeImmutable objects as a period, but sharing the same common sense and logic you put inside your class. I know this is none of your business and you're not here to deal with those exotic cases.

I think we all agree about making the class final again. Then, apart from managing the PeriodInterface, which should be fine to handle, you don't have to worry about BC again in the future. If people handle Periods differently, it's their problem, not yours.

But what you should state now, is to allow or not people to have a different implementation than yours or not. That doesn't mean you'll need to maintain them.

IMHO, if you prefer to prevent composition, everything you've coded is lost for any developer that need something more specific. In this case, I personnally prefer to code my own specific stuff instead of forking and editing something, because this is harder to maintain, and I don't necessarily need all the functionnality of the library I cannot use. And there's nothing more frustrating than reinventing the whole wheel when you have github and composer in front of you.

nyamsprod · 2017-09-07T16:04:39Z

@bpolaszek correct me if I'm wrong what you are saying is:

make the class final but please provide a PeriodInterface in case

bpolaszek · 2017-09-07T16:54:15Z

That's what I suggest indeed: lock your implementation, allow other implementations. ☺️

colinodell · 2017-09-07T16:58:39Z

I like that approach!

frederikbosch · 2017-09-08T07:44:45Z

IMHO, if you prefer to prevent composition, everything you've coded is lost for any developer that need something more specific. In this case, I personnally prefer to code my own specific stuff instead of forking and editing something, because this is harder to maintain, and I don't necessarily need all the functionnality of the library I cannot use. And there's nothing more frustrating than reinventing the whole wheel when you have github and composer in front of you.

Different domain should have a different solution. Remember that interfaces can be subject to change, especially if you have a value object. How about maintaining that when you have a use case in a (totally) different domain? What if the interface is not suitable for your use case anymore? Your request is like "give me free software that is suitable for me, and if it is not, provide me a solution that I can make it suitable for me, otherwise you frustrate me." We are talking here of a single class. It's an extreme silly argument.

The single and foremost reason to make the object final is to prevent that objects instantiated through this class might become mutable. Value objects are to be immutable for variety of well known reasons. This should be enforced by code, not by hoping that there won't be a developer that extends it and adds a mutable property to the extended class.

nyamsprod · 2017-09-08T08:02:02Z

i know comparison is not reason but JodaTime Interval which does essentially the same thing as Period but in Java is marked as Immutable and Final and I don't recall it having any kind of Interface :) . Having said that,

@bpolaszek If a PeriodInterface was created it would most likely only feature the current API getters and none of the modifying methods which is if I'm correct the most valuable part of the class in your eyes ?

@colinodell yes you are right the Period::next and Period::previous method are badly named :'( I should have called them Period::adjacentAfter and Period::adjacentBefore maybe I should correct that in v4 but that is out of scope for this discussion :)

bpolaszek · 2017-09-08T08:41:34Z

@bpolaszek If a PeriodInterface was created it would most likely only feature the current API getters and none of the modifying methods which is if I'm correct the most valuable part of the class in your eyes ?

Hi @nyamsprod, that's what I said, but after all, it makes no sense. The interface is an API contract for manipulating period objects, including modifiers.

As a consequence, if I typehint a PeriodInterface $period, I should expect $period can create a new PeriodInterface object with $period->withDuration() for example. I formerly said that putting the setters in the interface would break the LSP (actually, only an implementation of that interface could break it). For instance, if I throw a TooLongPeriodException with my own implementation of $holidayPeriod->withDuration('1 year'), I indeed break LSP, but it's my problem, not yours, and thus my own responsibility to deal with it.

@frederikbosch I understand that creating an interface for a single class may seem useless. Usually we don't need this because the final keyword is not widely used and a common practice is to create a child class.

frederikbosch · 2017-09-08T08:58:57Z

@frederikbosch I understand that creating an interface for a single class may seem useless. Usually we don't need this because the final keyword is not widely used and a common practice is to create a child class.

Let's do only things that are widespread and common practise. If the whole world would use that kind of logic, what place would we live in? And since 'not widely used' is subjective, the argument is technically irrelevant. I would advise you to increase your education of value objects, what immutability means, why it makes sense for a value object, and therefore why you would not want to open these classes for mutability.

bpolaszek · 2017-09-08T09:02:15Z

I never said that this common practice was a good one, that's the point :)
PHP does not currently offer the feature to enforce a subclass to be immutable.

So if the only answer to this is to lock any possible extension / composition / inheritance of any PHP project because of that, let's do this.

nyamsprod · 2017-09-12T06:52:45Z

Thanks for the constructive discussion in the next major version Period will be marked as final.

davispuh · 2020-01-13T14:47:13Z

This is stupid. I don't agree at all with this decision. final is not a feature, that's a limitation which prevents adding useful functionality to Period for basically no good reason. Internal methods should just be marked private then you still can refactor them without breaking BC.

Now if I need extended MyPeriod which is 100% same only with some more methods I always have to convert between them.

$myPeriod = new MyPeriod($period);
$myPeriod = $myPeriod->doSomething(); // this could be 99% same as already existing method, now duplication because it's not possible to reuse.
$period = $myPeriod->toPeriod();
someFunction($period); // someFunction might not be under my control, but expects Period as arguemnt

GrahamCampbell · 2020-01-13T15:34:18Z

@davispuh You should instead use composition to achieve what you need. That is what is being imposed by this final key word. Everything you can do with extension, you can also do by thinking in terms of composition.

davispuh · 2020-01-13T18:01:03Z

That is what my example shows, it's not as convenient.

nyamsprod added enhancement question labels Sep 7, 2017

nyamsprod mentioned this issue Sep 7, 2017

New feature: split() with backwards option #53

Merged

nyamsprod closed this as completed Sep 12, 2017

nyamsprod mentioned this issue Jan 28, 2019

Why is Duration no longer final ? jeromegamez/duration-php#2

Closed

nyamsprod pinned this issue Apr 10, 2019

final or not final #54

final or not final #54

Comments

nyamsprod commented Sep 7, 2017 • edited

Issue summary

Ocramius commented Sep 7, 2017

mathiasverraes commented Sep 7, 2017 via email

mathiasverraes commented Sep 7, 2017 via email

bpolaszek commented Sep 7, 2017

nyamsprod commented Sep 7, 2017

mathiasverraes commented Sep 7, 2017 via email • edited

turanct commented Sep 7, 2017 • edited

mathiasverraes commented Sep 7, 2017

bpolaszek commented Sep 7, 2017

Ocramius commented Sep 7, 2017 • edited

bpolaszek commented Sep 7, 2017

Ocramius commented Sep 7, 2017

shadowhand commented Sep 7, 2017 • edited

Ocramius commented Sep 7, 2017

bpolaszek commented Sep 7, 2017 • edited

turanct commented Sep 7, 2017

bpolaszek commented Sep 7, 2017 • edited

nyamsprod commented Sep 7, 2017

shadowhand commented Sep 7, 2017

nyamsprod commented Sep 7, 2017

shadowhand commented Sep 7, 2017 • edited

shadowhand commented Sep 7, 2017

Ocramius commented Sep 7, 2017 via email

bpolaszek commented Sep 7, 2017

Ocramius commented Sep 7, 2017 via email

turanct commented Sep 7, 2017 • edited

bpolaszek commented Sep 7, 2017

Ocramius commented Sep 7, 2017 via email

nyamsprod commented Sep 7, 2017

mathiasverraes commented Sep 7, 2017 via email

colinodell commented Sep 7, 2017 • edited

nyamsprod commented Sep 7, 2017

mathiasverraes commented Sep 7, 2017 via email

colinodell commented Sep 7, 2017 • edited

colinodell commented Sep 7, 2017

bpolaszek commented Sep 7, 2017 • edited

nyamsprod commented Sep 7, 2017

bpolaszek commented Sep 7, 2017

colinodell commented Sep 7, 2017

frederikbosch commented Sep 8, 2017

nyamsprod commented Sep 8, 2017

bpolaszek commented Sep 8, 2017 • edited

frederikbosch commented Sep 8, 2017

bpolaszek commented Sep 8, 2017 • edited

nyamsprod commented Sep 12, 2017

davispuh commented Jan 13, 2020

GrahamCampbell commented Jan 13, 2020

davispuh commented Jan 13, 2020

nyamsprod commented Sep 7, 2017 •

edited

mathiasverraes commented Sep 7, 2017 via email •

edited

turanct commented Sep 7, 2017 •

edited

Ocramius commented Sep 7, 2017 •

edited

shadowhand commented Sep 7, 2017 •

edited

bpolaszek commented Sep 7, 2017 •

edited

bpolaszek commented Sep 7, 2017 •

edited

shadowhand commented Sep 7, 2017 •

edited

turanct commented Sep 7, 2017 •

edited

colinodell commented Sep 7, 2017 •

edited

colinodell commented Sep 7, 2017 •

edited

bpolaszek commented Sep 7, 2017 •

edited

bpolaszek commented Sep 8, 2017 •

edited

bpolaszek commented Sep 8, 2017 •

edited