Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
hook_search_admin() is a specialized hook that's less useful than hook_form_alter(). Let's kill it and tell people to use hook_form_FORM_ID_alter() instead.
See #1831802: Allow implementors of hook_search_admin to add #submit on the form for background.
Comments
Comment #1
larowlanNot sure what happened there
Comment #2
alexweber CreditAttribution: alexweber commentedPatch attached that removes the hook definition, rewrites all implementations as hook_form_FORM_ID_alters() and updates relevant code comments.
Comment #4
alexweber CreditAttribution: alexweber commentedImproved patch attached but I still get 2 test fails.
I'm going to need some help with this please... the tests that fail are:
They live in lines 96-97 of SearchConfigSettingsFormTest.php
The funny thing is that if I unhide the search_extra_type module and enable it in the UI, these 2 fields DO in fact appear in the search config form. However, for whatever reason the tests fail... any ideas?
Thanks!
Comment #5
znerol CreditAttribution: znerol commentedCurrently only the search-settings are visible if the search-module was activated. So if you activate
usernode search, theusernode-search settings become visible. Same for third-party modules implementing the oldhook_search_admin
. This logic has now changed with thehook_form_alter
approach. The last two assertions from the following excerpt of SearchConfigSettingsFormTest.php are spoiling your patch (pay attention to the negation):I suggest to modernize this form and just hide module settings using the states API if the module is not selected. In this case I think it would be legible to drop those assertions.
Comment #6
alexweber CreditAttribution: alexweber commentedassertNoText... darn how did I miss that? :) Thanks @znerol!
Better than hiding using States, shouldn't we just do a module_exists() and not even add those elements?
Comment #7
tim.plunkettNo, #states are slow and should only be used if it has the possibility to change after the page load from user interaction.
I think this should focus on swapping the two hooks, and push any major refactoring to a follow-up.
Comment #8
alexweber CreditAttribution: alexweber commented@tim.plunkett,
Please advise as to how to proceed then. The patch works as advertised in the UI but fails 2 tests for the reasons stated in #5 by znerol. Not sure what to do here...
Comment #9
znerol CreditAttribution: znerol commentedThat's actually the case here, the page should change depending on user interaction. Search can be enabled and disabled in the "ACTIVE SEARCH MODULES" fieldset for modules supporting it. Therefore
module_exist
will not help us here. However currently the settings for to be activated search modules are only shown after the configuration is saved. Another funny effect of the current implementation is that one is able to deselect a search module and forget to change the default search module. The result is that a validation error is thrown at form-submit that the chosen default search module is not active.It also would be possible to forego
hook_form_alter
and simply tell modules which have their own search-settings to install a tab on the search settings page. In this case we can avoid#states
and leave the search configuration form as is.Comment #10
znerol CreditAttribution: znerol commentedComment #11
znerol CreditAttribution: znerol commentedOk, this is somewhat blocked by #1831632: Convert node_rank_ $rank variables to use configuration system. We actually need to add
node_search_admin_submit
as a submit-callback from within theform_alter
, otherwise the search-ranking settings won't be saved.Comment #12
znerol CreditAttribution: znerol commentedComment #13
jhodgdonThis hook is going away since we are replacing the search hooks with a plugin system.
#2003482: Convert hook_search_info to plugin system