Fix code reflection - getBody/getContents method #5245

Closed
wants to merge 15 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

blanchonvincent commented Oct 8, 2013

Zend\Code\Reflection getBody function is often broken with some code format

  • implement getBody in FunctionReflection
  • fix getBody in MethodReflection
  • fix getContents in FunctionReflection
  • fix getContents in MethodReflection

@blanchonvincent blanchonvincent referenced this pull request in Ocramius/ProxyManager Oct 8, 2013

Closed

[TO BE MOVED] Parametric polymorphism proxy / Object method extensions #103

7 of 7 tasks complete
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_Code
+namespace ZendTest\Code\Reflection\TestAsset;
+
+$function1 = function()
+{
@samsonasik

samsonasik Oct 8, 2013

Contributor

move open brace up and add space for PSR-2

@blanchonvincent

blanchonvincent Oct 8, 2013

Contributor

no, it's deliberate :)

@samsonasik

samsonasik Oct 8, 2013

Contributor

ok ^^

@Maks3w

Maks3w Oct 8, 2013

Member

@blanchonvincent Anyway, please add a comment advising that is deliberated (and explaining why) for prevent accidental changes in the future (mass changes, etc).

@samsonasik

samsonasik Oct 8, 2013

Contributor

@blanchonvincent I think that brace at new line is one of the reasons that make travis failure instead of trailing_spaces.

Contributor

blanchonvincent commented Oct 11, 2013

@Ocramius can you confirm that on this function :

function1()
{
    return 'foo';
}

getBody is :

return 'foo';

getContent is :

function1()
{
    return 'foo';
}

?

Member

Ocramius commented Oct 17, 2013

@blanchonvincent never used that API - are there no other tests for it?

Contributor

blanchonvincent commented Oct 17, 2013

@Ocramius The prototype is

public function getContents($includeDocBlock = true)

So, if the contents is :

{
   return ....;
}

what is the contents with docblock ? I think there is a bug, and the contents must be :

function foo()
{
   return ....;
}

and with docblock :

/**
 * bla bla bla
 */
function foo()
{
   return ....;
}
Member

Ocramius commented Oct 17, 2013

@blanchonvincent yep, I agree on that. I'd just first check what the consumers of that method are before applying any change.

Contributor

blanchonvincent commented Oct 17, 2013

@Ocramius, Ok, let me know

Member

Ocramius commented Oct 17, 2013

@blanchonvincent no, I was suggesting you to grep for its usage :)

Contributor

blanchonvincent commented Oct 19, 2013

@Ocramius, Look never used, and $useDockBlock param doesn't work, and lot of time, the function return an empty result ... so I fix that.

Contributor

blanchonvincent commented Oct 19, 2013

PR is finished -- better code coverage & lot of fixs

Contributor

mwillbanks commented Oct 20, 2013

@Ocramius since you did original review; could you please review a last time?

+
+ $content = false;
+ if ($this->isClosure()) {
+ preg_match('#function\s*\([^\)]*\)\s*(use\s*\([^\)]+\))?\s*\{(.*\;)?\s*\}#s', $functionLine, $matches);
@Ocramius

Ocramius Oct 20, 2013

Member

Woah, this witchcraft needs a comment :-)

@Ocramius

Ocramius Oct 20, 2013

Member

Does this handle also closures containing closures?

@weierophinney

weierophinney Oct 23, 2013

Owner

@Ocramius I don't see any reason why it wouldn't -- the body regex is looking for any number of terminated statements.

@blanchonvincent

blanchonvincent Oct 28, 2013

Contributor

@Ocramius, yep, look at the tests

+ }
+ } else {
+ $name = substr($this->getName(), strrpos($this->getName(), '\\')+1);
+ preg_match('#function\s+' . $name . '\s*\([^\)]*\)\s*{([^{}]+({[^}]+})*[^}]+)?}#', $functionLine, $matches);
@Ocramius

Ocramius Oct 20, 2013

Member

Same here (comment)

@Ocramius

Ocramius Oct 20, 2013

Member

$name should go through preg_quote

+ }
+ }
+
+ return $includeDocBlock && $this->getDocComment() ? $this->getDocComment() . "\n" . $content : $content;
@Ocramius

Ocramius Oct 20, 2013

Member

Can you avoid calling $this->getDocComment() twice here? It's not about performance, but you can't trust $this->getDocComment() retrieving always the same result.

@Ocramius

Ocramius Oct 20, 2013

Member

As an additional note: what if the separator between body and docblock is not \n (for example \n\n\n)? (may also not be relevant)

@blanchonvincent

blanchonvincent Oct 28, 2013

Contributor

I think I can't catch if the separator is not a \n :/

