admin/reports/search dependent on dblog module

I just tried this and the 'Top search phrases' page works both with and without the dblog module.

If this is still a problem for you, please provide more details of the error message.

Ian

Agreed, this looks like a real bug.

This menu item should really only exist if *both* the search module and the dblog module are turned on, by the way.

This is the same in Drupal 7 and should likely be fixed there first, then backported to D6.

OK, here's a simple patch, which works fine. When running search_menu(), it only adds the Top Search Phrases report to the menu item list if dblog.module is enabled.

The other way to do this would be to put the menu item into dblog_menu() and change it to module_exists('search'). Which will have exactly the same effect, so I don't have strong feelings one way or another.

I also confirmed that without this patch, if dblog.module has been uninstalled (not just disabled), if you go to admin/reports/search you get a big orange error box saying:

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaldev.watchdog' doesn't exist: SELECT COUNT(DISTINCT(message)) AS expression FROM {watchdog} watchdog WHERE (type = :db_condition_placeholder_0) ; Array ( [:db_condition_placeholder_0] => search ) in PagerDefault->execute() (line 86 of /opt/www/eclipsework/drupal7/includes/pager.inc).

So I think the patch is needed for Drupal 7.

#7: 421838.patch queued for re-testing.

Looks like this patch needs rerolling

re-rolled. there was just a 2 line offset. I'm not sure why it failed to patch that?

Anyone want to review this? I wrote the patch, so I can't. chrisdolby in #9 confirmed that it works, but the patch still needs a review. :)

I did test it visually myself, and it works.

I only remember doing these hook_menu conditionals in Drupal 5 or 6 in contrib. I thought maybe menu_enable was the only thing to call menu_rebuild on module_enable, but it looks like it's happening anyway.

The patch in #13 has a tab/space indentation issue that was not in the original patch.

Here's another reroll.

#16: 421838-fixtabs.patch queued for re-testing.

#13: 421838-2.patch queued for re-testing.

Maybe we should move the menu handler to the dblog module instead of adding a module_exists()? Not 100% sure why we have it in search.module.

It's a report of the most recent (or is it popular?) search terms. It doesn't work correctly unless both the search and dblog are turned on.

I think it makes most sense to leave it in search.module, but only turn it on if dblog is turned on.

dblog_top('search') lists the watchdog entries that are of type 'search'.

Technically, you could uninstall the search module and this would still list the search entries that occurred when the search module was active. But this would be confusing I think.

This menu item shouldn't appear unless both search and dblog are enabled.

It makes sense for it to be in dblog as that module has the function to be called and we can leave off the 'file path' setting.

Patch works, approach is fine with me.

Subscribing.

Committed to CVS HEAD. Thanks.

Maybe we should port this to Drupal 6?

Here's a patch on D6.

When you upload patches, please read the info about extensions on the Attachments field (don't put d6 in the extension).

This patch looks right but someone should test it in D6 and make sure:
- the problem exists without the patch (see #8)
- the problem is fixed with the patch
- the report still works if both search and dblog are enabled

1. The problem does indeed exist without the patch.
2. The problem is fixed with the patch.
3. The report still works with the patch if both search and dblog are enabled.