ZF-11066: Inconsistent use of __get() in ControllerTestCase

Description

It is hard to read parts of this class because some parts of it use the {{__get()}} method to access members, and some sections use accessor methods. I suggest that Zend Framework avoid using {{__get()}} internally of a class since at first glance, {{$this->frontController}} appears to be a typo for {{$this->_frontController}} to the novice.

Comments

Index: library/Zend/Test/PHPUnit/ControllerTestCase.php
===================================================================
--- library/Zend/Test/PHPUnit/ControllerTestCase.php    (revision 23692)
+++ library/Zend/Test/PHPUnit/ControllerTestCase.php    (working copy)
@@ -160,7 +160,7 @@
                 }
             }
         }
-        $this->frontController
+        $this->getFrontController()
              ->setRequest($this->getRequest())
              ->setResponse($this->getResponse());
     }
@@ -193,7 +193,7 @@
         $request->setPathInfo(null);
 
         $controller = $this->getFrontController();
-        $this->frontController
+        $this->getFrontController()
              ->setRequest($request)
              ->setResponse($this->getResponse())
              ->throwExceptions(false)
@@ -202,7 +202,7 @@
         if ($this->bootstrap instanceof Zend_Application) {
             $this->bootstrap->run();
         } else {
-            $this->frontController->dispatch();
+            $this->getFrontController()->dispatch();
         }
     }
 
@@ -225,7 +225,7 @@
         $this->resetResponse();
         Zend_Layout::resetMvcInstance();
         Zend_Controller_Action_HelperBroker::resetHelpers();
-        $this->frontController->resetInstance();
+        $this->getFrontController()->resetInstance();
         Zend_Session::$_unitTestEnabled = true;
     }
 
@@ -292,7 +292,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__)) {
             $constraint->fail($path, $message);
         }
@@ -310,7 +310,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__)) {
             $constraint->fail($path, $message);
         }
@@ -329,7 +329,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__, $match)) {
             $constraint->fail($path, $message);
         }
@@ -348,7 +348,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__, $match)) {
             $constraint->fail($path, $message);
         }
@@ -367,7 +367,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__, $pattern)) {
             $constraint->fail($path, $message);
         }
@@ -386,7 +386,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__, $pattern)) {
             $constraint->fail($path, $message);
         }
@@ -405,7 +405,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__, $count)) {
             $constraint->fail($path, $message);
         }
@@ -424,7 +424,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__, $count)) {
             $constraint->fail($path, $message);
         }
@@ -443,7 +443,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__, $count)) {
             $constraint->fail($path, $message);
         }
@@ -462,7 +462,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__, $count)) {
             $constraint->fail($path, $message);
         }
@@ -492,7 +492,7 @@
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
         $constraint->registerXpathNamespaces($this->_xpathNamespaces);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__)) {
             $constraint->fail($path, $message);
         }
@@ -511,7 +511,7 @@
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
         $constraint->registerXpathNamespaces($this->_xpathNamespaces);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__)) {
             $constraint->fail($path, $message);
         }
@@ -531,7 +531,7 @@
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
         $constraint->registerXpathNamespaces($this->_xpathNamespaces);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__, $match)) {
             $constraint->fail($path, $message);
         }
@@ -551,7 +551,7 @@
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
         $constraint->registerXpathNamespaces($this->_xpathNamespaces);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__, $match)) {
             $constraint->fail($path, $message);
         }
@@ -571,7 +571,7 @@
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
         $constraint->registerXpathNamespaces($this->_xpathNamespaces);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__, $pattern)) {
             $constraint->fail($path, $message);
         }
@@ -591,7 +591,7 @@
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
         $constraint->registerXpathNamespaces($this->_xpathNamespaces);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__, $pattern)) {
             $constraint->fail($path, $message);
         }
@@ -611,7 +611,7 @@
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
         $constraint->registerXpathNamespaces($this->_xpathNamespaces);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__, $count)) {
             $constraint->fail($path, $message);
         }
@@ -631,7 +631,7 @@
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
         $constraint->registerXpathNamespaces($this->_xpathNamespaces);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__, $count)) {
             $constraint->fail($path, $message);
         }
@@ -651,7 +651,7 @@
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
         $constraint->registerXpathNamespaces($this->_xpathNamespaces);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__, $count)) {
             $constraint->fail($path, $message);
         }
@@ -671,7 +671,7 @@
         require_once 'Zend/Test/PHPUnit/Constraint/DomQuery.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_DomQuery($path);
         $constraint->registerXpathNamespaces($this->_xpathNamespaces);
