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

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

Merged
merged 4 commits into from
Oct 6, 2013
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Bug 1331 - detachAll listeners from EventsManager
  • Loading branch information
Cinderella-Man committed Oct 5, 2013
commit 58489a89fe4f852c7138c5e4affb50cfa6678c47
36 changes: 35 additions & 1 deletion ext/events/manager.c
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,40 @@ PHP_METHOD(Phalcon_Events_Manager, getResponses){
RETURN_MEMBER(this_ptr, "_responses");
}

/**
* Removes all events from the EventsManager (deprecated)
*
* @param string $type
*
* @deprecated
*/
PHP_METHOD(Phalcon_Events_Manager, dettachAll){

zval *type = NULL, *events = NULL;

PHALCON_MM_GROW();

phalcon_fetch_params(1, 0, 1, &type);

if (!type) {
type = PHALCON_GLOBAL(z_null);
}

PHALCON_OBS_VAR(events);
phalcon_read_property_this(&events, this_ptr, SL("_events"), PH_NOISY_CC);
if (Z_TYPE_P(type) == IS_NULL) {
PHALCON_INIT_NVAR(events);
} else {
if (phalcon_array_isset(events, type)) {
phalcon_array_unset(&events, type, 0);
}
}

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

PHALCON_MM_RESTORE();
}

/**
* Removes all events from the EventsManager
*
Expand All @@ -252,7 +286,7 @@ PHP_METHOD(Phalcon_Events_Manager, detachAll){
phalcon_array_unset(&events, type, 0);
Copy link

Choose a reason for hiding this comment

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

I think we need PH_SEPARATE instead of 0 here

}
}

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

PHALCON_MM_RESTORE();
Expand Down
8 changes: 7 additions & 1 deletion ext/events/manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +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);
Expand All @@ -47,6 +48,10 @@ 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_ARG_INFO(0, type)
ZEND_END_ARG_INFO()

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,7 +83,8 @@ 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, detachAll, arginfo_phalcon_events_manager_detachall, 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)
Expand Down
2 changes: 1 addition & 1 deletion ext/events/managerinterface.c
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ PHALCON_DOC_METHOD(Phalcon_Events_ManagerInterface, attach);
*
* @param string $type
*/
PHALCON_DOC_METHOD(Phalcon_Events_ManagerInterface, detachAll);
PHALCON_DOC_METHOD(Phalcon_Events_ManagerInterface, dettachAll);

/**
* Fires a event in the events manager causing that the acive listeners will be notified about it
Expand Down
4 changes: 2 additions & 2 deletions ext/events/managerinterface.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ ZEND_BEGIN_ARG_INFO_EX(arginfo_phalcon_events_managerinterface_attach, 0, 0, 2)
ZEND_ARG_INFO(0, handler)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO_EX(arginfo_phalcon_events_managerinterface_detachall, 0, 0, 0)
ZEND_BEGIN_ARG_INFO_EX(arginfo_phalcon_events_managerinterface_dettachall, 0, 0, 0)
ZEND_ARG_INFO(0, type)
ZEND_END_ARG_INFO()

Expand All @@ -42,7 +42,7 @@ ZEND_END_ARG_INFO()

PHALCON_INIT_FUNCS(phalcon_events_managerinterface_method_entry){
PHP_ABSTRACT_ME(Phalcon_Events_ManagerInterface, attach, arginfo_phalcon_events_managerinterface_attach)
PHP_ABSTRACT_ME(Phalcon_Events_ManagerInterface, detachAll, arginfo_phalcon_events_managerinterface_detachall)
PHP_ABSTRACT_ME(Phalcon_Events_ManagerInterface, dettachAll, arginfo_phalcon_events_managerinterface_dettachall)
PHP_ABSTRACT_ME(Phalcon_Events_ManagerInterface, fire, arginfo_phalcon_events_managerinterface_fire)
PHP_ABSTRACT_ME(Phalcon_Events_ManagerInterface, getListeners, arginfo_phalcon_events_managerinterface_getlisteners)
PHP_FE_END
Expand Down
75 changes: 75 additions & 0 deletions unit-tests/EventsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -294,4 +294,79 @@ public function testBug1331()
$this->assertContainsOnlyInstancesOf('MySecondWeakrefListener', $logListeners);
$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()
{
if (!class_exists('MyFirstWeakrefListener')
|| !class_exists('MySecondWeakrefListener')
) {
return;
}

$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->assertContainsOnlyInstancesOf('MyFirstWeakrefListener', $logListeners);
$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()->dettachAll('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->assertContainsOnlyInstancesOf('MySecondWeakrefListener', $logListeners);
$this->assertCount(1, $logListeners);
}
}