Zend Framework

Zend_Reflection_Method::getBody() removes the last close brace

Details

Description

When I use Zend_Reflection_Method::getBody() on a method like that :

method() {
if(1) { ; }
}

it returns only :
if(1) {
;

I don't understand why getBody() returns rtrim(ltrim(implode("\n", $lines), '{'), '}') instead of only implode("\n", $lines)

Attachments

  1. File
    library.diff
    30/Aug/11 10:58 AM
    2 kB
    Franck STAUFFER
  2. Text File
    ZF-9018.patch
    09/Mar/12 8:02 PM
    3 kB
    Adam Lundrigan
  3. Java Source File
    ZF-9018-library.diff
    03/Apr/10 5:25 AM
    2 kB
    Jan Pieper
  4. Java Source File
    ZF-9018-tests.diff
    03/Apr/10 5:25 AM
    4 kB
    Jan Pieper

Issue Links

This issue is duplicated by:
ZF-11472 "zf create action" can cause malformed PHP when other action methods already exist Major Closed
ZF-12040 Syntax error in Controller file after creating an Action Minor Closed
This issue is related to:
ZF-10870 getBody in Zend_Reflection_Method returns wrong on multi-line function declarations Major Open
This issue is related to:
ZF-9501 zf tool create action breaks the php code in controller Major Open

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Jan Pieper added a comment -

Added diff files (library, tests).

Show
Jan Pieper added a comment - Added diff files (library, tests).
Hide
Permalink
Jan Pieper added a comment -

Reassign to Ralph for validation.

Show
Jan Pieper added a comment - Reassign to Ralph for validation.
Hide
Permalink
BitConstructor Co. Ltd. added a comment -

this issue is stil present in 1.11.2 (and trunk) actually now more broken than ever.

Additionally to faulty formatting and wrongly cut out closing brace of structures like (if, ..) at end of function calls it now messes up the document head. The files arent more declared as PHP.

It removes <?php from beginning of file and adds following code at end of file;
<?php
/* Zend_CodeGenerator_Php_File-DocblockMarker */

Hope somebody can fix it soon. We are writing bigger applications and have to clean up the classes always after adding a action.

Otherwise Zend Tool is a great help and thank you all for creating it.

Show
BitConstructor Co. Ltd. added a comment - this issue is stil present in 1.11.2 (and trunk) actually now more broken than ever. Additionally to faulty formatting and wrongly cut out closing brace of structures like (if, ..) at end of function calls it now messes up the document head. The files arent more declared as PHP. It removes <?php from beginning of file and adds following code at end of file; <?php /* Zend_CodeGenerator_Php_File-DocblockMarker */ Hope somebody can fix it soon. We are writing bigger applications and have to clean up the classes always after adding a action. Otherwise Zend Tool is a great help and thank you all for creating it.
Hide
Permalink
Franck STAUFFER added a comment -

The patch doesn't fully solve the issue as getStartLine on ReflectionMethod returns the line number containing the name of the function...a line break in the arguments lists for instance would also break the parsing. The only solution that's a 100% sure to fix the issue (which I have a patch for) is to use token_get_all to extract the method's body.

Show
Franck STAUFFER added a comment - The patch doesn't fully solve the issue as getStartLine on ReflectionMethod returns the line number containing the name of the function...a line break in the arguments lists for instance would also break the parsing. The only solution that's a 100% sure to fix the issue (which I have a patch for) is to use token_get_all to extract the method's body.
Hide
Permalink
Franck STAUFFER added a comment -

New patch to fix the bug using the PHP tokenizer to make sure any kind of weird source formatting doesn't break the parsing.

Show
Franck STAUFFER added a comment - New patch to fix the bug using the PHP tokenizer to make sure any kind of weird source formatting doesn't break the parsing.
Hide
Permalink
Adam Lundrigan added a comment -

This patch breaks compatibility with the existing implementation, as it includes everything (specifically, the leading and trailing whitespace characters) inside the method brackets. When I run the Zend_CodeGenerator test suite after applying your patch, I get failures like these:

5) Zend_CodeGenerator_Php_MethodTest::testMethodFromReflection
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
     {
+
         /* test test */
+
     }
Show
Adam Lundrigan added a comment - This patch breaks compatibility with the existing implementation, as it includes everything (specifically, the leading and trailing whitespace characters) inside the method brackets. When I run the Zend_CodeGenerator test suite after applying your patch, I get failures like these: 
5) Zend_CodeGenerator_Php_MethodTest::testMethodFromReflection
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
     {
+
         /* test test */
+
     }
Hide
Permalink
Franck STAUFFER added a comment -

Yeah I know, but I don't immediately see the point in not preserving the full method body as is - at least that's not something (as an API user) I'd expect.

