From 3485ee77ea0fb14aa485cf7adcba9b59abb6849c Mon Sep 17 00:00:00 2001 From: Charles Date: Thu, 20 Sep 2012 18:29:21 +0700 Subject: [PATCH 1/6] Native extension: handle dynamic properties defined in the get_properties handler in a per instance fashion. --- ext/twig/twig.c | 30 +++++-------------------- test/Twig/Tests/NativeExtensionTest.php | 26 +++++++++++++++++++++ 2 files changed, 32 insertions(+), 24 deletions(-) create mode 100644 test/Twig/Tests/NativeExtensionTest.php diff --git a/ext/twig/twig.c b/ext/twig/twig.c index f8fc8592d..f464faf1c 100644 --- a/ext/twig/twig.c +++ b/ext/twig/twig.c @@ -612,26 +612,6 @@ static int twig_add_property_to_class(void *pDest APPLY_TSRMLS_DC, int num_args, return 0; } -/* {{{ _adddynproperty */ -static int twig_add_dyn_property_to_class(void *pDest APPLY_TSRMLS_DC, int num_args, va_list args, zend_hash_key *hash_key) -{ - APPLY_TSRMLS_FETCH(); - zend_class_entry *ce = *va_arg(args, zend_class_entry**); - zval *retval = va_arg(args, zval*), member; - char *class_name, *prop_name; - - if (hash_key->nKeyLength < 1 || hash_key->arKey[0] == '\0') { - return 0; /* non public cannot be dynamic */ - } - - ZVAL_STRINGL(&member, hash_key->arKey, hash_key->nKeyLength-1, 0); - if (zend_get_property_info(ce, &member, 1 TSRMLS_CC) == &EG(std_property_info)) { - zend_unmangle_property_name((&EG(std_property_info))->name, (&EG(std_property_info))->name_length, &class_name, &prop_name); - add_assoc_string(retval, prop_name, prop_name, 1); - } - return 0; -} - static void twig_add_class_to_cache(zval *cache, zval *object, char *class_name TSRMLS_DC) { zval *class_info, *class_methods, *class_properties; @@ -649,10 +629,6 @@ static void twig_add_class_to_cache(zval *cache, zval *object, char *class_name zend_hash_apply_with_arguments(&class_ce->function_table APPLY_TSRMLS_CC, twig_add_method_to_class, 1, class_methods); zend_hash_apply_with_arguments(&class_ce->properties_info APPLY_TSRMLS_CC, twig_add_property_to_class, 2, &class_ce, class_properties); - if (object && Z_OBJ_HT_P(object)->get_properties) { - HashTable *properties = Z_OBJ_HT_P(object)->get_properties(object TSRMLS_CC); - zend_hash_apply_with_arguments(properties APPLY_TSRMLS_CC, twig_add_dyn_property_to_class, 2, &class_ce, class_properties); - } add_assoc_zval(class_info, "methods", class_methods); add_assoc_zval(class_info, "properties", class_properties); add_assoc_zval(cache, class_name, class_info); @@ -853,6 +829,12 @@ PHP_FUNCTION(twig_template_get_attributes) efree(class_name); + // An object property can be dynamically created in the get_properties handler of the object, + // so if the property is not found and the object has a custom get_properties_handler, then call it first. + if (! tmp_item && ! TWIG_HAS_PROPERTY(object, &zitem TSRMLS_CC) && Z_OBJ_HT_P(object)->get_properties && Z_OBJ_HT_P(object)->get_properties != zend_std_get_properties) { + Z_OBJ_HT_P(object)->get_properties(object TSRMLS_CC); + } + if (tmp_item || TWIG_HAS_PROPERTY(object, &zitem TSRMLS_CC) || TWIG_ARRAY_KEY_EXISTS(object, item, item_len) // FIXME: Array key? is that array access here? ) { if (isDefinedTest) { diff --git a/test/Twig/Tests/NativeExtensionTest.php b/test/Twig/Tests/NativeExtensionTest.php new file mode 100644 index 000000000..936c12774 --- /dev/null +++ b/test/Twig/Tests/NativeExtensionTest.php @@ -0,0 +1,26 @@ + true, + 'cache' => false, + 'autoescape' => array($this, 'escapingStrategyCallback'), + )); + + // If it fails, PHP will crash. + } +} From db3cb803d2b5c760c943836c27164648bb5f0463 Mon Sep 17 00:00:00 2001 From: Charles Date: Thu, 20 Sep 2012 18:35:44 +0700 Subject: [PATCH 2/6] Fix NativeExtensionTest --- test/Twig/Tests/NativeExtensionTest.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/test/Twig/Tests/NativeExtensionTest.php b/test/Twig/Tests/NativeExtensionTest.php index 936c12774..69833897b 100644 --- a/test/Twig/Tests/NativeExtensionTest.php +++ b/test/Twig/Tests/NativeExtensionTest.php @@ -13,14 +13,17 @@ class Twig_Tests_NativeExtensionTest extends PHPUnit_Framework_TestCase { public function testGetProperties() { - $loader = new Twig_Loader_String('{{ d1.date }}{{ d2.date }}'); - - $twig = new Twig_Environment($loader, array( + $twig = new Twig_Environment(new Twig_Loader_String(), array( 'debug' => true, 'cache' => false, - 'autoescape' => array($this, 'escapingStrategyCallback'), + 'autoescape' => false )); + $twig->render('{{ d1.date }}{{ d2.date }}', array( + 'd1' => new DateTime, + 'd2' => new DateTime + )); + // If it fails, PHP will crash. } } From 498090364b65066eb8cf4139d659c4b9dc1277f6 Mon Sep 17 00:00:00 2001 From: Charles Date: Fri, 21 Sep 2012 14:47:45 +0700 Subject: [PATCH 3/6] Enhancements for twig extension - Fix extension memory leak - Fix gcc compile warning (cast to const char *) - Add TWIG_HAS_DYNAMIC_PROPERTY - Remove the previous translation of 'array_key_exists($item, $object)' into TWIG_ARRAY_KEY_EXISTS when checking object property because what it really means is property_exists which has already been handled by TWIG_HAS_PROPERTY and TWIG_HAS_DYNAMIC_PROPERTY - Fix native extension spacing --- ext/twig/twig.c | 38 ++++++++++++++++--------- test/Twig/Tests/NativeExtensionTest.php | 16 +++++------ 2 files changed, 33 insertions(+), 21 deletions(-) diff --git a/ext/twig/twig.c b/ext/twig/twig.c index f464faf1c..a1b9d0e8b 100644 --- a/ext/twig/twig.c +++ b/ext/twig/twig.c @@ -317,6 +317,19 @@ int TWIG_HAS_PROPERTY(zval *object, zval *propname TSRMLS_DC) return 0; } +int TWIG_HAS_DYNAMIC_PROPERTY(zval *object, zval *propname TSRMLS_DC) +{ + if (Z_OBJ_HT_P(object)->get_properties && Z_OBJ_HT_P(object)->get_properties != zend_std_get_properties) { + return zend_hash_quick_exists( + Z_OBJ_HT_P(object)->get_properties(object TSRMLS_CC), // the properties hash + Z_STRVAL(*propname), // property name + Z_STRLEN(*propname)+1, // property length + zend_get_hash_value(Z_STRVAL(*propname), Z_STRLEN(*propname)+1) // hash value + ); + } + return 0; +} + zval *TWIG_PROPERTY_CHAR(zval *object, char *propname TSRMLS_DC) { zval *tmp_name_zval, *tmp; @@ -563,7 +576,7 @@ char *TWIG_GET_CLASS_NAME(zval *object TSRMLS_DC) if (Z_TYPE_P(object) != IS_OBJECT) { return ""; } - zend_get_object_classname(object, &class_name, &class_name_len TSRMLS_CC); + zend_get_object_classname(object, (const char **) &class_name, &class_name_len TSRMLS_CC); return class_name; } @@ -605,7 +618,7 @@ static int twig_add_property_to_class(void *pDest APPLY_TSRMLS_DC, int num_args, ce = *va_arg(args, zend_class_entry**); retval = va_arg(args, zval*); - zend_unmangle_property_name(pptr->name, pptr->name_length, &class_name, &prop_name); + zend_unmangle_property_name(pptr->name, pptr->name_length, (const char **) &class_name, (const char **) &prop_name); add_assoc_string(retval, prop_name, prop_name, 1); @@ -829,14 +842,7 @@ PHP_FUNCTION(twig_template_get_attributes) efree(class_name); - // An object property can be dynamically created in the get_properties handler of the object, - // so if the property is not found and the object has a custom get_properties_handler, then call it first. - if (! tmp_item && ! TWIG_HAS_PROPERTY(object, &zitem TSRMLS_CC) && Z_OBJ_HT_P(object)->get_properties && Z_OBJ_HT_P(object)->get_properties != zend_std_get_properties) { - Z_OBJ_HT_P(object)->get_properties(object TSRMLS_CC); - } - - if (tmp_item || TWIG_HAS_PROPERTY(object, &zitem TSRMLS_CC) || TWIG_ARRAY_KEY_EXISTS(object, item, item_len) // FIXME: Array key? is that array access here? - ) { + if (tmp_item || TWIG_HAS_PROPERTY(object, &zitem TSRMLS_CC) || TWIG_HAS_DYNAMIC_PROPERTY(object, &zitem TSRMLS_CC)) { if (isDefinedTest) { RETURN_TRUE; } @@ -907,6 +913,10 @@ PHP_FUNCTION(twig_template_get_attributes) } */ } else { + efree(tmp_method_name_get); + efree(tmp_method_name_is); + efree(lcItem); + if (isDefinedTest) { RETURN_FALSE; } @@ -916,10 +926,12 @@ PHP_FUNCTION(twig_template_get_attributes) TWIG_THROW_EXCEPTION("Twig_Error_Runtime" TSRMLS_CC, "Method \"%s\" for object \"%s\" does not exist", item, TWIG_GET_CLASS_NAME(object TSRMLS_CC)); return; } + + efree(tmp_method_name_get); + efree(tmp_method_name_is); + efree(lcItem); + if (isDefinedTest) { - efree(tmp_method_name_get); - efree(tmp_method_name_is); - efree(lcItem); RETURN_TRUE; } /* diff --git a/test/Twig/Tests/NativeExtensionTest.php b/test/Twig/Tests/NativeExtensionTest.php index 69833897b..8329cd8cf 100644 --- a/test/Twig/Tests/NativeExtensionTest.php +++ b/test/Twig/Tests/NativeExtensionTest.php @@ -11,13 +11,13 @@ class Twig_Tests_NativeExtensionTest extends PHPUnit_Framework_TestCase { - public function testGetProperties() - { - $twig = new Twig_Environment(new Twig_Loader_String(), array( - 'debug' => true, - 'cache' => false, - 'autoescape' => false - )); + public function testGetProperties() + { + $twig = new Twig_Environment(new Twig_Loader_String(), array( + 'debug' => true, + 'cache' => false, + 'autoescape' => false + )); $twig->render('{{ d1.date }}{{ d2.date }}', array( 'd1' => new DateTime, @@ -25,5 +25,5 @@ class Twig_Tests_NativeExtensionTest extends PHPUnit_Framework_TestCase )); // If it fails, PHP will crash. - } + } } From 34cf8e1c6e5e4c90243c3e204d5c8f22b19e3469 Mon Sep 17 00:00:00 2001 From: Charles Date: Fri, 21 Sep 2012 16:42:13 +0700 Subject: [PATCH 4/6] Fix double free --- ext/twig/twig.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/ext/twig/twig.c b/ext/twig/twig.c index a1b9d0e8b..4476bb7f0 100644 --- a/ext/twig/twig.c +++ b/ext/twig/twig.c @@ -927,11 +927,10 @@ PHP_FUNCTION(twig_template_get_attributes) return; } - efree(tmp_method_name_get); - efree(tmp_method_name_is); - efree(lcItem); - if (isDefinedTest) { + efree(tmp_method_name_get); + efree(tmp_method_name_is); + efree(lcItem); RETURN_TRUE; } /* @@ -943,6 +942,9 @@ PHP_FUNCTION(twig_template_get_attributes) TWIG_CALL_ZZ(TWIG_CALL_S(TWIG_PROPERTY_CHAR(template, "env" TSRMLS_CC), "getExtension", "sandbox" TSRMLS_CC), "checkMethodAllowed", object, &zitem TSRMLS_CC); } if (EG(exception)) { + efree(tmp_method_name_get); + efree(tmp_method_name_is); + efree(lcItem); return; } /* From 9126dc649c2f501ddc4a0c9d8095158d82ecaf16 Mon Sep 17 00:00:00 2001 From: Charles Date: Fri, 21 Sep 2012 17:29:41 +0700 Subject: [PATCH 5/6] Twig extension: fix case when accessing property of an array casted into object --- ext/twig/twig.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/ext/twig/twig.c b/ext/twig/twig.c index 4476bb7f0..c94fa0f4a 100644 --- a/ext/twig/twig.c +++ b/ext/twig/twig.c @@ -317,14 +317,14 @@ int TWIG_HAS_PROPERTY(zval *object, zval *propname TSRMLS_DC) return 0; } -int TWIG_HAS_DYNAMIC_PROPERTY(zval *object, zval *propname TSRMLS_DC) +int TWIG_HAS_DYNAMIC_PROPERTY(zval *object, char *prop, int prop_len TSRMLS_DC) { - if (Z_OBJ_HT_P(object)->get_properties && Z_OBJ_HT_P(object)->get_properties != zend_std_get_properties) { + if (Z_OBJ_HT_P(object)->get_properties) { return zend_hash_quick_exists( - Z_OBJ_HT_P(object)->get_properties(object TSRMLS_CC), // the properties hash - Z_STRVAL(*propname), // property name - Z_STRLEN(*propname)+1, // property length - zend_get_hash_value(Z_STRVAL(*propname), Z_STRLEN(*propname)+1) // hash value + Z_OBJ_HT_P(object)->get_properties(object TSRMLS_CC), // the properties hash + prop, // property name + prop_len + 1, // property length + zend_get_hash_value(prop, prop_len + 1) // hash value ); } return 0; @@ -842,7 +842,7 @@ PHP_FUNCTION(twig_template_get_attributes) efree(class_name); - if (tmp_item || TWIG_HAS_PROPERTY(object, &zitem TSRMLS_CC) || TWIG_HAS_DYNAMIC_PROPERTY(object, &zitem TSRMLS_CC)) { + if (tmp_item || TWIG_HAS_PROPERTY(object, &zitem TSRMLS_CC) || TWIG_HAS_DYNAMIC_PROPERTY(object, item, item_len TSRMLS_CC)) { if (isDefinedTest) { RETURN_TRUE; } From c23ef25c8c1e0d1ce939b86b1ce2d89eb1a1c138 Mon Sep 17 00:00:00 2001 From: Charles Date: Mon, 24 Sep 2012 09:04:21 +0700 Subject: [PATCH 6/6] Add assertEquals to NativeExtensionTest.php --- test/Twig/Tests/NativeExtensionTest.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/Twig/Tests/NativeExtensionTest.php b/test/Twig/Tests/NativeExtensionTest.php index 8329cd8cf..d8284369e 100644 --- a/test/Twig/Tests/NativeExtensionTest.php +++ b/test/Twig/Tests/NativeExtensionTest.php @@ -19,11 +19,11 @@ class Twig_Tests_NativeExtensionTest extends PHPUnit_Framework_TestCase 'autoescape' => false )); - $twig->render('{{ d1.date }}{{ d2.date }}', array( - 'd1' => new DateTime, - 'd2' => new DateTime - )); + $d1 = new DateTime; + $d2 = new DateTime; + $output = $twig->render('{{ d1.date }}{{ d2.date }}', compact('d1', 'd2')); // If it fails, PHP will crash. + $this->assertEquals($output, $d1->date . $d2->date); } }