-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
quote characters used directly in a query #32
Conversation
@@ -385,6 +385,7 @@ protected function findBy($entityClass, $searchQuery, array $searchableFields, $ | |||
; | |||
|
|||
foreach ($searchableFields as $name => $metadata) { | |||
$searchQuery = $this->getDoctrine()->getConnection()->quote($searchQuery); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two comments about this change:
- This line should be moved outside the
foreach
loop to not repeat it for each field. - I don't think that calling the
quote()
method is the right thing to do here. I've made some tests:
What the user typed in the search box | The result of quote() |
The expected result |
---|---|---|
foo' |
'foo\'' |
foo\' |
%foo% |
'%foo%' |
\%foo\% |
Keep in mind that the query is wrapped like this to do the full-text search: '%'.$searchQuery.'%'
This means that it cannot be quoted, we just have to escape the special chars in the context of the LIKE
query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, you're right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does adding $searchQuery = trim($searchQuery, "'");
seems a bad practice ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's not a good idea because we then relied on implementation specific behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to issue #26 the best solution might seem this one:
$wildcards = $this->getDoctrine()->getConnection()->getDatabasePlatform()->getWildcards();
if (!count($wildcards)) {
$wildcards = array('%','_');
}
$wildcards = implode('', $wildcards);
$searchQuery = addcslashes($searchQuery,$wildcards);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the code.
Thank you Christian! I appreciate the time you took to solve this PR and do all the asked code changes. |
This fixes #26.