-        $content    = $this->response->outputBody();
+        $content    = $this->getResponse()->outputBody();
         if (!$constraint->evaluate($content, __FUNCTION__, $count)) {
             $constraint->fail($path, $message);
         }
@@ -688,7 +688,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/Redirect.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_Redirect();
-        $response   = $this->response;
+        $response   = $this->getResponse();
         if (!$constraint->evaluate($response, __FUNCTION__)) {
             $constraint->fail($response, $message);
         }
@@ -705,7 +705,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/Redirect.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_Redirect();
-        $response   = $this->response;
+        $response   = $this->getResponse();
         if (!$constraint->evaluate($response, __FUNCTION__)) {
             $constraint->fail($response, $message);
         }
@@ -723,7 +723,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/Redirect.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_Redirect();
-        $response   = $this->response;
+        $response   = $this->getResponse();
         if (!$constraint->evaluate($response, __FUNCTION__, $url)) {
             $constraint->fail($response, $message);
         }
@@ -741,7 +741,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/Redirect.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_Redirect();
-        $response   = $this->response;
+        $response   = $this->getResponse();
         if (!$constraint->evaluate($response, __FUNCTION__, $url)) {
             $constraint->fail($response, $message);
         }
@@ -759,7 +759,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/Redirect.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_Redirect();
-        $response   = $this->response;
+        $response   = $this->getResponse();
         if (!$constraint->evaluate($response, __FUNCTION__, $pattern)) {
             $constraint->fail($response, $message);
         }
@@ -777,7 +777,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/Redirect.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_Redirect();
-        $response   = $this->response;
+        $response   = $this->getResponse();
         if (!$constraint->evaluate($response, __FUNCTION__, $pattern)) {
             $constraint->fail($response, $message);
         }
@@ -795,7 +795,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/ResponseHeader.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_ResponseHeader();
-        $response   = $this->response;
+        $response   = $this->getResponse();
         if (!$constraint->evaluate($response, __FUNCTION__, $code)) {
             $constraint->fail($response, $message);
         }
@@ -814,7 +814,7 @@
         require_once 'Zend/Test/PHPUnit/Constraint/ResponseHeader.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_ResponseHeader();
         $constraint->setNegate(true);
-        $response   = $this->response;
+        $response   = $this->getResponse();
         if (!$constraint->evaluate($response, __FUNCTION__, $code)) {
             $constraint->fail($response, $message);
         }
@@ -832,7 +832,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/ResponseHeader.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_ResponseHeader();
-        $response   = $this->response;
+        $response   = $this->getResponse();
         if (!$constraint->evaluate($response, __FUNCTION__, $header)) {
             $constraint->fail($response, $message);
         }
@@ -851,7 +851,7 @@
         require_once 'Zend/Test/PHPUnit/Constraint/ResponseHeader.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_ResponseHeader();
         $constraint->setNegate(true);
-        $response   = $this->response;
+        $response   = $this->getResponse();
         if (!$constraint->evaluate($response, __FUNCTION__, $header)) {
             $constraint->fail($response, $message);
         }
@@ -870,7 +870,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/ResponseHeader.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_ResponseHeader();
-        $response   = $this->response;
+        $response   = $this->getResponse();
         if (!$constraint->evaluate($response, __FUNCTION__, $header, $match)) {
             $constraint->fail($response, $message);
         }
@@ -890,7 +890,7 @@
         require_once 'Zend/Test/PHPUnit/Constraint/ResponseHeader.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_ResponseHeader();
         $constraint->setNegate(true);
-        $response   = $this->response;
+        $response   = $this->getResponse();
         if (!$constraint->evaluate($response, __FUNCTION__, $header, $match)) {
             $constraint->fail($response, $message);
         }
@@ -909,7 +909,7 @@
         $this->_incrementAssertionCount();
         require_once 'Zend/Test/PHPUnit/Constraint/ResponseHeader.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_ResponseHeader();
-        $response   = $this->response;
+        $response   = $this->getResponse();
         if (!$constraint->evaluate($response, __FUNCTION__, $header, $pattern)) {
             $constraint->fail($response, $message);
         }
@@ -929,7 +929,7 @@
         require_once 'Zend/Test/PHPUnit/Constraint/ResponseHeader.php';
         $constraint = new Zend_Test_PHPUnit_Constraint_ResponseHeader();
         $constraint->setNegate(true);
-        $response   = $this->response;
+        $response   = $this->getResponse();
         if (!$constraint->evaluate($response, __FUNCTION__, $header, $pattern)) {
             $constraint->fail($response, $message);
         }
