8000 [Yaml] save preg_match() calls when possible by xabbuh · Pull Request #28106 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Yaml] save preg_match() calls when possible #28106

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

Merged
merged 1 commit into from
Aug 2, 2018
Merged

[Yaml] save preg_match() calls when possible #28106

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 10 additions & 8 deletions src/Symfony/Component/Yaml/Parser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,13 +161,13 @@ private function doParse(string $value, int $flags)
Inline::initialize($flags, $this->getRealCurrentLineNb(), $this->filename);

$isRef = $mergeNode = false;
if (self::preg_match('#^\-((?P<leadspaces>\s+)(?P<value>.+))?$#u', rtrim($this->currentLine), $values)) {
if ('-' === $this->currentLine[0] && self::preg_match('#^\-((?P<leadspaces>\s+)(?P<value>.+))?$#u', rtrim($this->currentLine), $values)) {
if ($context && 'mapping' == $context) {
throw new ParseException('You cannot define a sequence item when in a mapping', $this->getRealCurrentLineNb() + 1, $this->currentLine, $this->filename);
}
$context = 'sequence';

if (isset($values['value']) && self::preg_match('#^&(?P<ref>[^ ]+) *(?P<value>.*)#u', $values['value'], $matches)) {
if (isset($values['value']) && '&' === $values['value'][0] && self::preg_match('#^&(?P<ref>[^ ]+) *(?P<value>.*)#u', $values['value'], $matches)) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to have $values['value'] being the empty string here or no ? If yes, we have a bug.

Copy link
Member Author
@xabbuh xabbuh Aug 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

No, the regular expression that has to match to reach this part of the code is #^\-((?P<leadspaces>\s+)(?P<value>.+))?$#u. So if the value element is set, it must have at least on character.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, the value group can indeed not be empty when it is matched, so things are fine.

$isRef = $matches['ref'];
$values['value'] = $matches['value'];
}
Expand Down Expand Up @@ -229,7 +229,7 @@ private function doParse(string $value, int $flags)
$key = (string) $key;
}

if ('<<' === $key && (!isset($values['value']) || !self::preg_match('#^&(?P<ref>[^ ]+)#u', $values['value'], $refMatches))) {
if ('<<' === $key && (!isset($values['value']) || '&' !== $values['value'][0] || !self::preg_match('#^&(?P<ref>[^ ]+)#u', $values['value'], $refMatches))) {
$mergeNode = true;
$allowOverwrite = true;
if (isset($values['value'][0]) && '*' === $values['value'][0]) {
Expand Down Expand Up @@ -286,7 +286,7 @@ private function doParse(string $value, int $flags)
$data += $parsed; // array union
}
}
} elseif ('<<' !== $key && isset($values['value']) && self::preg_match('#^&(?P<ref>[^ ]++) *+(?P<value>.*)#u', $values['value'], $matches)) {
} elseif ('<<' !== $key && isset($values['value']) && '&' === $values['value'][0] && self::preg_match('#^&(?P<ref>[^ ]++) *+(?P<value>.*)#u', $values['value'], $matches)) {
$isRef = $matches['ref'];
$values['value'] = $matches['value'];
}
Expand Down Expand Up @@ -671,7 +671,7 @@ private function parseValue(string $value, int $flags, string $context)
return $this->refs[$value];
}

if (self::preg_match('/^(?:'.self::TAG_PATTERN.' +)?'.self::BLOCK_SCALAR_HEADER_PATTERN.'$/', $value, $matches)) {
if (\in_array($value[0], array('!', '|', '>'), true) && self::preg_match('/^(?:'.self::TAG_PATTERN.' +)?'.self::BLOCK_SCALAR_HEADER_PATTERN.'$/', $value, $matches)) {
$modifiers = isset($matches['modifiers']) ? $matches['modifiers'] : '';

$data = $this->parseBlockScalar($matches['separator'], preg_replace('#\d+#', '', $modifiers), (int) abs($modifiers));
Expand Down Expand Up @@ -773,8 +773,10 @@ private function parseBlockScalar(string $style, string $chomping = '', int $ind

// determine indentation if not specified
if (0 === $indentation) {
if (self::preg_match('/^ +/', $this->currentLine, $matches)) {
$indentation = \strlen($matches[0]);
$currentLineLength = \strlen($this->currentLine);

for ($i = 0; $i < $currentLineLength && ' ' === $this->currentLine[$i]; ++$i) {
++$indentation;
}
}

Expand Down Expand Up @@ -1009,7 +1011,7 @@ private function isStringUnIndentedCollectionItem(): bool
*/
private function isBlockScalarHeader(): bool
{
return (bool) self::preg_match('~'.self::BLOCK_SCALAR_HEADER_PATTERN.'$~', $this->currentLine);
return '' !== $this->currentLine && (bool) self::preg_match('~'.self::BLOCK_SCALAR_HEADER_PATTERN.'$~', $this->currentLine);
Copy link
Member

Choose a reason for hiding this comment

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

what about adding && \in_array($this->currentLine[0], array('|', '>'), true) && here too ?

Copy link
Member Author
@xabbuh xabbuh Aug 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I thought about that too, but the regular expression should also match lines like foo: | or foo: >2, shouldn't it?

Copy link
Member

Choose a reason for hiding this comment

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

hmm indeed. This regex is not anchored at the start. I missed that

Copy link
Member Author

Choose a reason for hiding this comment

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

I just remembered that I indeed tried to add that check and was wondering now why tests didn't fail then. Turns out this method is totally useless with the current code (see #28187).

}

/**
Expand Down
0