Issues

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

Issue Type: Patch Created: 2011-02-11T08:17:39.000+0000 Last Updated: 2012-05-29T15:39:44.000+0000 Status: Reopened Fix version(s): Reporter: Aaron S. Hawley (ashawley) Assignee: Matthew Weier O'Phinney (matthew) Tags: - Zend_Controller

Related issues: Attachments: - ZF-11066.patch

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

Posted by Aaron S. Hawley (ashawley) on 2011-02-11T08:19:33.000+0000

<pre class="literal">
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)) {

Posted by Kim Blomqvist (kblomqvist) on 2011-04-24T19:27:45.000+0000

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

Posted by Matthew Weier O'Phinney (matthew) on 2011-04-25T13:42:13.000+0000

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".

Posted by Aaron S. Hawley (ashawley) on 2011-04-25T16:34:25.000+0000

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

Posted by Matthew Weier O'Phinney (matthew) on 2011-04-25T22:17:59.000+0000

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.

Posted by Aaron S. Hawley (ashawley) on 2011-05-02T16:35:06.000+0000

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

Posted by Adam Lundrigan (adamlundrigan) on 2012-05-29T15:37:07.000+0000

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

Have you found an issue?

See the Overview section for more details.

Copyright

© 2006-2016 by Zend, a Rogue Wave Company. Made with by awesome contributors.

This website is built using zend-expressive and it runs on PHP 7.

Contacts