Help File Fixup: search

Applied the patch and all is good there, but a few comments. :)

+++ modules/search/search.module	15 Nov 2009 15:57:48 -0000
@@ -96,9 +96,14 @@ define('PREG_CLASS_CJK', '\x{3041}-\x{30
+      $output .= '<p>' . t('The search module adds the ability to search for content by keywords. Search is often the only practical way to find content on a large site, and is useful for finding both users and posts.') . '</p>';

We shouldn't use 'posts' here per [#604342] so how about 'Search is often the only practical way to find content and users on a large site.'

+++ modules/search/search.module	15 Nov 2009 15:57:48 -0000
@@ -96,9 +96,14 @@ define('PREG_CLASS_CJK', '\x{3041}-\x{30
+      $output .= '<p>' . t('It is important to note that by default, the search module only supports exact keyword matching. You can modify this behavior by installing a language-specific stemming module for your language, which allows words such as walk, walking, and walked all to match each other. Another approach is to install an n-gram module, which breaks words down into small, overlapping chunks and finds words with a high degree of overlap, so that words like earthquake and quake can match each other. A third approach is to use a third-party search technology with features like this built in; there are modules available for several of these, such as Apache Solr and Sphinx.') . '</p>';

It might be better to make this paragraph more concise, and instead link to the documentation that deals with extending search, because newer and better search modules will appear later on (don't know how often core help files are updated?). It is a good idea to provide examples of how the search can be extended.

More comments:
- First sentence should mention you can search for users as well as content.
- I agree about removing "the only practical way". That is silly. Good IA makes content findable.
- The bits about stemming and n-gram were approved by Dries and other reviewers in a previous issue, so please leave them there. It is important for people to realize that core Search is exact keywords only and what they can do about it.

Now I'm thinking the first couple sentences could read better. I removed 3 instances of the word 'search' as well.

Not sure the phrase 'match each other' makes sense, but went with 'matches' since that's probably the Drupal term of choice. Thoughts?

Ignore previous, use this one instead ('and' instead of 'or' in the 1st sentence!)

remarks:

"The search module adds the ability to search for content by keywords and users by username or email. The search module includes an advanced option for searching by content type."
i would add links to the search and advanced search here

"modify this behavior by installing a language-specific stemming module for your language"
add link to this module

"Another approach is to install an n-gram module,"
add link to this module

"A third approach is to use a third-party search technology with features like this built in; there are modules available for several of these, such as Apache Solr and Sphinx."
add links

"For more information, see the online handbook entry for Search module."
for uniformity this should go to the end of the "About" paragraph

i miss a sentences about the search block that can be enables and positioned via blocks

i miss a sentence about the search variable that is available in some(?) themes and that must be switched on in the theme config page (maybe in D7 this isnt the case anymore?)

apart from that i'd say it's okay

I don't think you can add links to the stemming and n-gram modules. There are a lot of them!

then that can be explained in the help text. if you mention additional contrib search modules you should explain where they can be found. if not the first-time-drupal user will start searching inside his drupal installation ...

major overhaul off last patch - added some links in, moved info about add-ons into a new section, as well as new block section in uses, and fixing a bunch of formatting for consistency.

One minor typo:
"so that words like earthquake and quake to be matched" I think that last "to" should be "can".

Two other questions/suggestions:
- Should we maybe provide a link to find modules on drupal.org/project/Modules to find the stemming and n-gram modules?
- I don't think that linking to the status report page gives anyone much help on setting up cron. How about instead linking directly to http://drupal.org/cron (which actually explains how to set up cron)? This also applies to the admin/config/search/settings help text (which you didn't create/modify in this patch). My preference would be to link both of them to the d.o page on cron, or else put both links in.

One error (I think):
- The settings section makes it sound like the priorities on the search settings page are setting priorities for indexing content. In reality (I think) they are setting rankings for returning content in search results.

Looking much better.
One other thing: the links to Solr and Sphinx are not right, e.g. Sphinx is pointing to http://localhost:8888/d7/admin/help/sphinx

can't keep up with all the reviews, if someone else can incorporate these comments into a new patch, please do! Thanks! /join #drupal-contribute on IRC if you need someone to help you through that.

Got this

you rock! thanks :-)

(argh, all my comments here vanished. starting over...)

- addressed comments in 14 & 15 but didn't link Search settings page to d.o/cron (since it is already linked once)
- incorporated some of the consistency fixes arianek suggests (in another help file issue)
- added 'Indexing settings (word length)' to be consistent

Cheers

thanks lisa!

- added a bit more to one sentence that seemed too short: "These contributed modules can be downloaded by visiting Drupal.org."
- fixed a spelling mistake

RTBCing

Indentation again... :)

