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

Cloning objects should also clone their behaviors #16247

Closed
brandonkelly opened this issue May 8, 2018 · 3 comments
Closed

Cloning objects should also clone their behaviors #16247

brandonkelly opened this issue May 8, 2018 · 3 comments

Comments

@brandonkelly
Copy link
Contributor

Currently, Component::__clone() will null-out the $_behaviors array:

public function __clone()
{
$this->_events = [];
$this->_eventWildcards = [];
$this->_behaviors = null;
}

I can see why this is better than leaving $_behaviors alone (those behaviors contain a reference to the original component instance), but I think it would be better if those behaviors were cloned to the new instance as well.

public function __clone()
{
    $this->_events = [];
    $this-> _eventWildcards = [];

    if ($this->_behaviors !== null) {
        $behaviors = array_merge($this->_behaviors);
        $this->_behaviors = null;
        foreach ($behaviors as $name => $behavior) {
            $this->attachBehavior($name, clone $behavior);
        }
}

That way any property values set on the behavior will be maintained on the cloned instance, just like normal properties.

$obj = new MyClassWithBehavior();
$obj->myBehaviorProperty = 'bar';
$obj2 = clone $obj;
echo $obj2->myBehaviorProperty; // should output 'bar'

(related: craftcms/cms#2857)

@samdark samdark added this to the 2.1.0 milestone May 8, 2018
@samdark
Copy link
Member

samdark commented May 8, 2018

Yes. It's not backwards compatible and could be done in 2.1. Would you like to send a pull request?

@brandonkelly
Copy link
Contributor Author

I figured it would be more of a 2.1 thing. Happy to submit a PR.

@brandonkelly
Copy link
Contributor Author

@samdark ICYMI a PR was submitted for this (#16291).

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

No branches or pull requests

2 participants