-
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
Readd Nested filter #1020
Readd Nested filter #1020
Conversation
@kukulich Because of the tests that are broken I actually prefer to keep this PR currently out of 3.0.0. It is easy to add a feature later but removing and changing BC is worse I think. As we plan to ship 3.1 as soon as this is fixed, I don't think it should be a big issue. |
@ruflin I don't know if I understand you correctly. Do you plan to release 3.0.0 without |
@kukulich Yes, I just did. The reason is that it is very easy to add code after a release but hard to remove it. The tests for nested didn't pass because some results were empty instead of results. So I prefer that people are aware of the issue instead of just using it and not getting results. I hope to ship either 3.0.1 or 3.1.0 in the next days with a fix. |
$filter = new Nested(); | ||
$this->assertEquals(array('nested' => array()), $filter->toArray()); | ||
$query = new Terms(); | ||
$query->setTerms('hobby', array('guitar')); |
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.
@ruflin From the documentation "Note that any fields referenced inside the query must use the complete path (fully qualified)."
This should work $query->setTerms('hobbies.hobby', array('guitar'));
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.
@kukulich that is great. Seems to work.
* Fix tests * Improve docker files by removing --prefer-source as it is an automatic fallback now
Do it, I will merge it to my branch, I think it is not a problem for me. |
@ewgRa Perfect. Lets wait for it to get green. |
@kukulich Merged. Thanks a lot for the fast feedback for the fix. |
You're welcome. I'm happy that |
See #1001