Problem/Motivation

All searches managed by the core Search module, at least within the same search plugin, currently have the same page title. It would be nicer, and more consistent with standard practice on other web sites, if the search terms were shown in the page title.

Proposed resolution

Change the page title so that it shows the search terms: allow plugins to provide a search results page title, and provide a sensible default in the base plugin class.

Remaining tasks

Patch in comment #37, includes tests.

User interface changes

Page title includes search terms, if some are entered. For instance, previously a search at:
example.com/search/node?keys=foobar
would have title "Search", and now it will have title "Search for foobar".

API changes

Search plugin interface has a new method suggestedTitle(). Most plugins, however, will just use the default implementation provided by SearchPluginBase, so plugin classes should not need updates.

Original report by lizards

It would be nice if the title of the search results page had the search term..

<title>Search term | drupal.org</title>

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

magico’s picture

Version: 4.7.2 » x.y.z

Seems a nice usability feature.

formicin’s picture

Version: x.y.z » 4.7.x-dev

Did anyone make it work? I tried to hack the module but I couldn't even located the "search" word which is the title.

birdmanx35’s picture

Version: 4.7.x-dev » 6.x-dev

Moved to 6.0.

robertDouglass’s picture

Version: 6.x-dev » 7.x-dev

Perfectly valid feature request, especially since we have search URLs that can be bookmarked.

robertDouglass’s picture

The title currently comes from the parent menu item to the tabs:

  $items['search'] = array(
    'title' => 'Search',
    'page callback' => 'search_view',
    'access arguments' => array('search content'),
    'type' => MENU_SUGGESTED_ITEM,
    'file' => 'search.pages.inc',
  );

I don't know how easy it will be to do this at the menu level. Perhaps the title should be set by each implementing search in hook_search $op = "search"?

David Lesieur’s picture

"Field" keys such as type:XX and tid:NN further complicate matters since we don't want to show them as-is in the page title. If we had a kind of query object (which has been discussed during the Minnesota Search Sprint) built from the search text, each "key" might have a method for returning a "printable" string appropriate for use as a label or title.

naxoc’s picture

FileSize
92.25 KB

Something like this in the search_view function does the job:

      // Filter out type:XX  
      $search_string = preg_replace('/([^ ]*:[^ ]*)/i', '', $keys);
      if (!empty($search_string)) {
        drupal_set_title(t('Search results for') .' '. preg_replace('/([^ ]*:[^ ]*)/i', '', $keys));        
      }

It is probably a little too naive a solution, but I wanted to see how it would make the page look. I attached a screenshot.
While I like that the title tag contains the search phrase, I am not sure about the page title above the search form. The phrase "Search results" appears twice. It would be easy to append the search phrase to the "Search results" below the search form, but that would make the page even clunkier.

Any thoughts on the usablilty of this? Where should the search phrase appear?

naxoc’s picture

Issue tags: +Usability

Added tag

drupallogic’s picture

can you pls tell where to put this code ?

naxoc’s picture

Status: Active » Needs work
FileSize
992 bytes

Here is a patch with what I tested the page out with.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review

Hmmm... I guess as this is a feature request, it should go out to Drupal 8... See what happens when no one is maintaining the search module?

cluke009’s picture

I've tested this under d7 and confirmed that it works

Status: Needs review » Needs work

The last submitted patch, 74562.patch, failed testing.

cluke009’s picture

I haven't done any work with the testing framework yet but this looks like a pretty straight forward addition. Do the test files need to be updated to match the change or is there something actually wrong with the patch code?

naxoc’s picture

Status: Needs work » Needs review
FileSize
681 bytes

There isn't really a test for the piece of code yet. What happened was that the patch did not apply anymore. Here is a reroll. I am still unsure that this is a good way to name the page (looong search strings will give a funny page title).

Status: Needs review » Needs work

The last submitted patch, search_page_title-74562-15.patch, failed testing.

naxoc’s picture

Ouch. OK. What happened was that a whole lot of tests failed.

While I can probably update the tests, I do need some input on how we want the page titles to look? Is the patch from #7 a way to go?

joachim’s picture

