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.

Sign up for GitHub

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

Jump to bottom

[BUG] Phalcon\Events\Manager->attach('event') fails when preceded by ->dettach('event'). #1337

Merged
merged 4 commits into from
Oct 6, 2013
Merged

[BUG] Phalcon\Events\Manager->attach('event') fails when preceded by ->dettach('event'). #1337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2bb68a5
Bug 1331 - detachAll listeners from EventsManager
Cinderella-Man Oct 5, 2013
58489a8
Bug 1331 - detachAll listeners from EventsManager
Cinderella-Man Oct 5, 2013
7cc18b4
Bug 1331 - detachAll listeners from EventsManager changes
Cinderella-Man Oct 5, 2013
6e6f3c9
Deprecated warnings disabled for dettachAll() method
Cinderella-Man Oct 5, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions ext/events/manager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,9 +231,9 @@ PHP_METHOD(Phalcon_Events_Manager, getResponses){
*
* @param string $type
*/
PHP_METHOD(Phalcon_Events_Manager, dettachAll){
PHP_METHOD(Phalcon_Events_Manager, detachAll){

zval *type = NULL, *events = NULL, *null_value;
zval *type = NULL, *events = NULL;

PHALCON_MM_GROW();

Expand All @@ -249,11 +249,10 @@ PHP_METHOD(Phalcon_Events_Manager, dettachAll){
PHALCON_INIT_NVAR(events);
} else {
if (phalcon_array_isset(events, type)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can drop this check — unset() will suceed regardless whether $events[$type] is set. This will save us one lookup :-)

PHALCON_INIT_VAR(null_value);
phalcon_array_update_zval(&events, type, &null_value, PH_COPY | PH_SEPARATE);
phalcon_array_unset(&events, type, PH_SEPARATE);
}
}

phalcon_update_property_this(this_ptr, SL("_events"), events TSRMLS_CC);

PHALCON_MM_RESTORE();
Expand Down
9 changes: 5 additions & 4 deletions ext/events/manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ PHP_METHOD(Phalcon_Events_Manager, arePrioritiesEnabled);
PHP_METHOD(Phalcon_Events_Manager, collectResponses);
PHP_METHOD(Phalcon_Events_Manager, isCollecting);
PHP_METHOD(Phalcon_Events_Manager, getResponses);
PHP_METHOD(Phalcon_Events_Manager, dettachAll);
PHP_METHOD(Phalcon_Events_Manager, detachAll);
PHP_METHOD(Phalcon_Events_Manager, fireQueue);
PHP_METHOD(Phalcon_Events_Manager, fire);
PHP_METHOD(Phalcon_Events_Manager, hasListeners);
Expand All @@ -47,7 +47,7 @@ ZEND_BEGIN_ARG_INFO_EX(arginfo_phalcon_events_manager_collectresponses, 0, 0, 1)
ZEND_ARG_INFO(0, collect)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO_EX(arginfo_phalcon_events_manager_dettachall, 0, 0, 0)
ZEND_BEGIN_ARG_INFO_EX(arginfo_phalcon_events_manager_detachall, 0, 0, 0)
ZEND_ARG_INFO(0, type)
ZEND_END_ARG_INFO()

Expand Down Expand Up @@ -78,11 +78,12 @@ PHALCON_INIT_FUNCS(phalcon_events_manager_method_entry){
PHP_ME(Phalcon_Events_Manager, collectResponses, arginfo_phalcon_events_manager_collectresponses, ZEND_ACC_PUBLIC)
PHP_ME(Phalcon_Events_Manager, isCollecting, NULL, ZEND_ACC_PUBLIC)
PHP_ME(Phalcon_Events_Manager, getResponses, NULL, ZEND_ACC_PUBLIC)
PHP_ME(Phalcon_Events_Manager, dettachAll, arginfo_phalcon_events_manager_dettachall, ZEND_ACC_PUBLIC)
PHP_ME(Phalcon_Events_Manager, detachAll, arginfo_phalcon_events_manager_detachall, ZEND_ACC_PUBLIC)
PHP_ME(Phalcon_Events_Manager, fireQueue, arginfo_phalcon_events_manager_firequeue, ZEND_ACC_PUBLIC)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like this:

PHP_METHOD(Phalcon_Events_Manager, detachAll);

/* ... */
PHP_ME(Phalcon_Events_Manager, detachAll, arginfo_phalcon_events_manager_detachall, ZEND_ACC_PUBLIC)
PHP_MALIAS(Phalcon_Events_Manager, dettachAll, detachAll, arginfo_phalcon_events_manager_detachall, ZEND_ACC_PUBLIC | ZEND_ACC_DEPRECATED)

Notes:

  • you need only one arginfo as both methods are the same
  • there will be only one method implementation — PHP_METHOD(Phalcon_Events_Manager, detachAll);; no need to declare dettachAll anywhere as it will be created as an alias in the method table by Zend Engine.

PHP_ME(Phalcon_Events_Manager, fire, arginfo_phalcon_events_manager_fire, ZEND_ACC_PUBLIC)
PHP_ME(Phalcon_Events_Manager, hasListeners, arginfo_phalcon_events_manager_haslisteners, ZEND_ACC_PUBLIC)
PHP_ME(Phalcon_Events_Manager, getListeners, arginfo_phalcon_events_manager_getlisteners, ZEND_ACC_PUBLIC)
PHP_ME(Phalcon_Events_Manager, getListeners, arginfo_phalcon_events_manager_getlisteners, ZEND_ACC_PUBLIC)
PHP_MALIAS(Phalcon_Events_Manager, dettachAll, detachAll, arginfo_phalcon_events_manager_detachall, ZEND_ACC_PUBLIC | ZEND_ACC_DEPRECATED)
PHP_FE_END
};

155 changes: 149 additions & 6 deletions unit-tests/EventsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,15 @@ class LeDummyComponent

protected $_eventsManager;

public function setEventManager($eventsManager)
public function setEventsManager($eventsManager)
{
$this->_eventsManager = $eventsManager;
}

public function getEventsManager()
{
return $this->_eventsManager;
}

public function leAction()
{
Expand All @@ -41,7 +46,7 @@ class LeAnotherComponent

protected $_eventsManager;

public function setEventManager($eventsManager)
public function setEventsManager($eventsManager)
{
$this->_eventsManager = $eventsManager;
}
Expand Down Expand Up @@ -125,10 +130,10 @@ public function testEvents()
$eventsManager->attach('dummy', $listener);

$component = new LeDummyComponent();
$component->setEventManager($eventsManager);
$component->setEventsManager($eventsManager);

$another = new LeAnotherComponent();
$another->setEventManager($eventsManager);
$another->setEventsManager($eventsManager);

$component->leAction();
$component->leAction();
Expand Down Expand Up @@ -156,9 +161,9 @@ public function testEvents()

/*
//This is failling :(
$eventsManager->dettach('dummy', $listener);*/
$eventsManager->detach('dummy', $listener);*/

/*$eventsManager->dettachAll('dummy');
/*$eventsManager->detachAll('dummy');

$component->leAction();
$component->leAction();
Expand Down Expand Up @@ -218,4 +223,142 @@ public function testEventsWeakref()

$this->assertEquals($data, "show first listener\n");
}

/**
* "Attaching event listeners by event name fails if preceded by
* detachment of all listeners for that type."
*
* Test contains 4 steps:
* - assigning event manager to dummy service with single log event
* listener attached
* - attaching second log listener
* - detaching all log listeners
* - attaching different listener
*
* @see https://github.com/phalcon/cphalcon/issues/1331
*/
public function testBug1331()
{
$di = new Phalcon\Di;
$di->set('componentX', function() use ($di) {
$component = new LeDummyComponent();
$eventsManager = new Phalcon\Events\Manager;
$eventsManager->attach('log', $di->get('MyFirstWeakrefListener'));
$component->setEventsManager($eventsManager);
return $component;
});

$di->set('firstListener', 'MyFirstWeakrefListener');
$di->set('secondListener', 'MySecondWeakrefListener');

// ----- TESTING STEP 1 - SIGNLE 'LOG' LISTENER ATTACHED

$component = $di->get('componentX');

$logListeners = $component->getEventsManager()->getListeners('log');

$this->assertInstanceOf('MyFirstWeakrefListener', $logListeners[0]);
$this->assertCount(1, $logListeners);

// ----- TESTING STEP 2 - SECOND 'LOG' LISTENER ATTACHED

$component->getEventsManager()->attach('log', $di->get('MySecondWeakrefListener'));

$logListeners = $component->getEventsManager()->getListeners('log');

$this->assertCount(2, $logListeners);
$firstLister = array_shift($logListeners);
$secondLister = array_shift($logListeners);
$this->assertInstanceOf('MyFirstWeakrefListener', $firstLister);
$this->assertInstanceOf('MySecondWeakrefListener', $secondLister);

// ----- TESTING STEP 3 - ALL 'LOG' LISTENER DETACHED

$component->getEventsManager()->detachAll('log');

$logListeners = $component->getEventsManager()->getListeners('log');
$this->assertEmpty($logListeners);

// ----- TESTING STEP 4 - SINGLE 'LOG' LISTENER ATTACHED SECOND TIME

$component->getEventsManager()->attach('log', $di->get('MySecondWeakrefListener'));

$logListeners = $component->getEventsManager()->getListeners('log');

$this->assertInstanceOf('MySecondWeakrefListener', $logListeners[0]);
$this->assertCount(1, $logListeners);
}

/**
* "Attaching event listeners by event name fails if preceded by
* detachment of all listeners for that type."
*
* Test contains 4 steps:
* - assigning event manager to dummy service with single log event
* listener attached
* - attaching second log listener
* - detaching all log listeners
* - attaching different listener
*
* NOTE: This test looks the same as above but it checks dettachAll()
* instead of detachAll() method. To be DELETED when dettachAll()
* will not supported any more.
*
* @see https://github.com/phalcon/cphalcon/issues/1331
*/
public function testBug1331BackwardCompatibility()
{
$di = new Phalcon\Di;
$di->set('componentX', function() use ($di) {
$component = new LeDummyComponent();
$eventsManager = new Phalcon\Events\Manager;
$eventsManager->attach('log', $di->get('MyFirstWeakrefListener'));
$component->setEventsManager($eventsManager);
return $component;
});

$di->set('firstListener', 'MyFirstWeakrefListener');
$di->set('secondListener', 'MySecondWeakrefListener');

// ----- TESTING STEP 1 - SIGNLE 'LOG' LISTENER ATTACHED

$component = $di->get('componentX');

$logListeners = $component->getEventsManager()->getListeners('log');

$this->assertInstanceOf('MyFirstWeakrefListener', $logListeners[0]);
$this->assertCount(1, $logListeners);

// ----- TESTING STEP 2 - SECOND 'LOG' LISTENER ATTACHED

$component->getEventsManager()->attach('log', $di->get('MySecondWeakrefListener'));

$logListeners = $component->getEventsManager()->getListeners('log');

$this->assertCount(2, $logListeners);
$firstLister = array_shift($logListeners);
$secondLister = array_shift($logListeners);
$this->assertInstanceOf('MyFirstWeakrefListener', $firstLister);
$this->assertInstanceOf('MySecondWeakrefListener', $secondLister);

// ----- TESTING STEP 3 - ALL 'LOG' LISTENER DETACHED

$oldErrorLevel = error_reporting(E_ALL & ~E_DEPRECATED);

@$component->getEventsManager()->dettachAll('log');

error_reporting($oldErrorLevel);

$logListeners = $component->getEventsManager()->getListeners('log');
$this->assertEmpty($logListeners);

// ----- TESTING STEP 4 - SINGLE 'LOG' LISTENER ATTACHED SECOND TIME

$component->getEventsManager()->attach('log', $di->get('MySecondWeakrefListener'));

$logListeners = $component->getEventsManager()->getListeners('log');

$this->assertInstanceOf('MySecondWeakrefListener', $logListeners[0]);
$this->assertCount(1, $logListeners);
}
}