merged branch char101/nativeext-exception-info (PR #885)

This PR was merged into the master branch. Commits ------- db13b66 Pass lineno and filename to Twig_Error constructor feee667 Merge branch 'nativeext-exception-info' of https://github.com/fabpot/Twig into nativeext-exception-info adb31d4 Call Twig_Error constructor d4a8c8b added tests for exceptions thrown in Twig_Template::getAttribute() 1b82bf7 Add template filename for the rest of the exception 5675140 Handle NULL filename 71f64bc Add template name to error message Discussion ---------- Add template name to error message Issue #884 --------------------------------------------------------------------------- by fabpot at 2012-11-06T07:43:16Z For some unknown reasons, I don't see your branch when I try to submit a PR. Can you cherry-pick my unit tests from my `nativeext-exception-info` branch? --------------------------------------------------------------------------- by Tobion at 2012-11-06T08:29:19Z @char101 Maybe you could also integrate the fixes of #878? --------------------------------------------------------------------------- by fabpot at 2012-11-06T08:30:55Z @Tobion Let's do one thing at a time. --------------------------------------------------------------------------- by char101 at 2012-11-06T08:59:30Z @Tobion AFAIK, unlike the PHP implementation, the C implementation does not automatically convert array key type. --------------------------------------------------------------------------- by char101 at 2012-11-06T09:27:54Z @fabpot How about the lineno variable? Should I just set it to 0 or is there any function that can be called to obtain it? ```php if (-1 === $this->lineno || null === $this->filename) { $this->guessTemplateInfo(); } ``` --------------------------------------------------------------------------- by fabpot at 2012-11-06T09:32:09Z You need to keep `-1` for the line. --------------------------------------------------------------------------- by char101 at 2012-11-06T09:36:56Z If `$lineno` is `-1`, wouldn't `$this->guessTemplateInfo()` still be called? --------------------------------------------------------------------------- by fabpot at 2012-11-06T09:45:34Z Yes, but the template name will not be guessed. So, there is a performance overhead for the line guessing, but not for the template name guessing.
2026-09-13 19:06:40 +00:00 · 2012-11-06 12:54:36 +01:00
parent abec35b833 db13b66774
commit cfb341f4ba
3 changed files with 158 additions and 60 deletions
@@ -29,6 +29,10 @@
 #define Z_ADDREF_P(pz)                (pz)->refcount++
 #endif

+#define FREE_DTOR(z) 	\
+	zval_dtor(z); 		\
+	efree(z);
+
 #if PHP_VERSION_ID >= 50300
 	#define APPLY_TSRMLS_DC TSRMLS_DC
 	#define APPLY_TSRMLS_CC TSRMLS_CC
@@ -196,16 +200,14 @@ zval *TWIG_CALL_USER_FUNC_ARRAY(zval *object, char *function, zval *arguments TS
 	fci.no_separation = 0;

 	if (zend_call_function(&fci, NULL TSRMLS_CC) == FAILURE) {
-		zval_dtor(zfunction);
-		efree(zfunction);
+		FREE_DTOR(zfunction)
 		zend_throw_exception_ex(zend_exception_get_default(TSRMLS_C), 0 TSRMLS_CC, "Could not execute %s::%s()", zend_get_class_entry(object TSRMLS_CC)->name, function TSRMLS_CC);
 	}

 	if (args) {
 		efree(fci.params);
 	}
-	zval_dtor(zfunction);
-	efree(zfunction);
+	FREE_DTOR(zfunction);
 	return retval_ptr;
 }

@@ -275,8 +277,7 @@ zval *TWIG_GET_ARRAY_ELEMENT(zval *class, char *prop_name, int prop_name_length
 		ALLOC_INIT_ZVAL(tmp_name_zval);
 		ZVAL_STRING(tmp_name_zval, prop_name, 1);
 		tmp_ret_zval = TWIG_GET_ARRAYOBJECT_ELEMENT(class, tmp_name_zval TSRMLS_CC);
-		zval_dtor(tmp_name_zval);
-		efree(tmp_name_zval);
+		FREE_DTOR(tmp_name_zval);
 		return tmp_ret_zval;
 	}

@@ -337,8 +338,7 @@ zval *TWIG_PROPERTY_CHAR(zval *object, char *propname TSRMLS_DC)
 	ALLOC_INIT_ZVAL(tmp_name_zval);
 	ZVAL_STRING(tmp_name_zval, propname, 1);
 	tmp = TWIG_PROPERTY(object, tmp_name_zval TSRMLS_CC);
-	zval_dtor(tmp_name_zval);
-	efree(tmp_name_zval);
+	FREE_DTOR(tmp_name_zval);
 	return tmp;
 }