Might be an idea to truncate the search phrase when it appears in the page title, eg:

"Search results for 'foo bar really really long cut off here ...'"

naxoc’s picture

Status: Needs work » Needs review
FileSize
738 bytes

Here is a patch that truncates the string searched for to 200 characters. I haven't fixed the tests (quite a lot of work there), but if people think this is a way to move forward, I will.

Status: Needs review » Needs work

The last submitted patch, search_page_title-74562-19.patch, failed testing.

jhodgdon’s picture

Title: Title of search Pages » Show keywords in title of search results page

Updating title.

This still seems like a good idea, a mere 7 years after this issue was filed.

The patch will need an update; currently search_view() is being converted to a controller in
#2105609: Convert search_embedded_form_form to a Controller
but I think after that gets done (should be fairly soon), we should revisit this.

I wonder if maybe the suggested title should be set by the search plugin though. For instance, the node plugin could set the title based on the keywords without any advanced search stuff, and the user plugin might do something different? We could add a method on the plugin interface and base class for suggestedTitle() or something like that, and then call it in the controller to set the title? Or the plugin could just call drupal_set_title()?

mikemiles86’s picture

Assigned: Unassigned » mikemiles86
Issue summary: View changes
mikemiles86’s picture

Assigned: mikemiles86 » Unassigned
Status: Needs work » Needs review
Issue tags: +SprintWeekend2014
FileSize
2.21 KB

I wrote a patch based on the last comment/suggestion by jhodgdon.

I have added a method to the plugin interface called 'suggestedTitle()', which is expected to return the title suggestion when viewing search results. suggestedTitle() is invoked in the view() method of the search controller, which was previously setting the title by doing:

$build['#title'] = t('Search');

This mehtod makes it possible for search plugins can define their own suggested page titles. My patch also has a default implementation of suggestedTitle() it in the SearchPluginBase. It returns the title of 'Search' appended with any passed keywords.

Status: Needs review » Needs work

The last submitted patch, 23: search-show_keywords_in_title-74562-23.patch, failed testing.

jhodgdon’s picture

Thanks for the interest and the patch!

I think this is an excellent approach. Some details to work out:

a) Some of the tests are failing, and need updates.

b)

+  /**
+   * Provides a suggested title for a page of search results. Title suggestion
+   * should be translated.
+   *
+   * @return string
+   *    The suggested page title.
+   */
+  public function suggestedTitle();

This has a couple of formatting issues: the first line should be one sentence of less than 80 characters, and the indentation in @return is one character too much. Suggestion for the first sentence would be "Provides a translated title for a search results page."

c)

+  public function suggestedTitle() {
+    $title = $this->t('Search');
+    //If keywords are passed, append them to the title.
+    if (!empty($this->keywords)) {
+      $title .= ' ' . $this->keywords;
+    }
+    return $title;
+  }

Formatting: after // put a space before starting the comment.

Also... I am not certain that "Search(space)keywords" is the best title in all languages. It would probably be better to use a placeholder in t(). Something like passing Search @keywords into t()?

c) We also need to be careful here. We don't want to put unsantized text into the HTML title of a page.

d) This needs a test. Given (c) we should probably test it with some special characters like ' " ; < > etc. in the search keywords.

mikemiles86’s picture

I have updated my patch with the formatting changes suggested by jhodgdon in comment #25. (b & c). I have provided an interdiff in regards to changes between the two patches.

In regards to c) about making sure keywords are sanitized. By the time suggestedTitle uses $this->keywords, looks like their values have already been sanitized.

I'm a bit novice when it comes to writing/altering tests, if someone else is available to help write new tests and update the old failing tests.

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests

Thanks!

a) When you attach a patch, even if you don't think it's done, please set the status to "needs review" to launch the test bot and let human reviewers know there is a new patch to be reviewed.

b) I disagree about the sanitization being done ahead of time. The SearchController->view() method just takes the keywords from the URL and calls setSearch(), and all setSearch() does on the default plugin anyway is set $this->keywords to that value.

But that is actually OK the way you did the patch, because

+      $title = $this->t('Search @keywords', array('@keywords' => $this->keywords));

