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.

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

ClassMapGenerator: fix two bugs (long heredocs + markers in text) #10050

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Composer/Autoload/ClassMapGenerator.php
Expand Up @@ -246,7 +246,7 @@ private static function findClasses($path)
}

// strip heredocs/nowdocs
$contents = preg_replace('{<<<[ \t]*([\'"]?)(\w+)\\1(?:\r\n|\n|\r)(?:.*?)(?:\r\n|\n|\r)(?:\s*)\\2(?=\s+|[;,.)])}s', 'null', $contents);
$contents = preg_replace('{<<<[ \t]*([\'"]?)(\w+)\\1(?:\r\n|\n|\r)(?:.*(?=[\r\n]+[ \t]*\\2))[\r\n]+[ \t]*\\2(?=\s*[;,.)])}s', 'null', $contents);
// strip strings
$contents = preg_replace('{"[^"\\\\]*+(\\\\.[^"\\\\]*+)*+"|\'[^\'\\\\]*+(\\\\.[^\'\\\\]*+)*+\'}s', 'null', $contents);
// strip leading non-php code if needed
Expand Down
19 changes: 19 additions & 0 deletions tests/Composer/Test/Autoload/ClassMapGeneratorTest.php
Expand Up @@ -244,6 +244,25 @@ public function testDump()
$fs->removeDirectory($tempDir);
}

public function testCreateMapDoesNotHitRegexBacktraceLimit()
{
$expected = array(
'Foo\\StripNoise' => realpath(__DIR__) . '/Fixtures/pcrebacktracelimit/StripNoise.php',
'Foo\\VeryLongHeredoc' => realpath(__DIR__) . '/Fixtures/pcrebacktracelimit/VeryLongHeredoc.php',
'Foo\\ClassAfterLongHereDoc' => realpath(__DIR__) . '/Fixtures/pcrebacktracelimit/VeryLongHeredoc.php',
'Foo\\VeryLongPHP73Heredoc' => realpath(__DIR__) . '/Fixtures/pcrebacktracelimit/VeryLongPHP73Heredoc.php',
'Foo\\VeryLongPHP73Nowdoc' => realpath(__DIR__) . '/Fixtures/pcrebacktracelimit/VeryLongPHP73Nowdoc.php',
'Foo\\ClassAfterLongNowDoc' => realpath(__DIR__) . '/Fixtures/pcrebacktracelimit/VeryLongPHP73Nowdoc.php',
'Foo\\VeryLongNowdoc' => realpath(__DIR__) . '/Fixtures/pcrebacktracelimit/VeryLongNowdoc.php',
);

ini_set('pcre.backtrack_limit', '30000');
$result = ClassMapGenerator::createMap(__DIR__ . '/Fixtures/pcrebacktracelimit');
ini_restore('pcre.backtrack_limit');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use a try/finally, so in case of exceptions the value gets properly restored?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this retrieve the previous value and re-set it instead of restoring the initial value, which might not be the expected one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@staabm Good question. I'd done it like this to ensure that, even if the assertion fails, restore will still happen, but yes, in case of exceptions it may not.

I'm not a fan of using try/catch in test methods. Moving it to the tearDown() would be more appropriate IMO, or even moving the method to its own test class and then having the ini changes in setUp() and tearDown().

What do you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this retrieve the previous value and re-set it instead of restoring the initial value, which might not be the expected one?

Considered that too (actually started with that when I was writing the test), but couldn't find any other tests doing anything with ini_set(), let alone with this configuration setting, so I figured I may as well use the PHP native restore option. Happy to change that though.

Copy link
Member

Choose a reason for hiding this comment

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

IMO it's fine as is. Low chances of problems, and tests only, so not really worth a debate :)


$this->assertEqualsNormalized($expected, $result);
}

protected function assertEqualsNormalized($expected, $actual, $message = '')
{
foreach ($expected as $ns => $path) {
Expand Down
Expand Up @@ -17,7 +17,12 @@ class FailHeredocBasic
class FailHeredocWhitespace
{
}
WHITESPACE . <<<"DOUBLEQUOTES"
WHITESPACE . <<< MARKERINTEXT
In PHP < 7.3, the docblock marker could occur in the text as long as it did not occur at the very start of the line.
But, what are you blind McFly, it's there. How else do you explain that wreck out there? Doc, Doc. Oh, no. You're alive. Bullet proof vest, how did you know, I never got a chance to tell you. About all that talk about screwing up future events, the space time continuum. Okay, alright, I'll prove it to you.
MARKERINTEXT
Look at my driver's license, expires 1987. Look at my birthday, for crying out load I haven't even been born yet. And, look at this picture, my brother, my sister, and me. Look at the sweatshirt, Doc, class of 1984. Why do you keep following me around? Hey beat it, spook, this don't concern you.
MARKERINTEXT . <<<"DOUBLEQUOTES"
class FailHeredocDoubleQuotes
{
}
Expand Down
@@ -0,0 +1,87 @@
<?php

namespace Foo;

/**
* class StripNoise { }
*/
class StripNoise
{
public function test_heredoc()
{
return <<<HEREDOC
class FailHeredocBasic
{
}
HEREDOC . <<< WHITESPACE
class FailHeredocWhitespace
{
}
WHITESPACE . <<<"DOUBLEQUOTES"
class FailHeredocDoubleQuotes
{
}
DOUBLEQUOTES . <<< "DOUBLEQUOTESTABBED"
class FailHeredocDoubleQuotesTabbed
{
}
DOUBLEQUOTESTABBED . <<<HEREDOCPHP73
class FailHeredocPHP73
{
}
HEREDOCPHP73;
}

public function test_nowdoc()
{
return <<<'NOWDOC'
class FailNowdocBasic
{
}
NOWDOC . <<< 'WHITESPACE'
class FailNowdocWhitespace
{
}
WHITESPACE . <<< 'NOWDOCTABBED'
class FailNowdocTabbed
{
}
NOWDOCTABBED . <<<'NOWDOCPHP73'
class FailNowdocPHP73
{
}
NOWDOCPHP73;
}

public function test_followed_by_parentheses()
{
return array(<<<PARENTHESES
class FailParentheses
{
}
PARENTHESES);
}

public function test_followed_by_comma()
{
return array(1, 2, <<<COMMA
class FailComma
{
}
COMMA, 3, 4);
}

public function test_followed_by_period()
{
return <<<PERIOD
class FailPeriod
{
}
PERIOD.'?>';
}

public function test_simple_string()
{
return 'class FailSimpleString {}';
}
}

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.