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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Issue tags: +Novice

Not sure what happened there

alexweber’s picture

Status: Active » Needs review
FileSize
4.15 KB

Patch attached that removes the hook definition, rewrites all implementations as hook_form_FORM_ID_alters() and updates relevant code comments.

Status: Needs review » Needs work
alexweber’s picture

Improved patch attached but I still get 2 test fails.

I'm going to need some help with this please... the tests that fail are:

  • "Extra type settings" not found
  • "Boost method" not found

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!

znerol’s picture

Currently only the search-settings are visible if the search-module was activated. So if you activate user node search, the user node-search settings become visible. Same for third-party modules implementing the old hook_search_admin. This logic has now changed with the hook_form_alter approach. The last two assertions from the following excerpt of SearchConfigSettingsFormTest.php are spoiling your patch (pay attention to the negation):

  /**
   * Verify module-supplied settings form.
   */
  function testSearchModuleSettingsPage() {

    // Test that the settings form displays the correct count of items left to index.
    $this->drupalGet('admin/config/search/settings');

    // Ensure that the settings fieldset for the test module is not present on
    // the page
    $this->assertNoText(t('Extra type settings'));
    $this->assertNoText(t('Boost method'));

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.

alexweber’s picture

assertNoText... 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?

tim.plunkett’s picture

No, #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.

alexweber’s picture

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

znerol’s picture

No, #states are slow and should only be used if it has the possibility to change after the page load from user interaction.

That'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.

znerol’s picture

Assigned: Unassigned » znerol
znerol’s picture

Status: Needs work » Postponed

Ok, 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 the form_alter, otherwise the search-ranking settings won't be saved.

znerol’s picture

Assigned: znerol » Unassigned
jhodgdon’s picture

Status: Postponed » Closed (duplicate)

This hook is going away since we are replacing the search hooks with a plugin system.
#2003482: Convert hook_search_info to plugin system