Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
From Search API BoF: Add some 'tag' to processors; and use to for list display and trigger order.
Comment | File | Size | Author |
---|---|---|---|
#35 | 2349435-35--processing_stages.patch | 76.11 KB | drunken monkey |
Comments
Comment #1
drunken monkeyHow far are you on this? Do you still have time to work on it? Otherwise, please post a patch (or create a branch) with your current progress!
Comment #3
ekes CreditAttribution: ekes commentedBranch pushed: 2349435-seperate-processors-by-stage
I can make some more time to work on it; but it certainly needs feedback.
The patch:
Changes Index::getProcessors to return per-stage.
Enables different weighting and status for processors per stage.
Changed UI to seperate processors into different stages.
I'm pretty sure the way the stages constants are defined and made available (processor interface and base class) can be better. The Index::getProcessors() feels even more over-complex than it was before, I was going to break it up into a 'get all' and a 'get stage' function.
The filters form: With the individual status per stage the UI has the checkboxes repeated per stage, it's quite a lot. The original idea was to do away with the checkboxes, but with just drag: (a) you can't define a default order, the user actually has to make the decision up-front where to drag the processor too, which is not desirable, as the default order is usually good (and it matters the order); (b) you can't, or rather it would be difficult to automatically enable the processor in all stages by default (which is usually what people will want).
Test failing:-
IntegrationTest - one still needs just correcting because it's working in the UI; another, the settings of the fields that the processor works on I've yet to fathom, it's saved, and loaded as defaults on the form in the subform, but the fields are then not checked when the form is displayed.
ContentAccessTest - needs fixing
SearchApiDbTest - search_api_language field is enabled by default but not expected by the test. I think the field should be there though.
Comment #4
drunken monkeyGreat job so far, thanks! You are right, the
getProcessors()
method is a mess, that should definitely be refactored. But that, as well as UI tweaking (e.g., I'd love to see the three stages lined up horicontally, to a) better visualize the workflow and b) save space – but that would probably also need some responsive-design magic, so nothing we have to or should hurry) can be done in follow-up issues, I think.This, in any case, is how I'd imagine that form:
Apart from that:
add_fields
. Only ever makes sense in combination withpreprocess_index
, of course, but would allow for moving the fields-only processors. However, re-ordering for that stage would of course make even less sense than for others, so we shouldn't make that a displayed stage (just maybe some indication to admins which processors add fields).\Drupal\search_api\Entity\Index::preSave()
to this change so the "Language" processor is always enabled again.@ Nick: It would be great to get your feedback on this.
Comment #8
ekes CreditAttribution: ekes commentedOk.
That's tidied up the form; moved back to single status value; and broken that ugly getProcessors() into two functions getProcessors() and getProcessorsByStage() which makes it much clearer what (should) be going on.
Comment #9
ekes CreditAttribution: ekes commentedSetting to Needs Review for feedback; also on the possibilities in #4.
Comment #11
drunken monkeyThanks for the overhaul, this looks already very good now!
Made a few changes in the branch and added two
@todo
s, but apart from those and the two failing tests this seems to me to be ready to go. Thanks!Comment #12
ekes CreditAttribution: ekes commentedOK fixed those failing tests:
http://travis-ci.org/nickveenhof/search-api-sandbox/builds/40387169
Comment #13
drunken monkeyGreat, thanks!
Will you also work on the remaining
@todo
s in the branch? (Regarding the one: when moving the stages information to a method, I do think we should translate them, yes. Which probably means it shouldn't be static, so it can use proper DI.)Otherwise, please unassign and we'll see who wants to pick it up.
Comment #14
Nick_vhI don't recall but now it is confusing. The tab mentions Filters but the whole page mentions processors. I think Filters is much more clear to users what it means. Can I rename all the wording on the page to Filters but in the code we can keep calling it Processors?
Doing this makes sense, the UI should be friendly and not map 1:1 to the underlying code. Having it called Filters is perfectly valid, even in the case where we move some processors that add new fields to the Fields tab. In code, they are all processors but in the UI they appear in different places.
Comment #15
Nick_vhAlso saving processors is not working at the moment.
Comment #17
drunken monkeyNo, we voted in the BoF and agreed on "processors" – remember? But of course we should rename the tab now: see #2348185: Clean up terminology.
Also, this has nothing to do with this issue. Please revert your commit and start a new issue if you want to propose a new, expanded description.
Comment #18
Nick_vhMy memory is really bad - sorry for that. Reverting!
Comment #19
drunken monkeyNo problem.
Still waiting for ekes' answer to #13.
Comment #20
ekes CreditAttribution: ekes commentedI'll take my name off it for now. If I succeed in freeing up some time I'll pop it back.
Comment #21
drunken monkeyOK, thanks for the response.
I'm currently on finalizing the move back to the proper repository, so I won't work on this for now. Maybe someone else wants to take this up? Otherwise, let's get back to it later.
Comment #22
drunken monkeyMoving this issue to the proper project. Also re-merged the branch – I guess we should still develop that in the sandbox, so ekes (or whoever) can commit to it.
Comment #24
drunken monkeyI'll finally tackle this before there is more rebasing trouble …
I'll also try to fix the other current problems with the "Filters" tab while I'm at it – there are enough at the moment, after all.
Comment #25
drunken monkeyUnfortunately, I didn't have enough time to finish this yet and, as per #1188562-53: [meta] Important project announcements, I'll be on vacation the next three weeks.
I pushed my latest code to the sandbox branch, if someone has the time to pick it up and finish it that would be great. But since there are currently some weird issues I don't really understand myself, I can't really expect it.
Comment #26
drunken monkeyAt it again.
Comment #27
drunken monkeyAnd, finally done! That was really quite some work there …
Attached is a patch, and the
sandbox/2349435-seperate-processors-by-stage
also contains the latest code. It would be great if someone else could take a look and maybe give it a try – however, since I didn't want to complicate things even more by also chasing HEAD, the patch still pertains to the D8 Core version 7c6a23f, from December 12. But everything seems to work fine for me, and all tests run green as well.So, if no-one objects (or says they still want to test) I'll commit this in a few days.
Comment #28
Nick_vhI'd love to test it. Please hold till I can take a look :) Ekes, it would be also be great if you could do the same?
Comment #30
BerdirOh, sorry, I didn't see you've been working on this recently in here. Contrary to your worries that it would be a pain to merge, this was trivial, or at least it seems so, did not run any tests yet.
I only had merge conflicts in Index.php and IndexFiltersForm.php. In both cases, I simply went with the code from this branch. I find it a bit scary to carry around the processors in $form_state and let them get serialized, I kind of preferred my approach to only use $form_state temporarily between validate and submit, but let's see how this goes.
Also pushed the merge (did not rebase) to your sandbox branch again.
Comment #32
BerdirOh, you renamed the key, this should be better.
Comment #34
drunken monkeyFinally got the time to rebase this and fix things up. The attached version passes tests for me locally, so I hope we can finally commit this.
(I also updated the branch, in case you want to look at interdiffs.)
Comment #35
drunken monkeyForgot that that was still a WIP regarding the "Content access" processor. This is the finished patch.
Comment #36
drunken monkeyNice to see it pass.
Does anyone still want to take a look or should I commit?
Comment #38
drunken monkeyOK, finally committed!
Thanks a lot for your work on this, ekes, and also to Sascha for helping finish it!
Comment #39
ekes CreditAttribution: ekes commentedAdded follow up #2472419: Use core CSS for admin UI filter/processor stages columns