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

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

Merged
merged 5 commits into from
Dec 20, 2014

Conversation

Ocramius
Copy link
Member

See #44

…ows a longer sequence to be parsed

Note that this doesn't solve the stack overflow issue with `preg_split()`, but just increases the size of the string that could be matched.
@@ -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!

@Ocramius Ocramius added the bug label Dec 19, 2014
@Ocramius Ocramius added this to the v1.2.3 milestone Dec 19, 2014
@Ocramius Ocramius self-assigned this Dec 19, 2014
@nidup
Copy link

nidup commented Dec 19, 2014

Hello, FYI, we encountered the BC Break in our existing code base with,

``* @Assert\Choice(choices={"""", "'"}, message="The value must be one of "" or '")`

@Ocramius
Copy link
Member Author

@nidup this issue fixes exactly that

@nidup
Copy link

nidup commented Dec 19, 2014

@Ocramius perfect, thank you ! for now we tag a patch in our project to force the 1.2.1 in composer.json

@Ocramius
Copy link
Member Author

@nidup I'll likely tag again later today.

Ocramius added a commit that referenced this pull request Dec 20, 2014
@Ocramius Ocramius merged commit eeda578 into doctrine:master Dec 20, 2014
@Ocramius Ocramius deleted the hotfix/#44-match-escaped-quotes branch December 20, 2014 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants