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

Bugfix: Fix unescaped dash in character group in regex #14223

Merged
merged 1 commit into from Jul 2, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -286,9 +286,10 @@ protected function getClassesFromDir($path, $hostMode = false)
} elseif (substr($file, -4) == '.php') {
$content = file_get_contents($rootDir . $path . $file);
$namespacePattern = '[\\a-z0-9_]*[\\]';
$pattern = '#\W((abstract\s+)?class|interface)\s+(?P<classname>' . basename($file, '.php') . '(?:Core)?)'
. '(?:\s+extends\s+' . $namespacePattern . '[a-z][a-z0-9_]*)?(?:\s+implements\s+' . $namespacePattern . '[a-z][\\a-z0-9_]*(?:\s*,\s*' . $namespacePattern . '[a-z][\\a-z0-9_]*)*)?\s*\{#i';
$namePattern = '[a-z_\x7f-\xff][a-z0-9_\x7f-\xff]*';
This conversation was marked as resolved by mvorisek

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jun 26, 2019

Contributor

Why adding \x7f and \xff, I don't find any reason to add it?

This comment has been minimized.

Copy link
@mvorisek

mvorisek Jun 26, 2019

Author Contributor

https://stackoverflow.com/questions/6362241/php-variable-function-class-names-using-special-characters#6362268 To match all valid names no matter if Unicode names are currently used or not.

$nameWithNsPattern = '(?:\\\\?(?:' . $namePattern . '\\\\)*' . $namePattern . ')';
This conversation was marked as resolved by mvorisek

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jun 26, 2019

Contributor

Shouldn't be this?

Suggested change
$nameWithNsPattern = '(?:\\\\?(?:' . $namePattern . '\\\\)*' . $namePattern . ')';
$nameWithNsPattern = '(?:\\\\)?(?:' . $namePattern . '\\\\)*' . $namePattern;

This comment has been minimized.

Copy link
@mvorisek

mvorisek Jun 26, 2019

Author Contributor

No, it shouldn't. \\\\ is properly escaped \ char and the (?: + ) outside parenthesis are there for safe use with quantifiers (imagine ab later used as ab? instead of (?:ab)?).

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jun 26, 2019

Contributor

?: only mean Non-capturing group.
And ab? is different than (?:ab)?).
But here I understand that \\\\ is only one character, and I think my suggestion is easier to understand, (especially for people not familiar with regex ^^)

This comment has been minimized.

Copy link
@mvorisek

mvorisek Jun 26, 2019

Author Contributor

?: only mean Non-capturing group.
But here I understand that \\\\ is only one character, and I think my suggestion is easier to understand, (especially for people not familiar with regex ^^)

Yes, I know. If you want, wrap the first \\\\ inside another (?: + ), but it is not needed, as it is only one character.

And ab? is different than (?:ab)?).

Yes, that is the example of the difference, beaware that the second/last ) is part of the text :)

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jun 26, 2019

Contributor

Yes, I know. If you want, wrap the first \\ inside another (?: + ), but it is not needed, as it is only one character.

Of course it's not needed, but I think many people are not familiar with regex and if you ask to many people they will answer you it's two \ :D But indeed, let's merge your PR 👍

$pattern = '~(?<!\w)((abstract\s+)?class|interface)\s+(?P<classname>' . basename($file, '.php') . '(?:Core)?)'
. '(?:\s+extends\s+' . $nameWithNsPattern . ')?(?:\s+implements\s+' . $nameWithNsPattern . '(?:\s*,\s*' . $nameWithNsPattern . ')*)?\s*\{~i';
//DONT LOAD CLASS WITH NAMESPACE - PSR4 autoloaded from composer
$usesNamespace = false;
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.