+ $fileName = $this->getFileName();
+ if (false === $fileName) {
+ throw new Exception\InvalidArgumentException(
+ 'Cannot determine internals functions body'
@Ocramius

Ocramius Oct 20, 2013

Member

What if the class is eval'd?

+ $fileName = $this->getFileName();
+ if (false === $fileName) {
+ throw new Exception\InvalidArgumentException(
+ 'Cannot determine internals functions contents'
@Ocramius

Ocramius Oct 20, 2013

Member

What if the function is eval'd?

@blanchonvincent

blanchonvincent Oct 28, 2013

Contributor

ok, test added

+
+ $body = false;
+ if ($this->isClosure()) {
+ preg_match('#function\s*\([^\)]*\)\s*(use\s*\([^\)]+\))?\s*\{(.*\;)\s*\}#s', $functionLine, $matches);
@Ocramius

Ocramius Oct 20, 2013

Member

Same remarks as above

@Ocramius

Ocramius Oct 20, 2013

Member

Also: these regexes look constant. Can you move them to either a class constant or a private property?

@weierophinney

weierophinney Oct 30, 2013

Owner

They're not -- there are subtle differences in what each is matching.

+ }
+ } else {
+ $name = substr($this->getName(), strrpos($this->getName(), '\\')+1);
+ preg_match('#function\s+' . $name . '\s*\([^\)]*\)\s*{([^{}]+({[^}]+})*[^}]+)}#', $functionLine, $matches);
@Ocramius

Ocramius Oct 20, 2013

Member

Same remarks as above

+ /**
+ * Get method body
+ *
+ * @return string
@Ocramius

Ocramius Oct 20, 2013

Member

The result may be false

+ );
+
+ $functionLine = implode("\n", $lines);
+ preg_match('#[(public|protected|private|abstract|final|static)\s*]+function\s+' . $this->getName() . '\s*\([^\)]*\)\s*{([^{}]+({[^}]+})*[^}]+)?}#s', $functionLine, $matches);
@Ocramius

Ocramius Oct 20, 2013

Member

Split this line

@Ocramius

Ocramius Oct 20, 2013

Member

a function doesn't necessarily have a visibility descriptor:

class Foo
{
    function bar()
    {
    }
}
+
+ $content = $matches[0];
+
+ return $includeDocBlock && $this->getDocComment() ? $this->getDocComment() . "\n" . $content : $content;
@Ocramius

Ocramius Oct 20, 2013

Member

Same as above - don't trust getDocComment()

+ $fileName = $this->getFileName();
+ if (false === $fileName) {
+ throw new Exception\InvalidArgumentException(
+ 'Cannot determine internals functions body'
@Ocramius

Ocramius Oct 20, 2013

Member

eval'd code?

true
);
- $firstLine = array_shift($lines);
+ $functionLine = implode("\n", $lines);
+ preg_match('#[(public|protected|private|abstract|final|static)\s*]+function\s+' . $this->getName() . '\s*\([^\)]*\)\s*{([^{}]+({[^}]+})*[^}]+)}#s', $functionLine, $matches);
@Ocramius

Ocramius Oct 20, 2013

Member

preg_quote?

@Ocramius

Ocramius Oct 20, 2013

Member

Same comment about the visibility descriptors

@@ -32,7 +32,187 @@ public function testParemeterReturn()
public function testFunctionDocBlockReturn()
{
require_once __DIR__ . '/TestAsset/functions.php';
- $function = new FunctionReflection('ZendTest\Code\Reflection\TestAsset\function6');
+ $function = new FunctionReflection('ZendTest\Code\Reflection\TestAsset\function3');
@Ocramius

Ocramius Oct 20, 2013

Member

Can you avoid altering the test? (a reason why the test was modified is also ok for me)

@blanchonvincent

blanchonvincent Oct 28, 2013

Contributor

yep because i renamed function :)

public function testGetBodyReturnsCorrectBody()
{
- $body = ' //we need a multi-line method body.
@Ocramius

Ocramius Oct 20, 2013

Member

Also here, can you avoid modifying the existing test?

@blanchonvincent

blanchonvincent Oct 28, 2013

Contributor

I updated the test because it fail with the function update. There was a bug with \n, so i just add a \n

Member

Maks3w commented Oct 29, 2013

@blanchonvincent Please rebase the PR for fix merge conflicts

weierophinney added a commit that referenced this pull request Oct 30, 2013

Merge pull request #5245 from blanchonvincent/feature/zend-code-gener…
…ator-functiongenerator

Fix code reflection - getBody/getContents method

Conflicts:
	library/Zend/Code/Reflection/FunctionReflection.php
	library/Zend/Code/Reflection/MethodReflection.php
	tests/ZendTest/Code/Reflection/FunctionReflectionTest.php
	tests/ZendTest/Code/Reflection/MethodReflectionTest.php

weierophinney added a commit that referenced this pull request Oct 30, 2013

[#5245] CS fixes
- trailing whitespace, braces
- Added functions.php to the exclude list for php-cs-fixer, as the tests are
  testing specific formatting issues.

weierophinney added a commit that referenced this pull request Oct 30, 2013

@ghost ghost assigned weierophinney Oct 30, 2013

Owner

weierophinney commented Oct 30, 2013

Merged to develop for release with 2.3.0, as it represents new functionality. I took care of the merge conflicts.

stefanotorresi added a commit to stefanotorresi/zf2 that referenced this pull request Oct 31, 2013

Revert "[#5245] CS fixes"
This reverts commit 76074d4.

@stefanotorresi stefanotorresi referenced this pull request Oct 31, 2013

Merged

CS fix for #5245 #5391

weierophinney added a commit that referenced this pull request Nov 1, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment