Carbon::addMonths() vs Carbon::addMonthsNoOverflow() and mutability #621

Closed
travisamiller opened this Issue Feb 26, 2016 · 3 comments

Comments

3 participants
@travisamiller

I was bit by (a bug in?) addMonthsNoOverflow() today as it returns a copy() while other mutators seem to return $this. This issue was mentioned here in PR #239 but there doesn't seem to have been further comment.

Does addMonthsNoOverflow() differ on purpose? Would a PR bringing it in line with the other mutators be considered?

Example:

$start = new Carbon('2016-01-01 00:00:00');
$start->addMonths(1);
var_dump($start->toDateTimeString() === '2016-02-01 00:00:00'); // true

$start = new Carbon('2016-01-01 00:00:00');
$start->addMonthsNoOverflow(1);
var_dump($start->toDateTimeString() === '2016-02-01 00:00:00'); // false (?!)

lucasmichot added a commit to lucasmichot/Carbon that referenced this issue Feb 27, 2016

@lucasmichot

This comment has been minimized.

Show comment
Hide comment
Collaborator

lucasmichot commented Feb 27, 2016

@travisamiller

This comment has been minimized.

Show comment
Hide comment

👍

@judgej

This comment has been minimized.

Show comment
Hide comment
@judgej

judgej Apr 19, 2016

For now, I'm using copy() explicitly with this method, just to make sure the behaviour in my application does not change and catch me out later. Having it working consistently is fine, but surprise changes to functionality are not so much fun.

$myNewDate = $date->copy()->addMonthsNoOverflow($number);

It took me a while to find this method, as it is not documented. It can also be described by another name: add calendar months, as they is what this actually does.

TBH, treating the Carbon object as a value object is really the way to go.

judgej commented Apr 19, 2016

For now, I'm using copy() explicitly with this method, just to make sure the behaviour in my application does not change and catch me out later. Having it working consistently is fine, but surprise changes to functionality are not so much fun.

$myNewDate = $date->copy()->addMonthsNoOverflow($number);

It took me a while to find this method, as it is not documented. It can also be described by another name: add calendar months, as they is what this actually does.

TBH, treating the Carbon object as a value object is really the way to go.

lucasmichot added a commit to lucasmichot/Carbon that referenced this issue May 3, 2016

@briannesbitt briannesbitt closed this in #622 Jun 25, 2016

briannesbitt added a commit that referenced this issue Jun 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment