-
Notifications
You must be signed in to change notification settings - Fork 736
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
slash / is escaped with two slashes instead of one, parse failure #660
Comments
Here is the issues with pull request when we changed it: #589 |
@maxgalbu Can you take a look at this to see why this breaks again (or not)? |
IMO the best way would be to add an actual test of escapeTerm in a query with an elasticsearch daemon. In the tests folder I only see a test for the output of the function, but I could be wrong. If that's not the case, can you point me to a SomethingTest.php file where I can add an actual query? I can try to edit it and make a pull request |
Unfortunately it seems that you are right. Can you create a test with an actual query and open a pull request? |
That's what I was saying man :) I'll see what test file I can edit |
👍 |
The travis-ci test confirms the bug on php 5.3 but not on php 5.4... WTF? |
This was the first thing that came to my mind but I think it is not related. http://php.net/manual/en/security.magicquotes.php PHP 5.3 is EOL ;-) |
magicquotes should not be related, it shouldn't escape chars on a literal string. Removing the double quote makes it work with php 5.3 but not > 5.3: https://travis-ci.org/maxgalbu/Elastica/builds/32806687 So, would you accept a patch that makes Elastica work with php 5.3? It would be a single if that escapes with a single or double |
As I'm quite sure still a lot of users will keep using PHP 5.3 I will accept a patch as long as it doesn't break PHP 5.4 and following. |
I faced a similar issue and here is what I found:
This 2 things combined produce unexpected results, especially on different PHP versions:
Here you can see this problem in action: http://3v4l.org/YYd4F I propose to fix it by: If you apply the above fixes, it will work the same in both PHP 5.3 and above: http://3v4l.org/5gE31 P.S. We're still using Elastica 1.1, where the whole escaping is complicated even more by this weird maniuplation |
@kostiklv Thank your for the detailed analysis. I agree with all points you mentioned above. Can you open a pull request based on it? |
Good analysis, well done :) If you open a pull request, can you copy my test that i added in my fork: ...to make sure the escape is tested against elasticsearch? Or if you want i can add your changes to my fork and do a pull request myself. |
Ok, if you agree on the proposed changes, I'll try to create a PR later today or tomorrow. Thanks! |
Please coordinate with each other which is going to do which part. In any case, I'm looking forward to the pull request. |
@kostiklv good job |
I'm using:
My fix is to escape with one \ instead of \
I had a look at previous issues but i can't see why it was escaped two times.
here's the log
The text was updated successfully, but these errors were encountered: