Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Zend code generator eats last brace #6255

Closed
Danack opened this issue May 12, 2014 · 14 comments
Closed

Zend code generator eats last brace #6255

Danack opened this issue May 12, 2014 · 14 comments
Assignees
Milestone

Comments

@Danack
Copy link

Danack commented May 12, 2014

When there is a brace immediately before the closing brace of a method, that penultimate brace is not present in the output. e.g.

require_once('../vendor/autoload.php');

use Weaver\ExtendWeaveInfo;
use Weaver\MethodBinding;
use Weaver\ImplementsWeaveInfo;

use Zend\Code\Reflection\ClassReflection;
use Zend\Code\Generator\ClassGenerator;

use Zend\Code\Generator\MethodGenerator;

class SomeClass {

    function someFunction() {

       if (true) {
           echo "true";
       }
    }
}

$reflector = new ClassReflection('SomeClass');
$classGenerator = new ClassGenerator();
$methods = $reflector->getMethods();

$classGenerator->setName("OutputClass");

foreach ($methods as $method) {
    $methodGenerator = MethodGenerator::fromReflection($method);

    $classGenerator->addMethodFromGenerator($methodGenerator);
}

$text = $classGenerator->generate();

echo $text;

Outputs:

class OutputClass
{

    public function someFunction()
    {
        if (true) {
                   echo "true";
    }


}

So the code is not valid.

@Danack
Copy link
Author

Danack commented May 13, 2014

btw there is a hacky workaround of adding a comment between the braces:

class SomeClass {

    function someFunction() {

       if (true) {
           echo "true";
       }
        // Don't eat me
    }
}

@steverhoades
Copy link
Contributor

I am unable to reproduce this against master.

The following test was performed against the "SomeClass" provided.

File: ZendTest/Code/Generator/ClassGeneratorTest.php

    /**
     * @group 6255
     */
    public function testClassReflectionParsesBracesCorrectly()
    {
        $expected = <<<CODE
class OutputClass
{

    public function someFunction()
    {


        if (true) {
        echo "true";
        }

    }


}

CODE;

        $reflector = new ClassReflection('ZendTest\Code\Generator\SomeClass');
        $classGenerator = new ClassGenerator();
        $methods = $reflector->getMethods();

        $classGenerator->setName("OutputClass");

        foreach ($methods as $method) {

            $methodGenerator = MethodGenerator::fromReflection($method);

            $classGenerator->addMethodFromGenerator($methodGenerator);
        }

        $text = $classGenerator->generate();
        $this->assertEquals(str_replace(array("\n", " ", "\t"), "", $text), str_replace(array("\n", " ", "\t"), "", $expected));
    }

$ phpunit --group 6255 ZendTest/Code/Generator/ClassGeneratorTest.php
OK (1 test, 1 assertion)

@Danack
Copy link
Author

Danack commented May 13, 2014

Hi Steve,

It appears that the bug is very fragile, and depends on the exact whitespace - which may or may not get eaten by Github/HTML

<?php

use Zend\Code\Reflection\ClassReflection;
use Zend\Code\Generator\ClassGenerator;
use Zend\Code\Generator\MethodGenerator;


require_once('../vendor/autoload.php');


class OutputClass
{

    public function thisIsOkay()
    {
        if (true) {
            echo "true"; //space between bracket on next line and method closing bracket
        }

    }

    public function thisIsOkay2()
    {
        if (true) {
            echo "true";// comment between next bracket and closing bracket
        }
        //Don't eat me
    }

    public function thisIsOkay3()
    {
        if (true) {
            echo "true"; //next line has spaces after bracket
        } 
    }

    public function thisIsBroken()
    {
        if (true) {
            echo "true"; //Next line ends on bracket
        }
    }
}

$reflector = new ClassReflection('OutputClass');
$classGenerator = new ClassGenerator();
$methods = $reflector->getMethods();

$classGenerator->setName("OutputClass");

foreach ($methods as $method) {

    $methodGenerator = MethodGenerator::fromReflection($method);

    $classGenerator->addMethodFromGenerator($methodGenerator);
}

$text = $classGenerator->generate();
echo $text;


outputs:


class OutputClass
{