will sanitize the keywords (@ means "pass the input through check_plain()).

Anyway, I'll set this to Needs Review so we can see which tests need fixing. If you want to get some help during Office Hours to write the tests, assign this back to yourself; otherwise, hopefully someone else will pick it up and take care of the tests.

jhodgdon’s picture

Oh, one other note:

The documentation for suggestedTitle() needs to mention that the title should be translated.

Also I think in the default implementation

    // If keywords are passed, append them to the title.

this comment is a bit misleading because the keywords are not "passed" into this function. Probably it should say "If keywords have been set"?

Status: Needs review » Needs work

The last submitted patch, 26: search-show_keywords_in_title-74562-26.patch, failed testing.

naxoc’s picture

Status: Needs work » Needs review

Here is a reroll. It's going to fail the tests, but I want to see which ones I need to look at.

naxoc’s picture

FileSize
2.19 KB

No wait, HERE is the patch.

Status: Needs review » Needs work

The last submitted patch, 31: 74562-30.patch, failed testing.

jhodgdon’s picture

Hi @naxoc, are you planning to continue with this? Because the patch looks nice and simple. All we need is to fix those 11 test assertion fails.

I think the reason most of those tests are failing is that they're using assertNoText() to assert that certain things are not found in searches. These things are in the keywords that are being searched for, and with this patch, they'll now appear on the page because the page title contains keywords and that is in an H1 tag on the page.

And then the other test failures are actually testing the page titles of the search pages; those obviously need adjustment.

So the tests should hopefully be pretty easy to fix. We should still do this.

jhodgdon’s picture

Oh, one other thing:

+   * @return string
+   *   The suggested page title run through t().
+   */
+  public function suggestedTitle();

The usual way to document this is to say "The translated suggested page title." not "run through t()".

eiriksm’s picture

Status: Needs work » Needs review
FileSize
6.81 KB
6.81 KB

Updated patch. Based on #31, but also fixes the test failures (locally at least).

Not 100% sure if the best thing is to check for the "no results" text, but the other alternative that I came up with was checking for a count of hits wrapper divs, and that is definitely worse.

jhodgdon’s picture

Status: Needs review » Needs work

Looking good!

A few more minor things to fix, and sorry I didn't catch these in the last review:

a)core/modules/search/src/Controller/SearchController.php

+    // Create page title.
+    $build['#title'] = $plugin->suggestedTitle();
     $build['search_form'] = $this->entityFormBuilder()->getForm($entity, 'search');

I don't think the code comment is actually necessary. But if you want to add it, that's OK; however, since it doesn't apply to the $build['search_form'] line, there should be a blank line added before that line so it's clearer that the comment only applies to the $build['#title'] line.

b) core/modules/search/src/Plugin/SearchPluginBase.php

+      return $this->t('Search for @keywords', array('@keywords' => truncate_utf8($this->keywords, 180, TRUE, TRUE)));

In D8, truncate_utf8() is a wrapper on Unicode::truncate(), so we should probably directly use the Unicode method instead of the wrapper.

c) core/modules/search/src/Tests/SearchPageTextTest.php -- same as (b)

Otherwise, looks great! I applied the patch and tested it with some special characters etc. and it seems to be working fine in the UI as well. However:

d) I think maybe we should add some tests for this functionality, which don't involve using the trunctate_utf8() function explicitly in the test code, but instead try a few known cases and make sure they produce the desired output. My suggested test cases would be (and they should go into SearchPageTextTest.php):

1. Search for a complicated string like
dog cat "I like cats" abc>123 `something can't etc.
and verify that the page title is exactly this:
Search for dog cat "I like cats" abc>123 `something can't etc. | Drupal

2. Search for something long like
abcdef (repeated about 30 times)
and verify that the page title is
Search for abcdef (repeated exactly N times ) ... | Drupal

Actually I think maybe 180 characters is too long for the page title. Maybe truncate it to 60 characters? In which case that "abcdef" string in the title should be present, let's see, exactly 8 times, right? Because it's supposed to truncate on a word boundary. We should definitely have a test for that.

