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

[Cinderella-Man]

Bug fix for: #1331

• change of dettach to detach

[phalcon]

Thanks Kamil, we're aware of the typo in the method, however I think that we could not just rename the method because that would break existing applications, can we keep the method and mark it as deprecated?

[Cinderella-Man]

No worries. Just to make myself sure... You would like to have both methods(dettachAll() and detachAll()) to retain backward compatibility? That makes sense for me as well, but what about interface? It cannot have both.

[phalcon]

I think the right way to go is keep both methods, we could remove the wrong one in 2.0, regarding the interface I think we must keep the wrong one because removing it would break a user component based on the interfaces.

[Cinderella-Man]

I totally agree with you - I deleted it to quickly, now I reverted back interface to contain dettachAll() method and method exists in events/manager class(backward compatibility), it's flagged as deprecated, I also added my fix for detaching and created detachAll() method(copy of dettachAll())

[ghost]

In order not to duplicate code, you can use PHP_MALIAS macro.

See ext/annotations/annotation.h for the usage example.

[ghost]

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.

[ghost]

When trying to run the test, I get this error:

Starting test 'EventsTest::testBug1331'.
PHP Fatal error:  Call to undefined method EventsTest::assertContainsOnlyInstancesOf() in /home/vladimir/workspace/cphalcon/unit-tests/EventsTest.php on line 266

PHPUnit 3.6.10 from Ubuntu 13.04

[ghost]

I offer this patch:

diff --git a/unit-tests/EventsTest.php b/unit-tests/EventsTest.php
index d66dc2c..bd9aeac 100644
--- a/unit-tests/EventsTest.php
+++ b/unit-tests/EventsTest.php
@@ -239,12 +239,6 @@ class EventsTest extends PHPUnit_Framework_TestCase
         */
        public function testBug1331()
        {
-               if (!class_exists('MyFirstWeakrefListener')
-                   || !class_exists('MySecondWeakrefListener')
-               ) {
-                       return;
-               }
-
                $di = new Phalcon\Di;
                $di->set('componentX', function() use ($di) {
                        $component = new LeDummyComponent();
@@ -263,8 +257,8 @@ class EventsTest extends PHPUnit_Framework_TestCase

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

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

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

@@ -291,8 +285,8 @@ class EventsTest extends PHPUnit_Framework_TestCase

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

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

        /**
@@ -314,12 +308,6 @@ class EventsTest extends PHPUnit_Framework_TestCase
         */
        public function testBug1331BackwardCompatibility()
        {
-               if (!class_exists('MyFirstWeakrefListener')
-                   || !class_exists('MySecondWeakrefListener')
-               ) {
-                       return;
-               }
-
                $di = new Phalcon\Di;
                $di->set('componentX', function() use ($di) {
                        $component = new LeDummyComponent();
@@ -338,8 +326,8 @@ class EventsTest extends PHPUnit_Framework_TestCase

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

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

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

@@ -366,7 +354,7 @@ class EventsTest extends PHPUnit_Framework_TestCase

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

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

This will work for those who don't have the latest PHPUnit installed.

[Cinderella-Man]

Thanks for your comments, usage example and tips on how to upgrade my fix. I hope that I changed all files in the correct way as you advised.

[ghost]

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

[ghost]

Looks good, thank you!