Remove Drupal Core 'search' module dependency from search404

@akhilavnair_zyxware Thank you for the patch, it works well for me as I tested this in Simplytestme & attached screenshot for same. PFA

Applied the patch successfully.

Can this change be reverted for use cases where core search is not being used on a site (for example, when using Search API Solr Search)?

Re-rolled the patch, so it will apply to the module downloaded from d.o. Also, updated note in the readme.

AKAICS, search 404 is not dependent on the core search module for any specific reason so this show be reverted.

The patch in #8 works for me applied to the dev branch. The patch in #9 did not apply to dev.

The Search 404 module works well with the Search API module which needs the core Search module disabled. The dependency should be removed. Thanks for the module and the patch!

RTBC

Some how I think the way this is being implemented is not the best way to do this.

Considering that in the d.o Common Pitfalls documentation it lists one of the common pitfalls is to have the core search on when another search module is being use.

Adding this to the project means that anyone that wants to improve search by adding, for example the Search API, now for best performance has know /learn how to patch a module to remove the dependency on the search module.

I think it would be better to see if a search module is enabled if not show a warning with a link to the modules page to enable the search module. If this warning can be shown on both the module's configuration page and the site status page this would alert the user that it is needed for this to work as no other search module was found installed.

Patch 9 fails on version 1.0.0. Patch 8 applies cleanly and successfully!

I was able to get #9 working by the look of it.

However with core's search module disabled this module no longer seems to do anything. Looks like it's hardcoded to depend on core search:

    if (\Drupal::moduleHandler()->moduleExists('search') && (\Drupal::currentUser()->hasPermission('search content') || \Drupal::currentUser()->hasPermission('search by page'))) {

I think more work is needed for this patch - if disabling a dependency means the module can't be used, then the dependency shouldn't be removed, and the module page should be updated to say that it must be used in conjunction with core's search.

Hi @Sophie.SK
Search 404 is not hard coded to use the the default Search module. However it is setup to use it out of the box. For other search modules you will need to configure the search module and Search 404 to work together.

I am using Search 404 with the Search API. Once you have the Search API module configured and an index created and indexed you need to configure the Search 404 to use a custom search url. See #2844729: Custom search to custom url? for an issue related to that that may give you issues.

Frederick

I am going to back pedal a bit as on a fresh test Drupal install when I went to apply patch 8 it failed, while patch 9 applied cleanly. The other project is still using patch 8 and both were using version 1.0.0 of search404.

OK, In case anyone else runs in to this, patch #9 works when using composer install --prefer-dist. Without --prefer-dist it looks like it uses source. Patch #8 applies cleanly to source. I discover this when I deployed as the deployment uses --prefer-dist, but I do not normally add it to my composer install commands during development.

@Sophie.SK where is that code snippet from? (#15)?

@Anybody that was from line 92 in search404/src/Controller/Search404Controller.php.

Thank you @Sophie.SK.

The comment in the following lines says:

// Get and use the default search engine for the site.

So I guess no modification is needed here, when using Search API instead of core. Which modules are you for search exactly? Did you modify the search path to match your Search Api configuration?

For me everything works fine with Search404 and Search API + properly set search path in Search404.

Let's find the reason for your problems and set RTBC again.

OK, I believe this issue has now been turned in to a support request when the issue status gets change not related to the issue at hand but related to a support request added to the issue.

The issue at hand has been clearly stated. The module even thought setup to use the default core search module out of the box should NOT require this module as a dependency, as it is considered a common pitfall (see previous comment) (I believe performance wise) to have the site creating duplicate indexes for search, when only one of them will be used. By requiring the core search module anyone that is using another search module, such as Search API, cannot currently easily disable the Core search module as it is a requirement. This is what this issue and its status should be focused on.

@Anybody and @Sophie.SK I have the Search API setup and working with this module. I applied a patch from this issue to be able to not have the core Drupal search module enabled. I had to configure the Search API and create an index for the content to be searched. You then need to configure search404 to use a custom search url.. The one on my site looks like this:
suche?suche=@keys
suche is German for search.

I am also tweaking the issue title and description to make it clear what this issue is about. Currently the wording of both is conflicting. I am wording this to be inline with the patches submitted to fix this issue.

I am also switching this back to RTBC. If you have a support issue related to configuring this module with the Search API module then there are better places to get help or you can create a separate support request ticket. I am however not opposed to anyone continuing to help @Sophie.SK with configuring Search 404 with the Search API in this issue. Look at my previous comments, I tried unsuccessfully it seems. However, the status of this issue should reflect that patches have been submitted and tested by the community, no reason related to the issue has been given that the maintainer(s) need more information to commit the patch and close this issue.

Thank you @frederickjh - I completely agree and confirm RTBC. @Sophie.SK should then create a separate issue after reading the docs and ensuring everything is configured correctly.