Fixes #16247: clone behaviors when cloning components

[brandonkelly]

Q	A
Is bugfix?	yes
New feature?	no
Breaks BC?	yes
Tests pass?	yes
Fixed issues	#16247

[samdark]

Merged. Thanks!

[rob006]

Why events are not cloned in the same way?

[samdark]

Should be cloned as well, I think. Want to make a pull request?

[brandonkelly]

Events don’t seem as obvious as behaviors. Cloning an object is basically a shortcut to defining a new object with the same class and setting the same properties on it - and if behaviors are ment to act as extensions of the class, that definitely seems like it should be cloned too. But events are intentionally registered either at the class level (already will work on a clone) or for a specific instance. Maybe it's not intended that the events should be set on the clone as well though.

[samdark]

Hmm... right. That's controversial.

[rob006]

Events don’t seem as obvious as behaviors.

I don't see any difference. It looks inconsistent to me, especially that events attached from behaviors are cloned, but events attached manually are not.

But AFAIK there will be some changes in events handling in 3.0 so it may be irrelevant after all.

[brandonkelly]

especially that events attached from behaviors are cloned

Events added from behaviors are essentially the class dynamically defining its own business logic. The non-behavior equivelant would have been for the owner class to just call some internal method alongside firing the event.

I’m not saying it definitely doesn’t make sense for events to be cloned as well; just that it’s not always going to be expected behavior. For example, if I did this:

$obj = new MyObject();
$obj->on('afterDoSomething', function() {
    // ...
});

$obj2 = clone $obj;
$obj2->doSomething();

I don’t think expected behavior would be that the afterDoSomething handler would get called for $obj2. If you disagree, maybe it’s worth opening this up as a new issue for debate there. This PR is probably the wrong place to discuss it :)

[rob006]

@brandonkelly Let's just agree to disagree. For me current behavior is inconsistent and I don't see any reason why dynamically attached behaviors and its events should be handled differently than dynamically attached events. But this is not important for me, I almost never use this feature anyway. I just wanted to point it now, because fixing this inconsistency may require BC break (you cannot just clone all events because cloned behaviors already attaches its events, so you will get duplicated events).

[samdark]

Let's not clone events for now.