Currently adding the same field more than once will not work. Only the last added search field will actually contain any value.
This is due to the extractFields() methods in src/Utility.php, it uses the property path as unique keys mapped to search fields.
Adding the same field twice is useful if you want to index it as String and as Full Text.
| Comment | File | Size | Author |
|---|---|---|---|
| #40 | search_api-2696131-40.patch | 7.07 KB | jeroen.b |
| #39 | 2696131-39--fix_indexing_of_duplicated_fields.patch | 7.71 KB | drunken monkey |
| #33 | can_t_add_the_same--test-only.patch | 1.36 KB | borisson_ |
Comments
Comment #2
jeroen.b commentedHere's a fix.
Comment #5
jeroen.b commentedTrying to fix tests and aggregation.
Comment #7
jeroen.b commentedComment #8
jeroen.b commentedComment #31
jeroen.b commentedComment #32
jeroen.b commentedComment #33
borisson_I think we need a new test case for this as well, but the code looks great so far.
Comment #34
jeroen.b commentedYou are right.
Adding the same field twice works fine though, it's extracting the values that is wrong.
I created a new patch that extends the extract tests.
Comment #35
drunken monkeyThanks a lot for reporting this issue! That is indeed a curious problem, seems I ignored this completely when changing the fields system. Unfortunate that I never checked it works, or included that in the tests.
All the better that you're now adding a test for this, in addition to implementing a fix for this problem – awesome job!
Just two notes on the patch:
Utility::extractFields(), you have to also adapt its documentation.Fieldobjects already have information on their property path (and field ID). If we can't use the convenient method of requiring the property paths as keys, it seems unnecessary to have any kind of extra information in the parameter. Just take an array of field objects (with arbitrary keys) and work on that. (Also makes the test method simpler again – you can just have a numerically-indexed array of field objects.)Otherwise, again: great job!
@ borisson_: As in the other issue – your patch file is broken. It seems to contain control characters for changing the colors, or something like that?
Comment #36
jeroen.b commentedThanks for the feedback, I'll address the issues soon.
I think @borisson_ runs
git diffin a colered shell and then copies the output.He probably wants to use
git diff > [project]-[issue-number]-[comment].patchand then upload the resulting file.Comment #37
borisson_I use do
git diff > *.patch, but I recently started using https://github.com/so-fancy/diff-so-fancy with custom colors, that broke the patches. Luckily that patch was not very useful anyway.Comment #38
jeroen.b commented@drunken monkey, I was just trying to just pass an array of Field[] objects, but then we have no way to store the nested path for the recursive extraction.
Comment #39
drunken monkeyOh, that's right, of course! Sorry about that!
Then maybe like in the attached patch, keeping the code more or less the same, but with arrays of field objects instead of single objects?
(We could also change the code to recognize both cases, but such parameter overloading is rarely a good idea.)
Comment #40
jeroen.b commentedI don't really like that.
Modified the patch.
Comment #43
drunken monkeyHm, yes, that would work, too. If you can make it into a working implementation, it's definitely an alternative to consider.
Please just:
- Also adapt the "Highlight" processor.
- Add an assertion to the test proving this is fixed (like I did).
- Fix the method's documentation.
- Avoid recursing multiple times into the same property (i.e., keep grouping by (direct) property).
@ Joris: Do you have any preference here?
Comment #44
borisson_I think I prefer the way @drunken monkey fixed it in #39 over the solution in #40. I can live with either one though.
Comment #45
drunken monkeyOK, then let's stick with that variant. Thanks for your input!
Anything else, or can I commit #39?
Comment #48
borisson_You can go ahead and commit #39.
Comment #50
drunken monkeyGreat, thanks for reviewing!
Committed.
Thanks again, everyone!