@@ -945,9 +945,9 @@
     public function assertModule($module, $message = '')
     {
         $this->_incrementAssertionCount();
-        if ($module != $this->request->getModuleName()) {
+        if ($module != $this->getRequest()->getModuleName()) {
             $msg = sprintf('Failed asserting last module used <"%s"> was "%s"',
-                $this->request->getModuleName(),
+                $this->getRequest()->getModuleName(),
                 $module
             );
             if (!empty($message)) {
@@ -967,7 +967,7 @@
     public function assertNotModule($module, $message = '')
     {
         $this->_incrementAssertionCount();
-        if ($module == $this->request->getModuleName()) {
+        if ($module == $this->getRequest()->getModuleName()) {
             $msg = sprintf('Failed asserting last module used was NOT "%s"', $module);
             if (!empty($message)) {
                 $msg = $message . "\n" . $msg;
@@ -986,9 +986,9 @@
     public function assertController($controller, $message = '')
     {
         $this->_incrementAssertionCount();
-        if ($controller != $this->request->getControllerName()) {
+        if ($controller != $this->getRequest()->getControllerName()) {
             $msg = sprintf('Failed asserting last controller used <"%s"> was "%s"',
-                $this->request->getControllerName(),
+                $this->getRequest()->getControllerName(),
                 $controller
             );
             if (!empty($message)) {
@@ -1008,9 +1008,9 @@
     public function assertNotController($controller, $message = '')
     {
         $this->_incrementAssertionCount();
-        if ($controller == $this->request->getControllerName()) {
+        if ($controller == $this->getRequest()->getControllerName()) {
             $msg = sprintf('Failed asserting last controller used <"%s"> was NOT "%s"',
-                $this->request->getControllerName(),
+                $this->getRequest()->getControllerName(),
                 $controller
             );
             if (!empty($message)) {
@@ -1030,8 +1030,8 @@
     public function assertAction($action, $message = '')
     {
         $this->_incrementAssertionCount();
-        if ($action != $this->request->getActionName()) {
-            $msg = sprintf('Failed asserting last action used <"%s"> was "%s"', $this->request->getActionName(), $action);
+        if ($action != $this->getRequest()->getActionName()) {
+            $msg = sprintf('Failed asserting last action used <"%s"> was "%s"', $this->getRequest()->getActionName(), $action);
             if (!empty($message)) {
                 $msg = $message . "\n" . $msg;
             }
@@ -1049,8 +1049,8 @@
     public function assertNotAction($action, $message = '')
     {
         $this->_incrementAssertionCount();
-        if ($action == $this->request->getActionName()) {
-            $msg = sprintf('Failed asserting last action used <"%s"> was NOT "%s"', $this->request->getActionName(), $action);
+        if ($action == $this->getRequest()->getActionName()) {
+            $msg = sprintf('Failed asserting last action used <"%s"> was NOT "%s"', $this->getRequest()->getActionName(), $action);
             if (!empty($message)) {
                 $msg = $message . "\n" . $msg;
             }
@@ -1068,7 +1068,7 @@
     public function assertRoute($route, $message = '')
     {
         $this->_incrementAssertionCount();
-        $router = $this->frontController->getRouter();
+        $router = $this->getFrontController()->getRouter();
         if ($route != $router->getCurrentRouteName()) {
             $msg = sprintf('Failed asserting matched route was "%s", actual route is %s',
                 $route,
@@ -1091,7 +1091,7 @@
     public function assertNotRoute($route, $message = '')
     {
         $this->_incrementAssertionCount();
-        $router = $this->frontController->getRouter();
+        $router = $this->getFrontController()->getRouter();
         if ($route == $router->getCurrentRouteName()) {
             $msg = sprintf('Failed asserting route matched was NOT "%s"', $route);
             if (!empty($message)) {

I have wondered this too so the fix gets my vote.

Changing this in ZF1 is a no-go as it would create a backwards compatibility break. In ZF2, from the prototypes I've been doing, we may not even need ControllerTestCase anymore. As such, I'm going to close this as "won't fix".

Huh? The patch shouldn't break anything externally since it is an internal change with no semantic difference.

Sorry -- I was looking more at the "do not use __get()" as a request to remove it entirely.

I'll look into the patch this week.

No worries.

I ran the tests for this particular file and they succeeded. I hadn't done so before.

{{monospaced}} OK (42 tests, 77 assertions) {{monospaced}}

I have applied the suggested patch and the {{Zend_Test}} test suite still passes. Attached a patch file which should apply cleanly against trunk.