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

Removing event handler while handling events fails #1191

Closed
yuehei opened this issue Aug 10, 2012 · 10 comments
Closed

Removing event handler while handling events fails #1191

yuehei opened this issue Aug 10, 2012 · 10 comments
Assignees
Labels
Milestone

Comments

@yuehei
Copy link

@yuehei yuehei commented Aug 10, 2012

public function saveGlobalState()
{
    if($this->_stateChanged)
    {
        $this->_stateChanged=false;
        $this->detachEventHandler('onEndRequest',array($this,'saveGlobalState'));
        $this->getStatePersister()->save($this->_globalState);
    }
}

CListIterator used reference, when performing this event remove event bind, and can lead to Array out of bound in raiseEvent

@cebe
Copy link
Member

@cebe cebe commented Nov 30, 2012

Unable to reproduce.
Can you create a test case that reproduces the issue?

Loading

@ghost ghost assigned cebe Nov 30, 2012
cebe added a commit that referenced this issue Dec 12, 2012
can not reproduce the issue
@cebe
Copy link
Member

@cebe cebe commented Dec 12, 2012

Tried to reproduce. Written a test in 23e32a7.

Loading

cebe added a commit to cebe/yii that referenced this issue Dec 19, 2012
@cebe
Copy link
Member

@cebe cebe commented Dec 19, 2012

Thanks for your comment in 23e32a7, I am now able to reproduce the issue: fb2cbc3

I changed the title of this issue to describe the general issue a bit better.

Loading

@cebe
Copy link
Member

@cebe cebe commented Dec 19, 2012

https://github.com/cebe/yii/compare/1191-remove-eventhandler-in-event

The Problem is that CListIterator->_c does not get updated when an item gets removed from data.
Also expected behavior would be the next event handler to be executed which is not the case when we only fix the adjustment of CListIterator->_c. Quite a complex thing to fix this though.

Loading

@cebe
Copy link
Member

@cebe cebe commented Feb 13, 2013

@yuehei is there a valid use case for this?

Loading

@tjconcept
Copy link

@tjconcept tjconcept commented Mar 3, 2013

I ran into this because I tried to do something like:

$app = Yii::app();
$app->setGlobalState('count', $app->getGlobalState('count', 0)+1);
echo $app->getGlobalState('count', 0);

Yii::app()->onEndRequest = function() {
    echo 'Test';
};

Loading

@Frostecho
Copy link

@Frostecho Frostecho commented Jan 10, 2014

I think we can use the $this->_ended variable to fix this bug, for example:

public function saveGlobalState()
{
    if($this->_stateChanged)
    {
        $this->_stateChanged=false;
        if(!$this->_ended)
            $this->detachEventHandler('onEndRequest',array($this,'saveGlobalState'));
        $this->getStatePersister()->save($this->_globalState);
    }
}

Loading

@cebe
Copy link
Member

@cebe cebe commented Jan 10, 2014

might fix it but looks like a hacky workaround. need to search for a better one.

Loading

@GodsBoss
Copy link

@GodsBoss GodsBoss commented Jul 28, 2014

Can it be fixed in context of CComponent/CList/CListIterator alone? https://gist.github.com/GodsBoss/71994b5ed1c0b43a2a6d seems like a reasonable use case, where no event handling occurs at all.

Loading

@cebe cebe removed this from the 1.1.16 milestone Aug 15, 2014
@cebe cebe added this to the 1.1.17 milestone Aug 15, 2014
@steven-hadfield
Copy link
Contributor

@steven-hadfield steven-hadfield commented Sep 11, 2014

I'm not sure what all of the implications would be, but what if CListIterator::current() and CComponent::raiseEvent() were modified to gracefully handle the missing data:

CListIterator::current()

    if (isset($this->_d[$this->_i])) {
        return $this->_d[$this->_i];
    }

CComponent::raiseEvent

    foreach($this->_e[$name] as $handler) {
        if (null === $handler) {
            continue;
        }

That seems like the most graceful solution. Another option would be to make CList::iterator() return an immuntable iterator by cloning the data and prevent passing _d by reference, but I think this would have far more reaching implications.

Loading

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

Successfully merging a pull request may close this issue.

7 participants