Do we agree that using the tokenizer is the only sensible way to make sure we get it right regardless of the coding style used in PHP files Zend_Reflection_Method gets called on ?
If we agree I can resubmit the patch to restore leading and trailing whitespaces removal, if not, then I just want to emphasize that the previous patch that was attached to this also fails in many cases.

Thanks

Show
Franck STAUFFER added a comment - Yeah I know, but I don't immediately see the point in not preserving the full method body as is - at least that's not something (as an API user) I'd expect. Do we agree that using the tokenizer is the only sensible way to make sure we get it right regardless of the coding style used in PHP files Zend_Reflection_Method gets called on ? If we agree I can resubmit the patch to restore leading and trailing whitespaces removal, if not, then I just want to emphasize that the previous patch that was attached to this also fails in many cases. Thanks
Hide
Permalink
Adam Lundrigan added a comment -

I absolutely agree. The current implementation of Zend_Reflection_Method::getBody blows up when it's given a PHP file which doesn't conform to Zend Framework coding standards; that is a product of it's design, which was primarily driven by it's use in Zend_Tool/Zend_CodeGenerator. Your approach is much better than the current implementation, unfortunately the existing narrow behavior is already in the wild and it's highly unlikely to be changed in ZFv1 without risk of breaking backwards-compatibility. I would push for your patch to be integrated into ZF2's Zend\Code\Reflection component.

In the meanwhile, we can probably fix the original poster's issue (which is the root cause of ZF-9501) using the simple fix below:

Index: library/Zend/Reflection/Method.php
===================================================================
--- library/Zend/Reflection/Method.php  (revision 24511)
+++ library/Zend/Reflection/Method.php  (working copy)
@@ -163,6 +163,6 @@
         }

         // just in case we had code on the bracket lines
-        return rtrim(ltrim(implode("\n", $lines), '{'), '}');
+        return implode("\n", $lines);
     }
 }
Index: tests/Zend/Reflection/MethodTest.php
===================================================================
--- tests/Zend/Reflection/MethodTest.php        (revision 24511)
+++ tests/Zend/Reflection/MethodTest.php        (working copy)
@@ -26,6 +26,11 @@
 require_once 'Zend/Reflection/Method.php';

 /**
+ * @see ZF-9018
+ */
+require_once dirname(__FILE__) . '/_files/ZF9018TestClass.php';
+
+/**
  * @category   Zend
  * @package    Zend_Reflection
  * @subpackage UnitTests
@@ -79,6 +84,36 @@
         $reflectionMethod = new Zend_Reflection_Method('Zend_Reflection_TestSampleClass6', 'doSomething');
         $this->assertEquals($body, $reflectionMethod->getBody());
     }
+
+    /**
+     * @group ZF-9018
+     * @group ZF-9501
+     */
+    public function testGetBodyReturnsCorrectBodyWhenContentEndsWithClosingCurlyBrace()
+    {
+        $body = '        if ( true ) {
+            echo "True";
+        } else {
+            echo "False";
+        }';
+        $reflectionMethod = new Zend_Reflection_Method('ZF9018TestClass', 'doSomething');
+        $this->assertEquals($body, $reflectionMethod->getBody());
+    }
+
+    /**
+     * @group ZF-9018
+     * @group ZF-9501
+     */
+    public function testGetBodyReturnsCorrectBodyWhenMethodWithInlineOpenBraceHasBodyWhichEndsWithClosingCurlyBrace()
+    {
+        $body = '        if ( true ) {
+            echo "True";
+        } else {
+            echo "False";
+        }';
+        $reflectionMethod = new Zend_Reflection_Method('ZF9018TestClass', 'doSomethingOpenBraceInline');
+        $this->assertEquals($body, $reflectionMethod->getBody());
+    }

     public function testGetContentsReturnsCorrectContent()
     {
Index: tests/Zend/Reflection/_files/ZF9018TestClass.php
===================================================================
--- tests/Zend/Reflection/_files/ZF9018TestClass.php    (revision 0)
+++ tests/Zend/Reflection/_files/ZF9018TestClass.php    (revision 0)
@@ -0,0 +1,21 @@
+<?php
+
+class ZF9018TestClass
+{
+    public function doSomething($var)
+    {
+        if ( true ) {
+            echo "True";
+        } else {
+            echo "False";
+        }
+    }
+
+    public function doSomethingOpenBraceInline($var) {
+        if ( true ) {
+            echo "True";
+        } else {
+            echo "False";
+        }
+    }
+}
\ No newline at end of file

Property changes on: tests/Zend/Reflection/_files/ZF9018TestClass.php
___________________________________________________________________
Added: svn:keywords
   + Id

As far as I can see, it fixes the issue with ZF-9501 without breaking the existing tested behavior of Zend_Reflection, Zend_Tool or Zend_CodeGenerator.

Show
Adam Lundrigan added a comment - I absolutely agree. The current implementation of Zend_Reflection_Method::getBody blows up when it's given a PHP file which doesn't conform to Zend Framework coding standards; that is a product of it's design, which was primarily driven by it's use in Zend_Tool/Zend_CodeGenerator. Your approach is much better than the current implementation, unfortunately the existing narrow behavior is already in the wild and it's highly unlikely to be changed in ZFv1 without risk of breaking backwards-compatibility. I would push for your patch to be integrated into ZF2's Zend\Code\Reflection component. In the meanwhile, we can probably fix the original poster's issue (which is the root cause of ZF-9501) using the simple fix below: 
Index: library/Zend/Reflection/Method.php
===================================================================
--- library/Zend/Reflection/Method.php  (revision 24511)
+++ library/Zend/Reflection/Method.php  (working copy)
@@ -163,6 +163,6 @@
         }

         // just in case we had code on the bracket lines