    public function thisIsOkay()
    {
        if (true) {
                    echo "true";
                }
    }

    public function thisIsOkay2()
    {
        if (true) {
                    echo "true";// comment between next bracket and closing bracket
                }
                //Don't eat me
    }

    public function thisIsOkay3()
    {
        if (true) {
                    echo "true"; //next line has spaces after bracket
                }
    }

    public function thisIsBroken()
    {
        if (true) {
                    echo "true"; //Next line ends on bracket
    }


}

I've put the code in a gist https://gist.github.com/Danack/36ab9cd01600ad71a654 to avoid HTML whitespace shenanigans.

@steverhoades
Copy link
Contributor

Strange - I copied your raw code, executed and I still get the follow output:

class OutputClass
{

    public function thisIsBroken()
    {
        if (true) {
                    echo "true"; //Next line ends on bracket
                }
    }


}

It appears to work for me still on master.

@Danack
Copy link
Author

Danack commented May 14, 2014

Apologies - it looks like my composer is grabbing an old version, and not the latest version. I'll retest this and the other issues I raised, making sure that I have the latest version.

@Danack
Copy link
Author

Danack commented May 15, 2014

Hi,

I've retested against 2.3.0. (version 2.3.1 is not installable) All of the above cases pass, however the below fails:

class OutputClass
{
    public function thisIsBroken()
    {
        if (true) {
            echo "true"; //Next line has two brackets
        }}
}

I'll retest when 2.3.1 is installable.

@steverhoades
Copy link
Contributor

Danack,

I have added a patch for this issue available here: https://github.com/steverhoades/zf2/tree/hotfix/6255. If you could please test this against your issues I would appreciate it.

Steve

@Ocramius
Copy link
Member

@Danack you should really test latest master/develop instead of tagged versions

@Danack
Copy link
Author

Danack commented May 15, 2014

Hi Steve,

Yes that looks fixed in your hotfix branch.

cheers
Dan

@steverhoades
Copy link
Contributor

@Danack @Ocramius please see pull request #6286, not sure why it didn't automatically link.

@Danack
Copy link
Author

Danack commented May 15, 2014

Not sure if you wanted me to test this or not, so apologies if I'm testing something while you're still working on it, but this:

<?php

require_once __DIR__ . '/./library/Zend/Loader/ClassMapAutoloader.php';
$loader = new Zend\Loader\ClassMapAutoloader();
$loader->registerAutoloadMap(__DIR__ . '/./library/autoload_classmap.php');
$loader->register();

use Zend\Code\Reflection\ClassReflection;
use Zend\Code\Generator\ClassGenerator;

use Zend\Code\Generator\MethodGenerator;

class SomeClass {

    function someFunction() {
        echo "true";
    }
}

$reflector = new ClassReflection('SomeClass');
$classGenerator = new ClassGenerator();
$methods = $reflector->getMethods();

$classGenerator->setName("OutputClass");

foreach ($methods as $method) {
    $methodGenerator = MethodGenerator::fromReflection($method);
    $classGenerator->addMethodFromGenerator($methodGenerator);
}

$text = $classGenerator->generate();

echo $text;

outputs:


class OutputClass
{

    public function someFunction()
    {
    }


}

i.e. the method body is gone.

But yeah, I also think a token based approach is the only reasonable way of doing this long term.

@steverhoades
Copy link
Contributor

@Danack which branch you using to test this? It appears to work fine for me currently.

@Danack
Copy link
Author

Danack commented May 15, 2014

I'm using your branch https://github.com/steverhoades/zf2/commits/hotfix/6255 but as I suspected, I'd grabbed version 3a04, which is just before your last commit.

Testing your latest commit does indeed fix this issue.

Ocramius added a commit that referenced this issue Aug 6, 2014
@Ocramius
Copy link
Member

Ocramius commented Aug 6, 2014

Handled in #6286

@Ocramius Ocramius added this to the 2.3.2 milestone Aug 6, 2014
@Ocramius Ocramius self-assigned this Aug 6, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants