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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol’s picture

Status: Active » Needs review
Issue tags: +Configuration system
FileSize
728 bytes

Attach patch and set tag.

Berdir’s picture

As 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.

znerol’s picture

znerol’s picture

Attached 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.

Status: Needs review » Needs work
Issue tags: -Configuration system

The last submitted patch, 1831802-3-allow-submit-on-hook-search-admin.patch, failed testing.

znerol’s picture

Status: Needs work » Needs review
Berdir’s picture

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me, assuming the patch still applies (re-tested already).

Berdir’s picture

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -Configuration system +Needs backport to D7

OK 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.

bnjmnm’s picture

Assigned: Unassigned » bnjmnm
Issue summary: View changes

Working on 7.x backport.

bnjmnm’s picture

Status: Patch (to be ported) » Needs review
FileSize
545 bytes

Backported to 7.x

bnjmnm’s picture

Assigned: bnjmnm » Unassigned
bnjmnm’s picture

Assigned: Unassigned » bnjmnm
jhodgdon’s picture

Status: Needs review » Needs work

The tests would also need to be backported.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
5.65 KB

The 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.

jhodgdon’s picture

I'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?

znerol’s picture

This 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.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Fixed
Issue tags: -Needs backport to D7

OK, 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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.