-        return rtrim(ltrim(implode("\n", $lines), '{'), '}');
+        return implode("\n", $lines);
     }
 }
Index: tests/Zend/Reflection/MethodTest.php
===================================================================
--- tests/Zend/Reflection/MethodTest.php        (revision 24511)
+++ tests/Zend/Reflection/MethodTest.php        (working copy)
@@ -26,6 +26,11 @@
 require_once 'Zend/Reflection/Method.php';

 /**
+ * @see ZF-9018
+ */
+require_once dirname(__FILE__) . '/_files/ZF9018TestClass.php';
+
+/**
  * @category   Zend
  * @package    Zend_Reflection
  * @subpackage UnitTests
@@ -79,6 +84,36 @@
         $reflectionMethod = new Zend_Reflection_Method('Zend_Reflection_TestSampleClass6', 'doSomething');
         $this->assertEquals($body, $reflectionMethod->getBody());
     }
+
+    /**
+     * @group ZF-9018
+     * @group ZF-9501
+     */
+    public function testGetBodyReturnsCorrectBodyWhenContentEndsWithClosingCurlyBrace()
+    {
+        $body = '        if ( true ) {
+            echo "True";
+        } else {
+            echo "False";
+        }';
+        $reflectionMethod = new Zend_Reflection_Method('ZF9018TestClass', 'doSomething');
+        $this->assertEquals($body, $reflectionMethod->getBody());
+    }
+
+    /**
+     * @group ZF-9018
+     * @group ZF-9501
+     */
+    public function testGetBodyReturnsCorrectBodyWhenMethodWithInlineOpenBraceHasBodyWhichEndsWithClosingCurlyBrace()
+    {
+        $body = '        if ( true ) {
+            echo "True";
+        } else {
+            echo "False";
+        }';
+        $reflectionMethod = new Zend_Reflection_Method('ZF9018TestClass', 'doSomethingOpenBraceInline');
+        $this->assertEquals($body, $reflectionMethod->getBody());
+    }

     public function testGetContentsReturnsCorrectContent()
     {
Index: tests/Zend/Reflection/_files/ZF9018TestClass.php
===================================================================
--- tests/Zend/Reflection/_files/ZF9018TestClass.php    (revision 0)
+++ tests/Zend/Reflection/_files/ZF9018TestClass.php    (revision 0)
@@ -0,0 +1,21 @@
+<?php
+
+class ZF9018TestClass
+{
+    public function doSomething($var)
+    {
+        if ( true ) {
+            echo "True";
+        } else {
+            echo "False";
+        }
+    }
+
+    public function doSomethingOpenBraceInline($var) {
+        if ( true ) {
+            echo "True";
+        } else {
+            echo "False";
+        }
+    }
+}
\ No newline at end of file

Property changes on: tests/Zend/Reflection/_files/ZF9018TestClass.php
___________________________________________________________________
Added: svn:keywords
   + Id
As far as I can see, it fixes the issue with ZF-9501 without breaking the existing tested behavior of Zend_Reflection, Zend_Tool or Zend_CodeGenerator.
Hide
Permalink
Franck STAUFFER added a comment -

OK, thanks for the clarification.

Show
Franck STAUFFER added a comment - OK, thanks for the clarification.
Hide
Permalink
Adam Lundrigan added a comment -

Attached file: ZF-9018.patch

Show
Adam Lundrigan added a comment - Attached file: ZF-9018.patch

People

Vote (6)
Watch (6)

Dates

  • Created:
    Updated:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Zend Framework. Try JIRA - bug tracking software for your team.