When building up the search_admin
form, hook_search_admin
is invoked on selected modules allowing them to add module-specific configuration form elements. Because system_settings_form
does no longer store configuration, implementors of hook_search_admin
need to implement their submit-handler themselves. In order to make adding #submit
-handlers from within hook_search_admin
work, the main form must be merged with the module-specific forms using array_merge_recursive
instead of array_merge
.
In the long run it would probably be feasible to remove hook_search_admin
entirely and instead tell other modules to form_alter
if they need to. I suggest to attack that one in a follow-up issue if needed.
Comments
Comment #1
znerol CreditAttribution: znerol commentedAttach patch and set tag.
Comment #2
BerdirAs discussed, I'm not sure if we should use NestedArray::mergeDeep() (or the drupal_array_merge_deep wrapper function) here.
I think we usually that one for forms as it could otherwise result in a messed up structure. Can you do a quick re-roll then I can RTBC this and we can unblock the issue that depends on this.
Comment #3
znerol CreditAttribution: znerol commentedDone.
Comment #4
znerol CreditAttribution: znerol commentedAttached is a test-only patch (which hopefully will turn red) demonstrating the problem and an updated patch including the tests and also updated hook-documentation in
search.api.php
.We need the fix over here #1831632: Convert node_rank_ $rank variables to use configuration system.
Because form-submission now must be handled by search clients, this issue probably ought to have a change notice.
Comment #6
znerol CreditAttribution: znerol commented#4: 1831802-3-allow-submit-on-hook-search-admin.patch queued for re-testing.
Comment #7
Berdir#4: 1831802-3-allow-submit-on-hook-search-admin.patch queued for re-testing.
Comment #8
BerdirThis looks good to me, assuming the patch still applies (re-tested already).
Comment #9
Berdir#4: 1831802-3-allow-submit-on-hook-search-admin.patch queued for re-testing.
Comment #10
catchOK I don't understand why we have this hook at all, but this looks like it could use backporting to 7.x (although 'minor' for that), so I've committed this patch and opened #1891024: Remove hook_search_admin() in favour of hook_form_alter() to remove it in 8.x.
Comment #11
bnjmnmWorking on 7.x backport.
Comment #12
bnjmnmBackported to 7.x
Comment #13
bnjmnmComment #14
bnjmnmComment #15
jhodgdonThe tests would also need to be backported.
Comment #16
bnjmnmThe test passes without making any changes to the search module, and manually performing the steps works fine as well. However, when I dsm($form) after adding module specific form elements and a new submit handler, only the new submit handler is present in the form array. Despite this being the only handler present, all changes appear to register after submitting the form. I'm not sure why that's the case.
Comment #17
jhodgdonI'm not sure what's going on... Someone reported this issue presumably because there was a bug somewhere. I don't know how to reproduce this bug... the test-only patch passing suggests that maybe there is not actually a bug to fix on this issue?
Comment #18
znerol CreditAttribution: znerol commentedThis was necessary for #1831632: Convert node_rank_ $rank variables to use configuration system. Therefore I consider this Drupal 8 only. I see no reason to backport it.
Comment #19
jhodgdonOK, until such time as someone protests that we need it in Drupal 7.x, I'm setting this back to 8.x/fixed and removing the backport tag.