Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hotfix: match escaped quotes (revert #44) #50

Merged
merged 5 commits into from
Dec 20, 2014
Merged

Hotfix: match escaped quotes (revert #44) #50

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a62760c
#44 - test case to verify correct escaping of quotes in lexed strings
Ocramius Dec 18, 2014
f177d96
#44 - verifying correct paring of annotation with escaped quotes
Ocramius Dec 18, 2014
918ba14
#44 - reverting #44 patch by using a matcher that allows a longer seq…
Ocramius Dec 19, 2014
028f4bf
#44 - increasing tested string size to check for more possible stack …
Ocramius Dec 20, 2014
ceaf9ad
#44 - using a possessive quantifier to reduce likeliness of `preg_spl…
Ocramius Dec 20, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/Doctrine/Common/Annotations/DocLexer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ protected function getCatchablePatterns()
return array(
'[a-z_\\\][a-z0-9_\:\\\]*[a-z_][a-z0-9_]*',
'(?:[+-]?[0-9]+(?:[\.][0-9]+)*)(?:[eE][+-]?[0-9]+)?',
'"(?:[^"])*"',
'"(?:""|[^"])*"',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern doesn't prevent PHP from killing itself because of a preg_split() stack overflow, but at least makes it possible to pass in a string longer than 10240 chars, which is enough for our use-cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is a normal problem for anything using a (?: pattern combined with |

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ocramius I think this regex could actually use a possessive quantifier, which would limit the stack overflow even more by forbidding to backtrack. In this case, the possessive regex should match the same cases than the non-possessive one (to be checked by the testsuite though): '"(?:""|[^"])*+"',

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works indeed, thanks for the suggestion!

);
}

Expand Down
49 changes: 49 additions & 0 deletions tests/Doctrine/Tests/Common/Annotations/DocLexerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,4 +165,53 @@ public function testWithinDoubleQuotesVeryVeryLongStringWillNotOverflowPregSplit

$this->assertInternalType('array', $lexer->glimpse());
}

/**
* @group 44
*/
public function testRecognizesDoubleQuotesEscapeSequence()
{
$lexer = new DocLexer();
$docblock = '@Foo("""' . "\n" . '""")';

$tokens = array (
array(
'value' => '@',
'position' => 0,
'type' => DocLexer::T_AT,
),
array(
'value' => 'Foo',
'position' => 1,
'type' => DocLexer::T_IDENTIFIER,
),
array(
'value' => '(',
'position' => 4,
'type' => DocLexer::T_OPEN_PARENTHESIS,
),
array(
'value' => "\"\n\"",
'position' => 5,
'type' => DocLexer::T_STRING,
),
array(
'value' => ')',
'position' => 12,
'type' => DocLexer::T_CLOSE_PARENTHESIS,
),
);

$lexer->setInput($docblock);

foreach ($tokens as $expected) {
$lexer->moveNext();
$lookahead = $lexer->lookahead;
$this->assertEquals($expected['value'], $lookahead['value']);
$this->assertEquals($expected['type'], $lookahead['type']);
$this->assertEquals($expected['position'], $lookahead['position']);
}

$this->assertFalse($lexer->moveNext());
}
}
13 changes: 13 additions & 0 deletions tests/Doctrine/Tests/Common/Annotations/DocParserTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1282,6 +1282,19 @@ public function testKeyHasNumber()
$this->assertEquals(1, count($annots));
$this->assertEquals(array('foo' => 'test', 'bar2' => 'test'), $annots[0]->settings);
}

/**
* @group 44
*/
public function testSupportsEscapedQuotedValues()
{
$result = $this->createTestParser()->parse('@Doctrine\Tests\Common\Annotations\Name(foo="""bar""")');

$this->assertCount(1, $result);

$this->assertTrue($result[0] instanceof Name);
$this->assertEquals('"bar"', $result[0]->foo);
}
}

/** @Annotation */
Expand Down