And can someone fix up the next couple lines after this patch:

     case 'admin/config/search/settings':
       return '<p>' . t('The search engine maintains an index of words found in your site\'s content. To build and maintain this index, a correctly configured <a href="@cron">cron maintenance task</a> is required. Indexing behavior can be adjusted using the settings below.', array('@cron' => url('admin/reports/status'))) . '</p>';

Let's link to drupal.org/cron rather than admin/reports/status. admin/reports/status doesn't let you set up cron, it just reports on the status of cron and many other things. I really don't think it's helpful to link to it when the text is about configuring your cron job.

indentation and link suggested changed

fixed a couple caps and punctuation - RTBC

Couple of minor tweaks:

"The Search module provides the ability to index and search for content, by keywords, and for users, by username or email."

The number of commas in that sentence makes my eyeballs hurt. :) Also, by the very narrowest of margins, it appears we use 'e-mail' more than 'email' in core (standardizing that across core might be another nice follow-up patch). How about change to:

"The Search module provides the ability to index and search for content by keywords, and for users by username or e-mail."

(Also, neat. I didn't know it searched e-mails. I thought that was Apache Solr doing that on Drupal.org.)

"The search module also includes an advanced search option"

"Search" module.

      $output .= '<dt>' . t('Extending Search module') . '</dt>';      $output .= '<dd>' . t('By default, the Search module only supports exact keyword matching. You can modify this behavior by installing a language-specific stemming module for your language, which allows words such as walk, walking, and walked to be matched. Another approach is to install an n-gram module, which breaks words down into small, overlapping chunks and finds words with a high degree of overlap, so that words like earthquake and quake can be matched. A third approach is to use a third-party search technology with features like this built in, such as Apache Solr and Sphinx. These <a href="@contributed">contributed modules</a> can be downloaded by visiting Drupal.org.', array('@contributed' => 'http://drupal.org/project/Modules/')) . '</dd>';

This is useful information, to be sure, but we don't mention extending stuff with contributed modules on any other help pages. This feels more like something that belongs in the handbook, perhaps? (Oh. It was in the old help text. Well hrm then.)

I dunno. This bit of text still feels like the odd duck out to me, and I would advocate removing it. But if we keep it, we should at least move it to the bottom. Right now we basically say "Welcome to the search module... it totally sucks. Here's how to get something better!" which is not very helpful. ;)

Actually, unless the module has been changed for Drupal 7, only highly priveleged users can search by email, because normal users cannot see each other's email addresses. I didn't catch that in my last review, sorry!

That bit about the alternatives to search was discussed extensively and approved in another issue, which I may be able to find if you want to see the discussion. I personally think it is VERY important to mention the limitation of Drupal core search that it only does exact matches, because due to most people's experience with Google etc. they are expect web searches to do fuzzy matching. That was what that issue was about. So if we mention the limitation, it makes sense to me to also mention how to fix it.

made all the changes suggested by @webchick - kept the extending search section, but moved it to the bottom. RTBC!

I just checked the code, and only users with 'administer users' permissions can search for users by email address.

Should that be mentioned? If not, I'm OK with RTBC.

new patch

i changed:

"The Search module provides the ability to index and search for content by keywords, and for users by username or e-mail. "

=>

"The Search module lets you search for content by keywords. Users with sufficient permissions can search for users by username or e-mail."

I think it's really important not to cut out the part about indexing content - reroll with the change to:

"The Search module provides users with the appropriate permissions the ability to index and search for content by keywords, and for users by username or e-mail."

RTBC

Couple of things:

1. I think it does make sense to add back the sentence from #30 about permissions for user searching. Only thing is elsewhere we explicitly spell out which permissions are for what, and we should do so here as well.

2. If we're going to give a couple of for instances of external search module, let's also give a "for instance" module for "n-gram" searching, since I have no idea what the hell that means, and I assume the average reader of this page won't either. (I think this is Porter-Stemmer module? can someone confirm?)

3. I'm cool with making passing reference to other contributed modules where it makes sense (ex. Pathauto in Path module's help), but in general, I find the depth of explanation we're doing here just seems completely over the top crazy in-depth. Is there a way we can communicate the finer points of that paragraph -- added in #546302: Help screen for Search module doesn't make limitations clear -- without totally overwhelming people with information?

