Parameter generator backslash escaping #6023

Merged
merged 5 commits into from Mar 23, 2014

3 participants

@Ocramius
Zend Framework member

Found while trying different default parameters for methods. Currently, Zend\Code is not able to produce valid code when following is being done:

$generator = new Zend\Code\Generator\ParameterGenerator();

$generator->setType('Foo');
$generator->setDefaultValue('\\');
$generator->setName('bar');

var_dump($generator->generator());

Produces

Foo $bar = '\'

Which is obviously going to break method signature if used as a method parameter.

@Ocramius Ocramius added Code bug labels Mar 22, 2014
@EvanDotPro EvanDotPro was assigned by Ocramius Mar 22, 2014
@Ocramius Ocramius added this to the 2.3.1 milestone Mar 22, 2014
@Maks3w Maks3w and 1 other commented on an outdated diff Mar 22, 2014
tests/ZendTest/Code/Generator/ValueGeneratorTest.php
@@ -138,4 +138,20 @@ public function testPropertyDefaultValueCanHandleArrayWithUnsortedKeys()
$this->assertEquals($expectedSource, $valueGenerator->generate());
}
+
+ /**
+ * @group 6023
+ */
+ public function testEscapesBackslash()
+ {
+ $this->assertSame('\\\\', ValueGenerator::escape('\\', false));
+ }
+
+ /**
+ * @group 6023
+ */
+ public function testEscapesQuotes()
+ {
+ $this->assertSame('\\\'', ValueGenerator::escape('\'', false));
@Maks3w
Zend Framework member
Maks3w added a line comment Mar 22, 2014

CS: If the use of single quotes implies escapping then use double quotes

@Maks3w
Zend Framework member
Maks3w added a line comment Mar 22, 2014

You could write a data provider instead write unique tests for each value

@Ocramius
Zend Framework member
Ocramius added a line comment Mar 22, 2014

CS: If the use of single quotes implies escapping then use double quotes

Hmm, always used single quotes in any possible case - is that a new one that I missed?

@Maks3w
Zend Framework member
Maks3w added a line comment Mar 23, 2014

It's a rule of the outdate ZF CS guidelines. It's more readable a string without escaping characters.

Anyway. CS is always subject to readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Maks3w
Zend Framework member

It's needed another test in ParameterGeneratorTest

@EvanDotPro EvanDotPro was unassigned by Maks3w Mar 23, 2014
@Maks3w Maks3w was assigned by Ocramius Mar 23, 2014
@Maks3w Maks3w merged commit 44d47fd into zendframework:master Mar 23, 2014

1 check passed

Details default The Travis CI build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment