In #3117171: Bulk Form field for SAPI, we've introduced a the bulk form SAPI field. As we've used SearchApiFieldTrait, this trait adds 4 default options (link_to_item, use_highlighting, multi_type, multi_separator), the field will receive these options, even, I think, it doesn't need them. As the options schema of search_api_bulk_form plugin is the same as for views.field.bulk_form, we're getting:
views.view.*:display.default.display_options.fields.search_api_bulk_form.link_to_item failed with: missing schema
on a config sanity test that we're running.
I see 2 options:
views.field.search_api_bulk_form schema.SearchApiFieldTrait only methods regarding these field plugin options: ::defineOptions(), ::buildOptionsForm(), etc and all the logic that depends on these options (not an easy job!).SearchApiFieldTrait. In this way we are not breaking BC.SearchApiBulkForm use the new trait.The 1st approach is effective, the 2nd is too complex. I will provide a fix for the 1st, while discussing all options.
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 3195199-8--views_bulk_options_schema.patch | 471 bytes | drunken monkey |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !308changes, plain diff MR !6
Comments
Comment #3
claudiu.cristeaCreated MR on option 1.
Comment #4
pfrenssenThis is looking good. I don't see how this patch can make that test fail so I restarted it. Possibly a random failure. If test comes back green this is RTBC.
Comment #5
drunken monkeyThanks a lot for reporting this, good catch!
The Composer test fails are apparently due to #3190024: Problem with test dependencies when testing issue forks. However, I don’t have an explanation for the others, either.
In any case, now that we are aware of the problem, I’d prefer fixing it correctly, not just with a band-aid – that is, not include options we don’t use in the field config.
The attached patch tries to implement that, please test/review!
Comment #6
drunken monkeyAny feedback on my patch?
Comment #7
claudiu.cristeaSorry for late feedback. Just now, I've tested #5. Unfortunately, I'm still getting:
With the code from https://git.drupalcode.org/project/search_api/-/merge_requests/6, the tests is passing. We're running this test https://github.com/ec-europa/joinup-dev/blob/develop/web/modules/custom/... to catch missing or erroneous schemas in exported config.
However, due to some infrastructure issues, we're still using Search API 1.13.0. I don't know if this influences the outcome of the test.
Comment #8
drunken monkeySeems like you just need to adapt the export of your
manage_collection_solutionsaccordingly? I.e., remove those settings keys (link_to_itemanduse_highlighting) from thesearch_api_bulk_formfield.We could of course also add an update hook to do that for you (and others), but I’m not sure that’s worthwhile.
Patch was also outdated, attaching a re-roll.
Comment #9
claudiu.cristeaOK, will assign to do a review on a vanilla install.
Comment #10
drunken monkeyAny update? Otherwise, I’ll just commit.
Comment #12
drunken monkeyCommitted.
Thanks again, everyone!
Comment #13
drunken monkeyComment #15
claudiu.cristeaI was only today able to test the patch from #8, as only now I've updated to Search API 1.21. Unfortunately, #8 doesn't fix. I'm still getting the error from issue summary and #5.
Comment #16
claudiu.cristeaOpened #3250991: Issue #3195199 is not fixed as I cannot re-open this one.
Comment #17
drunken monkeySorry it took me a while to read the new issue. In the future, please feel free to ping me on Slack in such cases. (Or even by mail/contact form, I guess, if it’s really just “please re-open issue XYZ”.)
Anyways, thanks for reporting back, even if it was a bit late. Does re-saving the view help? Or when you manually remove those keys from the config, and then re-save, do they stay removed? In the latter case, we could add an update function to implement this, even if it’s rather a lot of effort for a small problem (schema errors don’t really do much harm, right?). Otherwise, I think we should be happy with at least keeping saved views correct going forward.
If nothing helps, do you have any theory on why the options still get added? Did we miss something?
Comment #18
herved commentedI just checked this, here is why: https://3v4l.org/J99Af#vnull
Aliasing in traits does not remove or disable the original method, it only gives the method an additional name.
So the committed fix doesn't work.
Maybe what we could do is override defineOptions and buildOptionsForm as empty in the SearchApiBulkForm class.
Comment #20
herved commentedComment #21
herved commentedoops it didn't pass, checking...
Comment #22
herved commentedComment #24
drunken monkey@herved: Looks great, thanks a lot! Also thanks for the explanation – I was (obviously) not aware of that, very good to know.
@claudiu.cristea: Want to give the MR a try to ensure this finally gets resolved?
Comment #25
claudiu.cristeaThank you @herved. Looks good to me and works on my project
Comment #26
claudiu.cristeaComment #27
drunken monkeyGreat to hear, thanks for confirming!
Merged.
Thanks again!