Issues

ZF-9018: Zend_Reflection_Method::getBody() drops closing curly brace of method content

Issue Type: Bug Created: 2010-01-29T16:10:17.000+0000 Last Updated: 2012-06-01T17:28:30.000+0000 Status: Resolved Fix version(s): - 1.12.0 (27/Aug/12)

Reporter: Aurélien (bradypus) Assignee: Adam Lundrigan (adamlundrigan) Tags: - Zend_Reflection

  • FixForZF1.12
  • state:patch-ready-for-review
  • zf-caretaker-adamlundrigan
  • zf-crteam-review

Related issues: - ZF-10870

Attachments: - library.diff

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)

Comments

Posted by Jan Pieper (jpieper) on 2010-04-03T05:25:13.000+0000

Added diff files (library, tests).

Posted by Jan Pieper (jpieper) on 2010-04-03T05:25:57.000+0000

Reassign to Ralph for validation.

Posted by BitConstructor Co. Ltd. (bitconstructor) on 2010-12-30T21:56:57.000+0000

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.

Posted by Franck STAUFFER (franckstauffer) on 2011-08-26T13:56:24.000+0000

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.

Posted by Franck STAUFFER (franckstauffer) on 2011-08-30T10:58:24.000+0000

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

Posted by Adam Lundrigan (adamlundrigan) on 2011-10-17T16:15:27.000+0000

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:

<pre class="highlight">
5) Zend_CodeGenerator_Php_MethodTest::testMethodFromReflection
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
     {
+
         /* test test */
+
     }

Posted by Franck STAUFFER (franckstauffer) on 2011-10-17T16:32:47.000+0000

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

Posted by Adam Lundrigan (adamlundrigan) on 2011-10-17T16:52:32.000+0000

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:

<pre class="highlight">
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.

Posted by Franck STAUFFER (franckstauffer) on 2011-10-18T14:18:21.000+0000

OK, thanks for the clarification.

Posted by Adam Lundrigan (adamlundrigan) on 2012-03-09T20:03:25.000+0000

Attached file: ZF-9018.patch

Posted by Adam Lundrigan (adamlundrigan) on 2012-06-01T17:28:01.000+0000

Fixed in trunk (1.12.0): r24860

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