If a module's search implementation is disabled through search settings, why does hook_search_status not return 0?

Because the hook is poorly documented.

http://api.drupal.org/api/drupal/modules--search--search.api.php/functio...

Either the calling hook needs a wrapper to filter disabled search bins, or implementations need to check variable_get('search_active_modules', array('node', 'user')); before responding.

References:

#1101668: User module does not use Search APIs fully
#880554: search-index never completes, but admin/settings/search shows site as 100% indexed

CommentFileSizeAuthor
#33 1101678-28-hook_search_status-fix-text-7.x.patch577 bytesjhodgdon
#30 1101678-15-hook_search_status-fix-text.patch534 bytesjhodgdon
#28 1101678-28-hook_search_status-fix-text-7.x.patch577 bytesclayball
#15 1101678-15-hook_search_status-fix-text.patch534 bytesclayball
#10 1101678-hook_search_status.patch572 bytesReidsy

Comments

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

See http://api.drupal.org/api/drupal/modules--node--node.module/function/nod... for an example of the problem. It blindly returns a count.

If this is by design, that also needs to be documented.

Perhaps we should add a 'status' element to the return array instead.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Active » Postponed (maintainer needs more info)

I don't think this is necessary actually. The search module only invokes this hook on active modules -- see search_admin_settings()
http://api.drupal.org/api/drupal/modules--search--search.admin.inc/funct...

Right? So I don't think it's a problem if the hook doc stays how it is, and I don't think this is causing the other related problems you linked to?

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Postponed (maintainer needs more info) » Active

Search module is not the only thing that invokes this hook. Contrib can as well.

Drush, in the issue above, is assuming that 0 will be returned for inactive search modules. Drush is wrong because the API is unclear.

I can prove that hook_search_status returns non 0, and the question is: do we put the burden on the calling module? If so, the hook docs should explictly say so.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Read the current docs. The way they are written, it's up to the hook_search_status() implementation to return 0 if disabled. If so, that's implemented wrong in all cases.

Clarifying the docs is much easier.

/**
 * Report the status of indexing.
 *
 * Note that this hook returns an index count _regardless_ of the active
 * state of the module. It is the caller's responsibility to filter search totals
 * based on variable_get('search_active_modules').
 *
 * ...
 */
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Well... We can clarify the docs if you like. Changing how the API actually works for D7 at this point is a non-starter. I would also just point out that none of the other search hooks say anything about whether the search module is active or not. The individual module doesn't really know this -- search.module takes care of all the invoking of hooks and dispatching. If some contrib module is doing something other than what search.module does for hook invoking etc., then it's not really core's problem...

But OK, we can add something to say that the module doesn't have to check to see whether it's actually active or not. I am not thrilled with the exact language proposed in #4. How about something more like:

Implementing modules do not need to check to see whether they are active modules or not when calculating their return values, because search.module only invokes this hook on active modules.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Drush is a bit more advanced than a random contrib module. Since they got it wrong, I suspect others will, too.

I prefer to warn them, since we can. The language you propose does at least suggest that. A slight reword might make that stronger.

The core search module only invokes this hook on active modules. Implementing modules do not need to check whether they are active when calculating their return values.
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

That wording is fine with me. Just needs a patch...

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Yup. Hopefully a new contributor can do it. (Hence the novice tag).

Meta question: Do we have a general policy on hook documentation? There are two sides to a hook:

1) How to implement the hook.

2) How to properly call the hook.

We consistently do #1, but not #2. Is there a firm rationale for not doing so, or should I put that on my D8 target list?

We can open a new issue to discuss...

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

#8 - definitely a separate issue.

Still needs a patch with the wording on #6.

Reidsy’s picture

Reidsy commented
Status: Active » Needs review
StatusFileSize
new 1101678-hook_search_status.patch572 bytes
agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs review » Reviewed & tested by the community

Hooray! And a new contributor at that.

dries’s picture

dries commented

Committed to 8.x. Thanks Reidsy.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work

The patch that was submitted and reviewed contains a grammatical problem. Can someone please fix that?

Implementing modules do not need to check whether they are active calculating their return values.

Some word needs to be added between active and calculating, like "when".

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Just needs a 'when'. Oops.

clayball’s picture

clayball commented
Assigned: Unassigned » clayball
StatusFileSize
new 1101678-15-hook_search_status-fix-text.patch534 bytes

'when' added :-)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Thanks! This goes into 8.x.

Then we need to put the issue back to 7.x and make a patch combining #15 and #10.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1101678-15-hook_search_status-fix-text.patch, failed testing.

clayball’s picture

clayball commented

I see what I did.. sorry, rookie mistake. I'll fix it soon and submit a new patch.

clayball’s picture

clayball commented

Actually, I just redid this and came up with the same patch. I don't understand why it failed.

I'm new here so thanks in advance for your help and patience.

scor’s picture

scor commented
Status: Needs work » Needs review
Issue tags: -Documentation, -Novice

I'm not sure you did something wrong cwells73, it seems the testbot was not able to retrieve the patch.

#15: 1101678-15-hook_search_status-fix-text.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Documentation, +Novice

The last submitted patch, 1101678-15-hook_search_status-fix-text.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

This test failure in the Comment module is not related to the patch. Another glitch...

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs work » Reviewed & tested by the community
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue tags: -Documentation, -Novice

#15: 1101678-15-hook_search_status-fix-text.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Documentation, +Novice

The last submitted patch, 1101678-15-hook_search_status-fix-text.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

This is a doc-only patch. The patch applies. There is apparently something wrong with HEAD and a test is failing, which obviously has nothing at all to do with this patch. Let's just get this in (d7/8).

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue tags: +Needs backport to D7

Sorry. 8.x only. Then please mark "to be ported" for 7.x, because we need to get a combo of the fix in patch from #15, and the original patch from #10, in D7.

clayball’s picture

clayball commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 1101678-28-hook_search_status-fix-text-7.x.patch577 bytes

Here's a patch that combines the patched in comment #10 and #15 and is backported to D7.

Marked as 'Needs Review'.

Status: Needs review » Needs work

The last submitted patch, 1101678-28-hook_search_status-fix-text-7.x.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 1101678-15-hook_search_status-fix-text.patch534 bytes

Please don't make the 7.x patch until the 8.x patch has been accepted. It is confusing for both the test bot and the comitters.

Re-attaching the patch in #15. Which is RTBC for 8.x only (assuming the tests pass).

Then please mark 7.x - patch to be ported, at which time we can re-attach the patch in #28 and run it through the test bot.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

fix typo in tag

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Wow, confusing. :)

Ok, committed #30 to 8.x. Moving back to 7.x.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 1101678-28-hook_search_status-fix-text-7.x.patch577 bytes

Re-attaching cwells' patch from #28 to see if the test bot likes it.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

If the test bot is happy, this patch is RTBC for Drupal 7 (fixed already in Drupal 8, see above).

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x, too. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Documentation, -Novice, -Needs backport to D7

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

Issue tags: +Documentation, +Novice, +Needs backport to D7

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.