@webchick - i don't understand what you're saying in #1 (?)

for #2 - i have no idea what n-gram is either, i think jennifer had suggested those details. (anyone?)

#3 - i agree... i'm thinking a lot of that info should just be added in more detail to the handbook http://drupal.org/handbook/modules/search and then we can chop it out.

1. (only change since the patch in #31)

I tried to add the explicit permission for each action in the help page.

Also, I added a first section "Search", where I move the "advanced search" stuff form the introduction; so we can mention the permissions at the same time without giving too much information in the first help text paragraph.

2. & 3.

I also think this "Extending Search module" section should be at d.o, since some of you mention the concept is not so common. In the other side, the description there is really good, but like JuliaKM mention and example(see wolfram trying to do it) could confuse more the casual reader.

Regarding the latest patch:

- The Search section under Uses needs to mention which permissions are needed to search for users by email. It still doesn't. I think it should explicitly say it lets you search for nodes/content and users by user name, and if you have the other permission, you can also search for users by email. It should also clarify that advanced search only applies to content search.

- It is not valid HTML to put attributes (such as href=) in single quotes. They need to be in double quotes.

- "Users with Search content permission can use it." (in the Search block section)... This is unclear: Will other users see it but not be able to use it?

- Regarding the controversial "Extending" section... Here are my thoughts. First, it's really important to mention that the search module help mention that content search is exact matching only. I really think it should be mentioned in the About paragraph, and not just in an "Extending" paragraph. Second, I think it's a bad idea to mention Porter Stemmer (or any other specific stemming module) as an example, because stemming modules are language-specific and Porter is only for American English. Porter will probably be available for D7 (I am the maintainer) in spite of the lack of D7UX tag (I think that whole initiative is meaningless and stupid so I have not signed up for it, but let's not discuss that here/now). The n-gram module for D6... there used to be one, but I cannot find it now (tried a variety of searches on d.o). There was an effort to add this to D7 core, but it didn't happen, see #103548: Partial Search in Drupal Core. So probably the n-gram approach could be omitted now.

attempt to incorporate jennifers comments

Clarifications (I looked at the code again to make sure):
- Keywords must be exact matches of entire words for content search, but not for user search (user search looks for a substring automatically).
- You need to have 'search content' permissions ot see the search block or conduct a search at all, no matter what type of search is being done. Additionally, to search nodes, you need 'access content' permissions; to use advanced search on nodes you need 'use advanced search' permission (there is no advanced search for users); to search users, you need 'access user profiles' permissions; to search users by email address you need 'administer users' permission.

I think the two sentences about advanced searching in the Searching Content and Users section should be combined. Also, it looks like you can search by language on Advanced Search, if there is more than one language installed on your site.

In the extend section, let's take out the n-gram section, since there doesn't appear to be an available n-gram module. For stemming, you could say "such as the Porter Stemmer module for American English" and link to http://drupal.org/project/porterstemmer and for the others: http://drupal.org/project/apachesolr and http://drupal.org/project/sphinx

Here's a new patch.

* Oops, this has been in the last several patches "provides users with the appropriate permissions the ability to index and search for content"

- Have modified to "provides the ability to index and search for content"

* "can use the search form and the Search page."

- That's confusing. Changed to "can use the Search page"

- Caps on advanced search in 'Searching content and users' section

(We must be getting close ;-))

The search form and search page are actually two different things: There is a search block containing the search form, and a search page (which also contains the search form). Probably should say search block and search page, rather than search form and search page.

Your PNG cut off the extending section. :)

This may have been my problem, or someone else, but the latest patch in #41 has another issue: the change from single to double quotes in the admin/search/settings help makes an href='something', which is NOT legal HTML. Attributes have to be in double quotes in XHTML, not single quotes. Please set it back to single quotes, and use \' if you need to use an apostrophe in the text.

Other than that and my comment in #42, I'm happy with the latest version.

Rerolled with comments from 42 & 44.

This one is most likely RTBC now :)

just a note, i'm maxed on proper reviews for tonight - @jhodgdon using double quotes for the string and single quotes on the link, and no / is kosher - this was addressed in IRC... somewhere around the halfway point, and it was confirmed that it is ok, and that it is preferable not to escape the apostrophes. (don't have the reference link on hand, sorry)

OK, my mistake.

I like the latest patch.

Much better. Thanks, all. This addresses all of my previous comments!

Committed to HEAD!