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>
Comment | File | Size | Author |
---|---|---|---|
#37 | search_term_in_title-74562-37.patch | 8.09 KB | eiriksm |
#35 | search_term_in_title-74562-35.patch | 6.81 KB | eiriksm |
#35 | search_term_in_title-74562-35.patch | 6.81 KB | eiriksm |
#31 | 74562-30.patch | 2.19 KB | naxoc |
#26 | interdiff-74562-23-26.txt | 1.48 KB | mikemiles86 |
Comments
Comment #1
magico CreditAttribution: magico commentedSeems a nice usability feature.
Comment #2
formicin CreditAttribution: formicin commentedDid anyone make it work? I tried to hack the module but I couldn't even located the "search" word which is the title.
Comment #3
birdmanx35 CreditAttribution: birdmanx35 commentedMoved to 6.0.
Comment #4
robertDouglass CreditAttribution: robertDouglass commentedPerfectly valid feature request, especially since we have search URLs that can be bookmarked.
Comment #5
robertDouglass CreditAttribution: robertDouglass commentedThe title currently comes from the parent menu item to the tabs:
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"?
Comment #6
David Lesieur CreditAttribution: David Lesieur commented"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.
Comment #7
naxoc CreditAttribution: naxoc commentedSomething like this in the search_view function does the job:
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?
Comment #8
naxoc CreditAttribution: naxoc commentedAdded tag
Comment #9
drupallogic CreditAttribution: drupallogic commentedcan you pls tell where to put this code ?
Comment #10
naxoc CreditAttribution: naxoc commentedHere is a patch with what I tested the page out with.
Comment #11
jhodgdonHmmm... 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?
Comment #12
cluke009 CreditAttribution: cluke009 commentedI've tested this under d7 and confirmed that it works
Comment #14
cluke009 CreditAttribution: cluke009 commentedI 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?
Comment #15
naxoc CreditAttribution: naxoc commentedThere 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).
Comment #17
naxoc CreditAttribution: naxoc commentedOuch. 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?
Comment #18
joachim CreditAttribution: joachim commentedMight 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 ...'"
Comment #19
naxoc CreditAttribution: naxoc commentedHere 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.
Comment #21
jhodgdonUpdating 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()?
Comment #22
mikemiles86Comment #23
mikemiles86I 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:
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.
Comment #25
jhodgdonThanks 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)
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)
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.
Comment #26
mikemiles86I 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.
Comment #27
jhodgdonThanks!
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
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.
Comment #28
jhodgdonOh, one other note:
The documentation for suggestedTitle() needs to mention that the title should be translated.
Also I think in the default implementation
this comment is a bit misleading because the keywords are not "passed" into this function. Probably it should say "If keywords have been set"?
Comment #30
naxoc CreditAttribution: naxoc commentedHere is a reroll. It's going to fail the tests, but I want to see which ones I need to look at.
Comment #31
naxoc CreditAttribution: naxoc commentedNo wait, HERE is the patch.
Comment #33
jhodgdonHi @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.
Comment #34
jhodgdonOh, one other thing:
The usual way to document this is to say "The translated suggested page title." not "run through t()".
Comment #35
eiriksmUpdated 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.
Comment #36
jhodgdonLooking 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
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
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.
Comment #37
eiriksma:
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?
Comment #38
jhodgdonAn 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. :)
Comment #41
jhodgdonTest bot was messed up...
Comment #42
alexpottIs 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.
Comment #43
eiriksmI 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.
Comment #44
jhodgdonI 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...
Comment #45
alexpottexample.com/search/node?keys=foo
andexample.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.Comment #46
jhodgdonThis 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?
Comment #47
alexpottI'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!
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.
Comment #49
jhodgdonDuly noted on the messages. Thanks!
Comment #51
alexpottChanging the title of a page is not a feature. More a task.