destroy method in yii.captcha.js doesn't work as expected

[arogachev]

Let's say we want to use captcha plugin on some image:

<img id="captcha" src="/site/captcha/">

jQuery plugin initialization:

$('#captcha').yiiCaptcha({
    refreshUrl: '/site/captcha?refresh=1',
    hashKey: 'yiiCaptcha/site/captcha'
});

Then we decide to destroy it:

$('#captcha').yiiCaptcha('destroy');

With each repeated initialization:

$('#captcha').yiiCaptcha({
    refreshUrl: '/site/captcha?refresh=1',
    hashKey: 'yiiCaptcha/site/captcha'
});

click on the captcha will generate 1 more extra AJAX request. This is because click event handler was not removed correctly. Here is the current implementation:

destroy: function () {
    return this.each(function () {
        $(window).unbind('.yiiCaptcha');
        $(this).removeData('yiiCaptcha');
    });
},

Why unbind event handlers from window and in the loop which is even more weird? We call destroy on specific element(s), so this will be correct:

destroy: function () {
    return this.each(function () {
        var $e = $(this);
        $e.unbind('.yiiCaptcha');
        $e.removeData('yiiCaptcha');
    });
},

Also loop is completely useless here because this is jQuery object on which destroy called from, we can unbind event handlers and remove data without any loops here:

destroy: function () {
    var $e = $(this);
    $e.unbind('.yiiCaptcha');
    $e.removeData('yiiCaptcha');
    return $e;
},

Further improvements:

• this is already jQuery object, so there is no need to wrap it with $(...).

• From docs to unbind:

As of jQuery 3.0, .unbind() has been deprecated. It was superseded by the .off() method since jQuery 1.7, so its use was already discouraged.

And by the way event handler is initially attached with .on:

$e.on('click.yiiCaptcha', function () {
    methods.refresh.apply($e);
    return false;
});

so it's better to remove it in similar fashion.

So the final replacement will be:

destroy: function () {
    this.unbind('.yiiCaptcha');
    this.removeData('yiiCaptcha');
    return this;
},

I guess this problem did not pop out eariler because the usage of captcha is usually limited to just tuning it on server side without making any JS changes, and lack of tests obviously.

In #12936 I haven't posted problems in separate issues, so do it for yii.captcha.js now to be more clear.

The same applies to other JS plugins such as yii.gridView.js.

[dynasource]

it's very good to reassess the JS code with all the dust on it. Let's create tests first and then update the core code.

[arogachev]

@dynasource This was blocking the entire test suite for captcha plugin because after each test destroy is called to simulate initialization from scratch. So I think fixing it along with writing tests is OK in this case. But I agree that for "cosmetic" fixes like removing linebreak it's better to open separate PRs.

[SilverFire]

The issue should be closed, right?

[arogachev]

@SilverFire Yes. But I started to write tests for yii.gridView.js and found that exactly the same issue exists there. Should we make it common for all framework's JS plugins? I think this will be better rather than creating similar issues for each plugin. If you are agree, I can modify the title and description, and remove accoring changelog line, then apply modified changelog line and close this issue only when it will be fixed everywhere.

[SilverFire]

Ok, thank you!

[arogachev]

@SilverFire It's quite different for yii.gridView.js, so I think it's better to have a separate issue with explanation and details for it. I'm closing this one as already solved.