Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Still mutable #2

Closed
beberlei opened this issue Sep 12, 2012 · 13 comments
Closed

Still mutable #2

beberlei opened this issue Sep 12, 2012 · 13 comments

Comments

@beberlei
Copy link

@beberlei beberlei commented Sep 12, 2012

The most annoying thing about PHP DateTime is that they objects are mutable. It seems this is not changed in Carbon?

@briannesbitt
Copy link
Owner

@briannesbitt briannesbitt commented Sep 12, 2012

Correct. There are no immediate plans to change this. However, I don't think it would take too much to create a subclass that used a copy on write strategy for mutating function calls.

@marijn
Copy link

@marijn marijn commented Sep 12, 2012

I agree with @beberlei, the fact that DateTime is mutable is very annoying.

Please consider making this immutable.

@briannesbitt
Copy link
Owner

@briannesbitt briannesbitt commented Sep 17, 2012

So would a copy on write implementation fit the bill? Thoughts?

@marijn
Copy link

@marijn marijn commented Sep 17, 2012

Anything that modifies the internal state should return a new instance.

<?php
public function modify($mod)
{
    $instance = clone $this;

    $instance->modify($mod);

    return $instance;
}
@briannesbitt
Copy link
Owner

@briannesbitt briannesbitt commented Sep 17, 2012

Yes, I get that.. hence the copy on write implementation comment. Is that what you guys are looking for?

@rjkip
Copy link

@rjkip rjkip commented Nov 23, 2012

I agree, this behaviour is annoying, but wouldn't it break BC? What about a copy() method to allow chaining?

<?php

# Hmm, no pun intended here...
$carbon->copy()->modify("midnight");

Perhaps the copy on write implementation should wait for a BC breaking v2.0.0?

@rjkip
Copy link

@rjkip rjkip commented Nov 23, 2012

Meh, never mind. It already exists... :x

@briannesbitt
Copy link
Owner

@briannesbitt briannesbitt commented Aug 7, 2013

No further action here... going to close it for now.

@falk
Copy link

@falk falk commented Aug 11, 2013

@briannesbitt
Copy link
Owner

@briannesbitt briannesbitt commented Aug 11, 2013

Its not high on my list right now. As I have said though, I think a copy on write implementation would be pretty quick.

@beberlei
Copy link
Author

@beberlei beberlei commented Aug 11, 2013

@falk the issues of the mailing list were completly fixed, 5.5 has an immutable date time that works flawlessly.

briannesbitt pushed a commit that referenced this issue Jun 26, 2016
Fix Zh translation and add tests
@amitailanciano
Copy link

@amitailanciano amitailanciano commented Sep 15, 2016

+1, would love to see this -- remembering to chain ->copy() all the time is pretty annoying/frustrating

@tabennett
Copy link

@tabennett tabennett commented Oct 23, 2017

+1, this just bit us today at work. Side-effects suck and make Carbon more difficult to use as a value object.

Ovsyanka pushed a commit to Ovsyanka/Carbon that referenced this issue Jan 11, 2018
Update risky & skipped tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.