PHP clone on object w/ behaviors produces unexpected cloned object

[nateiler]

I noticed that cloning an object that has behaviors (such as custom fields) results in the behaviors not being copied. So, a query filtering via custom field would result in the cloned query not producing the same criteria.

A quick little example where a custom Plain Text field 'position' is configured for a user.

$query = \craft\elements\User::find();
$query->position = 'President';
echo $query->position ?: 'EMPTY';
$newQuery = clone $query;
echo $newQuery->position ?: 'EMPTY';

And idea on how to get around this (except not use behaviors)?

[nateiler]

After a little digging, I found the culprit:

https://github.com/yiisoft/yii2/blob/3d96a45f6b2ded171c607e20b9f3e39a618ab9cb/framework/base/Component.php#L307

Given the usage of the content behavior, my assumption here is behaviors SHOULD be cloned ... right?

[nateiler]

Obviously (since fields are managed via the same behavior), the same applies to elements:

$currentUser = Craft::$app->getUser()->getIdentity();
$currentUser->position = 'President';
echo $currentUser->position ?: 'EMPTY';
$newUser = clone $currentUser;
echo $newUser->position ?: 'EMPTY';

We're using the \craft\db\Query::EVENT_DEFINE_BEHAVIORS and \craft\base\Component::EVENT_DEFINE_BEHAVIORS to apply similar 'field like' capabilities to objects as well so I'm particularly interested in your position on this.

[brandonkelly]

I think the reason they do this is because clone produces a shallow copy of the object, and each behavior holds a reference to the parent object, via $this->owner. So if they didn’t clear out the behaviors, yes you would still get your position property, but the flip side is that it would live on the same behavior instance as $query, and its $owner property would still reference $query, not $newQuery.

That said, it does seem like a better solution would have been

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

I'll talk to the Yii people about that. In the meantime you can work around this by doing

$newQuery = clone $query;
foreach ($query->getBehaviors() as $name => $behavior) {
    $newQuery->attachBehavior($name, clone $behavior);
}

[nateiler]

Right.

Since we don't have access to the base component behaviors property anything extending it could do:

/**
 * @inheritdoc
 */
public function __clone()
{
    $behaviors = $this->getBehaviors();

    parent::__clone();

    foreach($behaviors as $name => $behavior) {
        $this->attachBehavior($name, clone $behavior);
    }
}

[brandonkelly]

Yes, that’s how you could do it from a component class in lieu of a Yii fix. But i’m hesitant to start making that change on core Craft classes, if there’s a chance it could be solved from the core Component class instead.

[narration-sd]

Good to hear this discussion from you guys, as I'd run into the same situation end of last week.

I solved the need then by using Craft functions like entry->setFieldValuesFromRequest(), since I had that data available, but other apparent means to get a proper deep copy weren't that useful.

Brandon's idea here looks a good way forward, and reminds me of how many other abilities he's helpet the Yii people to make useful.

For me, it kind of points towards all the php people have been doing as well as Yii, to re-invent what we had in objects I am not going to say how many years ago, having gotten in on the ground floor before C++ was public, and when Smalltalk was in flower. The trouble seems to be that they patch together, step after step, without much apparent attention to what's been known about the tougher cases, or indeed, how persons are going to need to use the results in a real world.

Probably a poster child for this would be, again, deep clone (or just visit) abilities, in Php itself, for which innings were held here also not long ago. And anyway, the state of documentation on both programs, etc., which is only alleviated by postings on various degrees of solutions others may generate.

Ideas alone don't answer needs by themselves, and it's great that you're as able as you are to handle both the application and system worlds, Brandon. Appreciated, and you also Nate, for getting in there.

[nateiler]

@brandonkelly any word from the Yii folks?

[angrybrad]

@nateiler here's the related issue and pull request. slated for Yii 2.1: yiisoft/yii2#16247

[nateiler]

2.1 has a lot of breaking/compatibility changes. Are you thinking Craft 4?

[brandonkelly]

Yeah.

[brandonkelly]

Went ahead and rushed a fix for this, as there are core bugs that are affected by this (#3019 and unexpected clone() Twig function behavior).

As of the next release (3.0.13), all classes that extend craft\base\Model or craft\db\Query will include behaviors in their clones.

[nateiler]

Nice! I thought I remembered seeing 'clone' when getting descendants but didn't want to go down that rabbit hole.

Thanks!

[brandonkelly]

And Yii just accepted a PR to fix this from yii\base\Component::__clone() for 3.0.0 🎉