Yii2: Behavior event doesn't detach

[laxity7]

Behavior event is not detach if handler is Closure.

Simple example:

$obj = new class () extends Component
{
    public $foo = 'foo';
};
$obj->attachBehavior('bar', (new class () extends Behavior
{
    public function events()
    {
        return [
            'barEventOnce' => function ($event) {
                echo $this->owner->foo;
                $this->detach(); // like that I intended to do
            },
        ];
    }
}));

$obj->trigger('barEventOnce'); // it's ok, see "foo"
$obj->trigger('barEventOnce'); // exception, because owner is null

Actually, I did the behavior for a model, the event in which was supposed to work only once. The handler of an event at me also anonymous function, but an event does not get rid and by a repeated call I receive a mistake.
It turns out that the behavior got rid, and the event does not, because of here such check in the class Component

if ($event[0] === $handler) {

I replaced Closure with a class function, but was unpleasantly surprised by this behavior. Maybe add this remark to the documentation or fix this error?

Yii version : 2.0.16
PHP version: 5.6/7.1

[laxity7]

The problem is not only in behavior, but global, therefore best solution is to compare functions in Component in off function, like this:

public function off($name, $handler = null)
{
    $this->ensureBehaviors();
    if (empty($this->_events[$name]) && empty($this->_eventWildcards[$name])) {
        return false;
    }
    if ($handler === null) {
        unset($this->_events[$name], $this->_eventWildcards[$name]);
        return true;
    }

    $removed = false;
    // just an example of a function to compare. Need to improve and move
    $isClosureEqual = function (\Closure $func1, \Closure $func2) {
        $refFunc = new \ReflectionFunction($func1);
        $sign1 = $refFunc->getFileName() . '|' . $refFunc->getStartLine();

        $refFunc = new \ReflectionFunction($func2);
        $sign2 = $refFunc->getFileName() . '|' . $refFunc->getStartLine();

        return $sign1 === $sign2;
    };

    // plain event names
    if (isset($this->_events[$name])) {
        foreach ($this->_events[$name] as $i => $event) {
            if ($handler instanceof \Closure) {
                $handlersEqual = $isClosureEqual($event[0], $handler);
            } else {
                $handlersEqual = $event[0] === $handler;
            }

            if ($handlersEqual) {
                unset($this->_events[$name][$i]);
                $removed = true;
            }
        }
        if ($removed) {
            $this->_events[$name] = array_values($this->_events[$name]);
            return $removed;
        }
    }

    // wildcard event names
    if (isset($this->_eventWildcards[$name])) {
        foreach ($this->_eventWildcards[$name] as $i => $event) {
            if ($handler instanceof \Closure) {
                $handlersEqual = $isClosureEqual($event[0], $handler);
            } else {
                $handlersEqual = $event[0] === $handler;
            }

            if ($handlersEqual) {
                unset($this->_eventWildcards[$name][$i]);
                $removed = true;
            }
        }
        if ($removed) {
            $this->_eventWildcards[$name] = array_values($this->_eventWildcards[$name]);
            // remove empty wildcards to save future redundant regex checks:
            if (empty($this->_eventWildcards[$name])) {
                unset($this->_eventWildcards[$name]);
            }
        }
    }

    return $removed;
}

But this code badly smells. It is necessary to take out check to other place or to improve.
@samdark , if you tell me how best to organize the code or improve, I will do PR and tests

[samdark]

@laxity7 isClosureEqual looks suspicious to me. Won't it fail if two functions start with the same line?

[GHopperMSK]

How it is possible to compare two functions? There aren't methods, there aren't members, there are only results and we can't compare them. It is objects, not pointers. They can have equal input params but in the same time do absolutely different works.

[rob006]

What if we cache events() result? AFAIK problem exist because two events() calls returns two different closures.

[GHopperMSK]

I see two possible solutions:

1. Deny Closure objects in event list.
2. If a Behavior contains a Closure event, removing the last one leads to removing the Behavior itself (as I already done). All we need - discribe this situation in the documentation.

If somebody for some reason wants to use anonymous function as an event, let him achieve the point by keeping the event within separate Behavior object.

[samdark]

Solution 2 looks better than solution 1.

[laxity7]

@GHopperMSK ,

1. If a Behavior contains a Closure event, removing the last one leads to removing the Behavior itself (as I already done). All we need - discribe this situation in the documentation.

Your solution removes all subscribers with the event name.
As long as there is no Closure comparison method in php, the only correct solution is to describe in the documentation that if you use anonymous functions as a handler, then you will not be able to detach it.

[samdark]

Yes. That's even better :)

[rob006]

As long as there is no Closure comparison method in php

You can compare Closures. But since Closure is an object, you need to compare the same instance. The solution would be to store reference to attached closure and use it for detaching.

[laxity7]

I understand. But in the current implementation, this will require, as it seems to me, unjustifiably many changes. Of course, if we are talking about behaviors, we can use a cache for the events function, like this:

private $events = null;

public function events()
{
    if ($this->events === null) {
        $this->events = [
            'barEventOnce' => function () {
                echo $this->owner->foo;
                $this->detach();
            },
        ];
    }

    return $this->events;
}

[rob006]

I was thinking more about this on attach:

        foreach ($this->events() as $event => $handler) {
            $this->_attachedEvents[$event] = $handler;
            $owner->on($event, is_string($handler) ? [$this, $handler] : $handler);
        }

And the on detach:

            foreach ($this->_attachedEvents as $event => $handler) {
                $this->owner->off($event, is_string($handler) ? [$this, $handler] : $handler);
            }

These are literally 2-3 lines of code do change/add.

[laxity7]

Yes, it's even better.
Tested, works correctly

[samdark]

@GHopperMSK what do you think about it?

[GHopperMSK]

I like the idea. Clear elegant solution in a few rows of code. Have tested it and everything works fine for me.

[GHopperMSK]

Wait a minute! Isn't the handler is an object instance?
$this->_attachedEvents[$event] = $handler;
If so, then we face the same issue.

https://github.com/yiisoft/yii2/blob/703644864ece69d5d3203e444fd6e9023fd66e85/framework/base/Component.php#L577-L580

[rob006]

@samdark Why do you think that issue is not straightforward? AFAIK solution from https://github.com/yiisoft/yii2/issues/17223#issuecomment-479203554 should work without any trouble.

[samdark]

How about https://github.com/yiisoft/yii2/issues/17223#issuecomment-479422140 comment?

[rob006]

I don't understand this comment. You can compare objects, but === will return true only when you're comparing the same instances. The whole point of solution from https://github.com/yiisoft/yii2/issues/17223#issuecomment-479203554 is to keep original instance to use it to === compare.

[samdark]

Right. Yes, seems to be OK to have it.