Hi,

I found this in the issue queue: http://drupal.org/node/635704 - which is essentially the same request. But I didn't want to warm up a very old issue.

I'm using search_api and would like to use search404. The dataset is rather large - enabling search.module creates a 200MB index in the database which will never be used. Enabling search.module also creates a couple of other problems when using search_api at the same time and simply put, makes things more complicated than they have to be.

I was thinking about manually removing the dependency from the module, but this breaks the update workflow. What about moving the dependency check to the module page (UI), checking for a supported search mechanism to be enabled or suggesting the default search.module instead of "hardcoding" it to one option?

Please, reconsider. :-) If not, maybe this module could be forked into a search_api-only version?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

azinck’s picture

+1. This module does *not* need to have a dependency on the Search module. Search 404 provides useful functionality without having Search enabled and should not require the enabling of such a large, non-performant module for no reason.

Hitby’s picture

Agreed. I'd love to use this as it seems to work well with the redirect to my search_api search. I don't need the standard drupal search or the related pages it creates (/search).

grasmash’s picture

Absolutely agree.

zyxware’s picture

Looks like there is renewed interest in this feature. I will look at moving the dependency into a hook_requirements check that will check whether one of the supported search modules are enabled. That should take care of this issue right?

Anonymous’s picture

Yes, I think this would be it. As long as search 404 doesn't need core search to be enabled, we would be very happy and thankful campers. :)

azinck’s picture

You could use hook_requirements to deliver a warning message, but I'd argue you should just transparently hide configuration options and functionality that aren't relevant when any given supported module is missing. Or, put another way, only display configuration for, and enable functionality that's relevant for the current suite of enabled modules.

pvhee’s picture

#4 would make the trick. Core Search is indeed often not wanted.

alanom’s picture

Even #4 is unnecessarily fussy. You would be hardcoding in the soon-to-be-out-of-date selection of search and search-like modules that existed at the time of writing, when you have an option, do_custom_search, that works with literally any path that accepts search-string like arguments. It could potentially work just fine with future modules that haven't even been created yet.

Here's a really simple patch that allows do_custom_search to work with any modules or features that might be able to react intelligently to the paths created. It seems to work fine.

Also, it uses a more appropriate security access check and skips some code that is redundant if do_custom_search is used. If we're redirecting to a custom path and letting some other module or feature do the hard work, the appropriate access check is drupal_valid_path(), which checks using the data we have (the path), not data based on search module which might not be relevant in this case. Also, the only variable we need is the $keys string to pass to the path and the messages, since $results etc are not used.

alanom’s picture

Status: Active » Needs review

If search isn't enabled, and no items are selected in the settings form (implying that the default, core search, is selected), nothing will happen - the module will fall back to the default 404. That's fine.

I thought about making custom path the default if core search is disabled, then realised that would be a terrible idea since a dummy default example path would be used after the module is enabled before it is configured.

The module doing nothing after it's enabled before it's configured is the most desirable result in cases where it needs to be configured.

xatoo’s picture

Status: Needs review » Needs work

It looks like you created the patch twice apending one patch to the other.

Apart from that, after deleting the first half of the patch, removing trailing whitespace on line 51 and using -p2 for git patch it does apply cleanly. However, using do_custom_search and with search disabled I get a 404 "Page not found" instead of search results. Even with the search module enabled i get a 404, al be it with a prepopulated search field.

alanom’s picture

Re the double patch: oops, removed first half and attached patch.

Re getting 404 - did you set up the settings form to use custom search, and did you input the custom URL to use in the form? Just getting a 404 is what will happen if nothing is set.

Beyond that, this patch works for my needs: feel free to take it and run with it, but don't wait for more work from me as I don't have time at the moment.

xatoo’s picture

I took a deeper look and it turns out that drupal_valid_path does not work with the path I use which is 'articles?q=@keys'. Maybe we should strip of the path from the '? sign', before passing it to drupal_valid_path. I am willing to change this and roll a new patch if you agree about this approach.

alexp999’s picture

I had the same problem of it not working due to drupal_valid_path() returning false, but I do not understand why this check is needed. It does not exist in the original implementation of custom path.

I just changed it to:

  if (variable_get('search404_do_custom_search', FALSE)) {
    if (!variable_get('search404_disable_error_message', FALSE)) {
      drupal_set_message(t('The page you requested does not exist. For your convenience, a search was performed using the query %keys.', array('%keys' => check_plain($keys))), 'error');
    }
    $custom_search_path = variable_get('search404_custo_search_path', 'search/@keys');
    $custom_search_path = str_replace('@keys', $keys, $custom_search_path);
    search404_goto($custom_search_path);
  }

Which is basically just moving that block of code outside of the search module check, that is all a path from this issue needs to be, we shouldn't be adding any more checks surely? I don't know how to create a proper patch, but this is the patch from #11 modified to remove the (imo not needed) check.

Island Usurper’s picture

Status: Needs work » Needs review
FileSize
10.68 KB

Even though search_by_page and google_cse rely on core search, I think this function could be cleaned up better by putting the module_exists('search') check after we're sure we won't be redirecting away to another page. Those other modules won't be enabled if search isn't either, and their pages should handle user permissions too.

As an added bonus, I've fixed the TODO concerning the duplication of the message code.

GaëlG’s picture

#1076190: Search API integration (patch) marked as a duplicate (I submitted a patch there).

Jorrit’s picture

Status: Needs review » Needs work

Unfortunately, the patch no longer applies. Can the patch be recreated?

Netrics’s picture

Status: Needs work » Needs review
FileSize
6.81 KB

Since I use this module with Google Appliance module, having the core Search module enabled for me is useless. I wanted to be able to remove the "/search/$1" path (enabled by default with the core Search module) and use the "/gsearch/$1" path from Google Appliance instead. Now since I only use the custom path setting in Search404, there is no need for any other the other options which only seem to require the core Search module.

This may not be the best patch file, but it does the job for me and my needs on the current Search404 7.x-1.3 version. Maybe it will be helpful to others.

mmikitka’s picture

I implemented the suggestion made by xatoo in comment #12 re: removing query parameters (i.e., everything after and including a "?").

This meets my needs.

Attached is the patch.

designerbrent’s picture

Issue summary: View changes

This patch mostly works for me using search api module.

I did find that some of the options did not work, including the Jump to First Result and Custom Error Message.

maxplus’s picture

Hi,

I used the patch from Netrics from #17 and does the job for me perfectly.

I'm also not using the core search module but I do use Search API and I only needed the option to redirect to the custom search path including the search keys.

Nice that by using this patch, I could use the "lightweight" version of the search 404 module

Thanks!

nodecode’s picture

@designerbrent, I'm pretty sure the options that you say don't work for you, don't actually work with search_api regardless of whether or not you have search.module enabled. Besides "Custom error message" is not an option AFAIK.

Love the direction of this thread though. For all of us Search API users, that redirect functionality is really the most important aspect IMHO. Looking forward to this being committed!

nodecode’s picture

#18 worked great for me with the latest dev release

DamienMcKenna’s picture

omarlopesino’s picture

Path #18 worked for me. Thanks!

This changes in the module couldn't be done with google cse option?

generalredneck’s picture

Status: Needs review » Reviewed & tested by the community

We've been successfully using #18 for a while. RTBC

anish_zyxware’s picture

Assigned: Unassigned » anish_zyxware
Status: Reviewed & tested by the community » Closed (fixed)
FileSize
3.09 KB

The issue is commited and is available in 7.x-1.x branch. Final patch is attached.

anish_zyxware’s picture