@@ -376,13 +376,12 @@ zval *TWIG_CALL_S(zval *object, char *method, char *arg0 TSRMLS_DC)
 	fci.no_separation = 0;

 	if (zend_call_function(&fci, NULL TSRMLS_CC) == FAILURE) {
-		zval_dtor(argument);
+		FREE_DTOR(zfunction);
+		zval_ptr_dtor(&argument);
 		return 0;
 	}
-	zval_dtor(zfunction);
-	efree(zfunction);
-	zval_dtor(argument);
-	efree(argument);
+	FREE_DTOR(zfunction);
+	zval_ptr_dtor(&argument);
 	return retval_ptr;
 }

@@ -428,16 +427,14 @@ int TWIG_CALL_Z(zval *object, char *method, zval *arg1 TSRMLS_DC)
 	fci.no_separation = 0;

 	if (zend_call_function(&fci, NULL TSRMLS_CC) == FAILURE) {
-		zval_dtor(zfunction);
-		efree(zfunction);
+		FREE_DTOR(zfunction);
 		if (retval_ptr) {
 			zval_ptr_dtor(&retval_ptr);
 		}
 		return 0;
 	}

-	zval_dtor(zfunction);
-	efree(zfunction);
+	FREE_DTOR(zfunction);

 	success = (retval_ptr && (Z_TYPE_P(retval_ptr) == IS_BOOL) && Z_LVAL_P(retval_ptr));
 	if (retval_ptr) {
@@ -475,11 +472,11 @@ int TWIG_CALL_ZZ(zval *object, char *method, zval *arg1, zval *arg2 TSRMLS_DC)
 	fci.no_separation = 0;

 	if (zend_call_function(&fci, NULL TSRMLS_CC) == FAILURE) {
-		zval_dtor(zfunction);
+		FREE_DTOR(zfunction);
 		return 0;
 	}

-	zval_dtor(zfunction);
+	FREE_DTOR(zfunction);

 	success = (retval_ptr && (Z_TYPE_P(retval_ptr) == IS_BOOL) && Z_LVAL_P(retval_ptr));
 	if (retval_ptr) {
@@ -555,8 +552,7 @@ static void TWIG_THROW_EXCEPTION(char *exception_name TSRMLS_DC, char *message,
 	va_list args;
 	zend_class_entry **pce;

-	if (zend_lookup_class(exception_name, strlen(exception_name), &pce TSRMLS_CC) == FAILURE)
-	{
+	if (zend_lookup_class(exception_name, strlen(exception_name), &pce TSRMLS_CC) == FAILURE) {
 		return;
 	}

@@ -565,9 +561,67 @@ static void TWIG_THROW_EXCEPTION(char *exception_name TSRMLS_DC, char *message,
 	va_end(args);

 	zend_throw_exception_ex(*pce, 0 TSRMLS_CC, buffer);
+	efree(buffer);
 }

-char *TWIG_GET_CLASS_NAME(zval *object TSRMLS_DC)
+static void TWIG_RUNTIME_ERROR(zval *template TSRMLS_DC, char *message, ...)
+{
+	char *buffer;
+	va_list args;
+	zend_class_entry **pce;
+	zval *ex;
+	zval *constructor;
+	zval *zmessage;
+	zval *lineno;
+	zval *filename_func;
+	zval *filename;
+	zval *constructor_args[3];
+	zval *constructor_retval;
+
+	if (zend_lookup_class("Twig_Error_Runtime", strlen("Twig_Error_Runtime"), &pce TSRMLS_CC) == FAILURE) {
+		return;
+	}
+
+	va_start(args, message);
+	vspprintf(&buffer, 0, message, args);
+	va_end(args);
+
+	MAKE_STD_ZVAL(ex);
+	object_init_ex(ex, *pce);
+
+	// Call Twig_Error constructor
+	MAKE_STD_ZVAL(constructor);
+	MAKE_STD_ZVAL(zmessage);
+	MAKE_STD_ZVAL(lineno);
+	MAKE_STD_ZVAL(filename);
+	MAKE_STD_ZVAL(filename_func);
+	MAKE_STD_ZVAL(constructor_retval);
+
+	ZVAL_STRINGL(constructor, "__construct", sizeof("__construct")-1, 1);
+	ZVAL_STRING(zmessage, buffer, 1);
+	ZVAL_LONG(lineno, -1);
+
+	// Get template filename
+	ZVAL_STRINGL(filename_func, "getTemplateName", sizeof("getTemplateName")-1, 1);
+	call_user_function(EG(function_table), &template, filename_func, filename, 0, 0 TSRMLS_CC);
+
+	constructor_args[0] = zmessage;
+	constructor_args[1] = lineno;
+	constructor_args[2] = filename;
+	call_user_function(EG(function_table), &ex, constructor, constructor_retval, 3, constructor_args TSRMLS_CC);
+
+	zval_ptr_dtor(&constructor_retval);
+	zval_ptr_dtor(&zmessage);
+	zval_ptr_dtor(&lineno);
+	zval_ptr_dtor(&filename);
+	FREE_DTOR(constructor);
+	FREE_DTOR(filename_func);
+	efree(buffer);
+
+	zend_throw_exception_object(ex TSRMLS_CC);
+}
+
+static char *TWIG_GET_CLASS_NAME(zval *object TSRMLS_DC)
 {
 	char *class_name;
 	zend_uint class_name_len;
@@ -736,18 +790,21 @@ PHP_FUNCTION(twig_template_get_attributes)
 			}
 /*
 			if (is_object($object)) {
-				throw new Twig_Error_Runtime(sprintf('Key "%s" in object (with ArrayAccess) of type "%s" does not exist', $item, get_class($object)));
-			// array
+				throw new Twig_Error_Runtime(sprintf('Key "%s" in object (with ArrayAccess) of type "%s" does not exist', $item, get_class($object)), -1, $this->getTemplateName());
+			} elseif (is_array($object)) {
+				throw new Twig_Error_Runtime(sprintf('Key "%s" for array with keys "%s" does not exist', $item, implode(', ', array_keys($object))), -1, $this->getTemplateName());
 			} else {
-				throw new Twig_Error_Runtime(sprintf('Key "%s" for array with keys "%s" does not exist', $item, implode(', ', array_keys($object))));
+				throw new Twig_Error_Runtime(sprintf('Impossible to access a key ("%s") on a "%s" variable', $item, gettype($object)), -1, $this->getTemplateName());
 			}
 		}
 	}
 */
 			if (Z_TYPE_P(object) == IS_OBJECT) {
-				TWIG_THROW_EXCEPTION("Twig_Error_Runtime" TSRMLS_CC, "Key \"%s\" in object (with ArrayAccess) of type \"%s\" does not exist", item, TWIG_GET_CLASS_NAME(object TSRMLS_CC));
+				TWIG_RUNTIME_ERROR(template TSRMLS_CC, "Key \"%s\" in object (with ArrayAccess) of type \"%s\" does not exist", item, TWIG_GET_CLASS_NAME(object TSRMLS_CC));
+			} else if (Z_TYPE_P(object) == IS_ARRAY) {
+				TWIG_RUNTIME_ERROR(template TSRMLS_CC, "Key \"%s\" for array with keys \"%s\" does not exist", item, TWIG_IMPLODE_ARRAY_KEYS(", ", object TSRMLS_CC));
 			} else {
-				TWIG_THROW_EXCEPTION("Twig_Error_Runtime" TSRMLS_CC, "Key \"%s\" for array with keys \"%s\" does not exist", item, TWIG_IMPLODE_ARRAY_KEYS(", ", object TSRMLS_CC));
+				TWIG_RUNTIME_ERROR(template TSRMLS_CC, "Impossible to access a key (\"%s\") on a \"%s\" variable", item, zend_zval_type_name(object));
 			}
 			return;
 		}
@@ -775,11 +832,11 @@ PHP_FUNCTION(twig_template_get_attributes)
 			RETURN_FALSE;
 		}
 		if (Z_TYPE_P(object) == IS_ARRAY) {
-			TWIG_THROW_EXCEPTION("Twig_Error_Runtime" TSRMLS_CC, "Item \"%s\" for \"Array\" does not exist", item);
+			TWIG_RUNTIME_ERROR(template TSRMLS_CC, "Item \"%s\" for \"Array\" does not exist", item);
 		} else {
 			Z_ADDREF_P(object);
 			convert_to_string_ex(&object);
-			TWIG_THROW_EXCEPTION("Twig_Error_Runtime" TSRMLS_CC, "Item \"%s\" for \"%s\" does not exist", item, Z_STRVAL_P(object));
+			TWIG_RUNTIME_ERROR(template TSRMLS_CC, "Item \"%s\" for \"%s\" does not exist", item, Z_STRVAL_P(object));
 			zval_ptr_dtor(&object);
 		}
 		return;
@@ -920,7 +977,7 @@ PHP_FUNCTION(twig_template_get_attributes)
 			if (ignoreStrictCheck || !TWIG_CALL_BOOLEAN(TWIG_PROPERTY_CHAR(template, "env" TSRMLS_CC), "isStrictVariables" TSRMLS_CC)) {
 				return;
 			}
-			TWIG_THROW_EXCEPTION("Twig_Error_Runtime" TSRMLS_CC, "Method \"%s\" for object \"%s\" does not exist", item, TWIG_GET_CLASS_NAME(object TSRMLS_CC));
+			TWIG_RUNTIME_ERROR(template TSRMLS_CC, "Method \"%s\" for object \"%s\" does not exist", item, TWIG_GET_CLASS_NAME(object TSRMLS_CC));
 			return;
 		}

@@ -13,12 +13,12 @@ class Twig_Node_Expression_GetAttr extends Twig_Node_Expression
 {
    public function __construct(Twig_Node_Expression $node, Twig_Node_Expression $attribute, Twig_Node_Expression_Array $arguments, $type, $lineno)
    {
-        parent::__construct(array('node' => $node, 'attribute' => $attribute, 'arguments' => $arguments), array('type' => $type, 'is_defined_test' => false, 'ignore_strict_check' => false), $lineno);
+        parent::__construct(array('node' => $node, 'attribute' => $attribute, 'arguments' => $arguments), array('type' => $type, 'is_defined_test' => false, 'ignore_strict_check' => false, 'disable_c_ext' => false), $lineno);
    }

    public function compile(Twig_Compiler $compiler)
    {
-        if (function_exists('twig_template_get_attributes')) {
+        if (function_exists('twig_template_get_attributes') && !$this->getAttribute('disable_c_ext')) {
            $compiler->raw('twig_template_get_attributes($this, ');
        } else {
            $compiler->raw('$this->getAttribute(');
@@ -11,17 +11,53 @@
 class Twig_Tests_TemplateTest extends PHPUnit_Framework_TestCase
 {
    /**
-     * @expectedException        Twig_Error_Runtime
-     * @expectedExceptionMessage Impossible to access a key ("a") on a "string" variable
+     * @dataProvider getAttributeExceptions
     */
-    public function testAttributeOnAString()
+    public function testGetAttributeExceptions($template, $message, $useExt)
    {
-        $template = new Twig_TemplateTest(
-            new Twig_Environment(null, array('strict_variables' => true)),
-            false
+        $name = 'index_'.($useExt ? 1 : 0);
+        $templates = array(
+            $name => $template.$useExt, // appending $useExt makes the template content unique
        );

-        $template->getAttribute('string', 'a', array(), Twig_TemplateInterface::ARRAY_CALL, false);
+        $env = new Twig_Environment(new Twig_Loader_Array($templates), array('strict_variables' => true));
+        if (!$useExt) {
+            $env->addNodeVisitor(new CExtDisablingNodeVisitor());
+        }
+        $template = $env->loadTemplate($name);
+
+        $context = array(
+            'string'       => 'foo',
+            'array'        => array('foo' => 'foo'),
+            'array_access' => new Twig_TemplateArrayAccessObject(),
+        );
+
+        try {
+            $template->render($context);
+        } catch (Twig_Error_Runtime $e) {
+            $this->assertEquals(sprintf($message, $name), $e->getMessage());
+        }
+    }
+
+    public function getAttributeExceptions()
+    {
+        $tests = array(
+            array('{{ string["a"] }}', 'Impossible to access a key ("a") on a "string" variable in "%s" at line 1', false),
+            array('{{ array["a"] }}', 'Key "a" for array with keys "foo" does not exist in "%s" at line 1', false),
+            array('{{ array_access["a"] }}', 'Key "a" in object (with ArrayAccess) of type "Twig_TemplateArrayAccessObject" does not exist in "%s" at line 1', false),
+            array('{{ string.a }}', 'Item "a" for "foo" does not exist in "%s" at line 1', false),
+            array('{{ array.a }}', 'Item "a" for "Array" does not exist in "%s" at line 1', false),
+            array('{{ array_access.a }}', 'Method "a" for object "Twig_TemplateArrayAccessObject" does not exist in "%s" at line 1', false),
+        );
+
+        if (function_exists('twig_template_get_attributes')) {
+            foreach (array_slice($tests, 0) as $test) {
+                $test[2] = true;
+                $tests[] = $test;
+            }
+        }
+
+        return $tests;
    }

    /**
@@ -29,10 +65,7 @@ class Twig_Tests_TemplateTest extends PHPUnit_Framework_TestCase
     */
    public function testGetAttribute($defined, $value, $object, $item, $arguments, $type, $useExt = false)
    {
-        $template = new Twig_TemplateTest(
-            new Twig_Environment(),
-            $useExt
-        );
+        $template = new Twig_TemplateTest(new Twig_Environment(), $useExt);

        $this->assertEquals($value, $template->getAttribute($object, $item, $arguments, $type));
    }
@@ -42,10 +75,7 @@ class Twig_Tests_TemplateTest extends PHPUnit_Framework_TestCase
     */
    public function testGetAttributeStrict($defined, $value, $object, $item, $arguments, $type, $useExt = false, $exceptionMessage = null)
    {
-        $template = new Twig_TemplateTest(
-            new Twig_Environment(null, array('strict_variables' => true)),
-            $useExt
-        );
+        $template = new Twig_TemplateTest(new Twig_Environment(null, array('strict_variables' => true)), $useExt);

        if ($defined) {
            $this->assertEquals($value, $template->getAttribute($object, $item, $arguments, $type));
@@ -67,10 +97,7 @@ class Twig_Tests_TemplateTest extends PHPUnit_Framework_TestCase
     */
    public function testGetAttributeDefined($defined, $value, $object, $item, $arguments, $type, $useExt = false)
    {
-        $template = new Twig_TemplateTest(
-            new Twig_Environment(),
-            $useExt
-        );
+        $template = new Twig_TemplateTest(new Twig_Environment(), $useExt);

        $this->assertEquals($defined, $template->getAttribute($object, $item, $arguments, $type, true));
    }
@@ -80,10 +107,7 @@ class Twig_Tests_TemplateTest extends PHPUnit_Framework_TestCase
     */
    public function testGetAttributeDefinedStrict($defined, $value, $object, $item, $arguments, $type, $useExt = false)
    {
-        $template = new Twig_TemplateTest(
-            new Twig_Environment(null, array('strict_variables' => true)),
-            $useExt
-        );
+        $template = new Twig_TemplateTest(new Twig_Environment(null, array('strict_variables' => true)), $useExt);

        $this->assertEquals($defined, $template->getAttribute($object, $item, $arguments, $type, true));
    }
@@ -205,11 +229,6 @@ class Twig_Tests_TemplateTest extends PHPUnit_Framework_TestCase

        return $tests;
    }
-
-    public function useExtGetAttribute()
-    {
-        return false;
-    }
 }

 class Twig_TemplateTest extends Twig_Template
@@ -404,3 +423,25 @@ class Twig_TemplateMagicMethodObject
        return '__call_'.$method;
    }
 }
+
+class CExtDisablingNodeVisitor implements Twig_NodeVisitorInterface
+{
+    public function enterNode(Twig_NodeInterface $node, Twig_Environment $env)
+    {
+        if ($node instanceof Twig_Node_Expression_GetAttr) {
+            $node->setAttribute('disable_c_ext', true);
+        }
+
+        return $node;
+    }
+
+    public function leaveNode(Twig_NodeInterface $node, Twig_Environment $env)
+    {
+        return $node;
+    }
+
+    public function getPriority()
+    {
+        return 0;
+    }
+}