eiriksm’s picture

Status: Needs work » Needs review
FileSize
8.09 KB

a:
Agreed. This was from the reroll. I removed the comment, I don't think it's necessary either.

b:
Ah, also left over from the last patch of course. Fixed it now.

c:
I agree on the length. Added a couple of texts and a couple of extra tests as suggested. Hope it is not controversial to add some obscure references in places like these :p

I had to add a more "custom" comparison for the complicated string though, is that the best way to do it?

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

An interdiff file is helpful when you update a patch. Next time. :) See https://drupal.org/documentation/git/interdiff

Anyway... Thanks! This patch looks great to me. And... it only took 8 years to address this issue. :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 37: search_term_in_title-74562-37.patch, failed testing.

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Test bot was messed up...

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Is there an issue with have different titles for the same url? Search terms are now query parameters. Maybe we need to use a canonical url to link back to the search without results.

eiriksm’s picture

I am sorry, I tried to read the message several times, but I am not sure what you are asking, and what the "needs work" part consists of.

- This issue needs work because you are wondering if there is an issue for different titles with the same URL?
- This issue needs work because we need a URL to link to a search with 0 results? (from a search with more than 0 results?)

I'd be happy to look into it, but I do not understand what you are asking about.

jhodgdon’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

I don't understand @alexpott's comment either. All this patch does is make it so that if you go to, for example,
example.com/search/node?keys=foobar
you would get "Search for foobar" in the title.

You will not get two different titles for two different URLs, since the keywords are in the URL (as GET parameters).

And we have never had a link from a search results page back to the empty search page... not sure what is being asked for here. I think I'll just set it back to RTBC...

I guess we also needed an issue summary update, and since this does add to the Search pages plugin API, a change notice. So I wrote one:
https://www.drupal.org/node/2351405
(draft)

Hopefully all of this is more clear now...

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

example.com/search/node?keys=foo and example.com/search/node?keys=bar are not different URLs - they are the same url with different query parameters. I'm wondering if this will in anyway affect how google would rank such a page.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

This is what Google itself does. For instance:
https://www.google.com/search?client=ubuntu&channel=fs&q=hello+world&ie=...
The page title is "hello world - Google search".

Other sites with this convention:
http://www.rei.com/search?query=jacket -- page title: Results for "jacket" at REI
https://twitter.com/search?q=drupal&src=typd -- page title: drupal - Twitter search
...

Basically, this is how the entire industry does this: keywords are in a GET query parameter, and keywords are shown in the page title. Drupal should be doing the same thing. Can you find any examples of search pages that don't do this, actually?

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I'm not saying that we should not do this I'm asking if we should be adding a canonical link relationship - twitter does this - google and rei do not. Based on this and https://www.mattcutts.com/blog/canonical-link-tag/ I think we are good to go here.

Committed 7bcd170 and pushed to 8.0.x. Thanks!

+++ b/core/modules/search/src/Tests/SearchPageTextTest.php
@@ -30,23 +32,38 @@ function testSearchText() {
+    $this->assertTitle(t('Search') . ' | Drupal', 'Search page title is correct');
...
+    $this->assertTitle(t($title_source, array('@keywords' => Unicode::truncate($search_terms, 60, TRUE, TRUE))), 'Search page title is correct');
...
+    $this->assertTitle(t($title_source, array('@keywords' => 'Every word is like an unnecessary stain on silence and…')), 'Search page title is correct');
...
+    $this->assertEqual($actual_title, decode_entities(t($title_source, array('@keywords' => Unicode::truncate($search_terms, 60, TRUE, TRUE)))), 'Search page title is correct');

In future can we not add messages to assertTitle since the default message is far superior to the custom one. 'Search page title is correct' does not really help the developer at all.

  • alexpott committed 7bcd170 on 8.0.x
    Issue #74562 by naxoc, eiriksm, mikemiles86 | llizards: Added Show...
jhodgdon’s picture

Duly noted on the messages. Thanks!

Status: Fixed » Closed (fixed)

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

alexpott’s picture

Category: Feature request » Task

Changing the title of a page is not a feature. More a task.