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

fix a problem with preg_replace parameters in the query #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

bookin
Copy link

@bookin bookin commented Feb 27, 2019

If one of parameters have a ? symbol in the text, preg_replace can work incorrectly, because just looking for any ? symbol

If one of parameters have a `?` symbol in the text, `preg_replace` can work incorrectly, because just looking for any `?` symbol
@@ -292,8 +292,8 @@ public function execute($params = null) : bool
} else {
foreach (array_keys($this->values) as $key) {
$sql = preg_replace(
'/(' . (is_int($key) ? '\?' : ':' . $key) . ')/i',
$this->getTypedParam($key),
'/(' . (is_int($key) ? '\?([,\)])' : ':' . $key) . ')/i',
Copy link
Member

Choose a reason for hiding this comment

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

what do you think about this expr: \?([,\)]|\s,|\s\)) for resolve next case with whitespace character: '? , ? ) ?

Copy link
Author

Choose a reason for hiding this comment

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

I haven't seen this situation, but yes, we can add just spaces

Suggested change
'/(' . (is_int($key) ? '\?([,\)])' : ':' . $key) . ')/i',
'/(' . (is_int($key) ? '\?(\s?[,\)])' : ':' . $key) . ')/i',

Copy link
Author

Choose a reason for hiding this comment

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

Anyway, it don't help if we will have ?, in the text :)

@bookin
Copy link
Author

bookin commented Feb 27, 2019

Seems you have a solution in the $hasZeroIndex part, sorry, I use an older version and can not test code to change all to use one method to both ways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants