Issues

ZF-1142: Allow reporting of an error when mispelled view variables are used.

Description

The two patches are mutually exclusive and present two different solutions to the same issue.

Currently, views automatically disable all warnings when using unset variables. This results in typos not producing warnings. However, many developers do not wish to see warnings when unset variables are used. The patch below provides a convenient compromise, supporting both checking for typos and ignoring unset variables.


Index: Abstract.php
===================================================================
--- Abstract.php    (revision 4225)
+++ Abstract.php    (working copy)
@@ -98,6 +98,14 @@
     private $_encoding = 'ISO-8859-1';
 
     /**
+     * Array of view variable names that should not result in an E_NOTICE
+     * warning, if used and not set.
+     * @var array
+     */
+    private $_declaredVars;
+
+    /**
+    /**
      * Constructor.
      *
      * @param array $config Configuration key-value pairs.
@@ -153,14 +161,24 @@
     }
 
     /**
-     * Prevent E_NOTICE for nonexistent values
+     * Prevent E_NOTICE for nonexistent values, unless declareVars()
+     * was used to provide a list of value names to prevent E_NOTICE.
+     * Using declareVars() yields a compromise solution enabling
+     * detection of spelling errors and typos in views, without 
+     * suffering from numerous E_NOTICE: "Notice: Undefined variable:".
      * 
      * @param string $key 
      * @return null
      */
     public function __get($key)
     {
-        return null;
+        if (!isset($this->_declaredVars)) {
+            return null;
+        }
+        if (isset($this->_declaredVars[$key])) {
+            return null;
+        }
+        trigger_error("Key '$key' does not exist.");
     }
 
     /**
@@ -230,6 +248,9 @@
             // load class and create instance
             $class = $this->_loadClass('helper', $name);
             $this->_helper[$name] = new $class();
+            if (property_exists($class, 'view')) {
+                $this->_helper[$name]->view = $this;
+            }
         }
 
         // call the helper method
@@ -770,4 +791,20 @@
      * @return mixed
      */
     abstract protected function _run();
+
+    /**
+     * Set any unset vars to avoid E_STRICT notices when the view uses them.
+     *
+     * When a view uses an undefined variable with E_STRICT enabled, use this
+     * method to avoid warnings without disabling E_STRICT for other "variables",
+     * such as those containing spelling errors and typos.
+     * Thus, typos continue to result in warnings, but correctly spelled variables will not.
+     * 
+     * @param string  variable number of arguments, all string names of variables to test
+     * @return void
+     */
+    public function declareVars()
+    {
+        $this->_declaredVars = array_flip(func_get_args());
+    }
 }

Comments

Given a choice, I prefer the alternative patch that relies on the View helper "DeclareVars".

Assigning to [~matthew].

While I see the view helper patch as elegant, I feel that the view object integration makes more sense from a usage point of view -- it's simpler, and I feel most developers will find it natural to call declareVars() than to utilize a helper.

I will implement this prior to the 0.9.2 release.

I wrote the DeclareVars view helper after the first patch above to address some shortcomings with the functionality provided by the view object patch. I won't have time this week to revisit the view object patch and try to upgrade it to support the same level of functionality in the DeclareVars view helper.

I see __get() in Abstract.php more as a problem and less as a feature. A developer can not easily remove __get() to implement their own alternatives (e.g. DeclareVars). With the DeclareVars view helper alternative, the developer is free to drop in a replacement into their view helper script path. With the current Abstract.php (i.e. __get()), the DeclareVars view helper won't work.

Perhaps we can make both alternatives possible with a little refactoring.

I applied your patch (minus the "->view = $this" segment) in my local tree. My issue is that one of the touted benefits of Zend_View is that you can access variables that have not been set, without raising notices. This patch, because it removes __get(), causes a ton of E_NOTICE messages to be raised. Additionally, it requires the developer to use the declareVars() helper if they want to avoid them.

My thought is to have an accessor by which one may change a 'strictVars' flag; if set, __get() would trigger a notice; otherwise, the normal behaviour would be observed (i.e., null returned and no notice).

This provides an easy way for a developer to modify the settings pre- and post-production (turn on strictVars() during development, turn it off in production). I will still add the declareVars helper, because when used with strictVars(), it could provide exactly the behaviour you suggest in your patch -- it just wouldn't be the default behaviour.

If more advanced functionality is desired, such as logging when undefined variables are accessed, or wanting to set strictVars() on by default, the individual developer could extend Zend_View or Zend_View_Abstract.

Yes, perfect! I like this blend of optional and default behaviors :)

Added the following functionality in r4332: * Setting $view->strictVars(true); will cause __get() to raise E_USER_NOTICEs when variables have not been previously declared * Added the DeclareVars helper. This helper will set any variables passed to it in the view object, with default values if the variable does not already exist. Used in conjunction with strictVars(), it helps suppress notices for valid, optional template variables.