Search admin menu entry uses "settings" -- UI-text standards violation

Should probably wait until #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit is finished to avoid unnecessary rerolls.

Other issue is taken care of.

Here's a 3-line patch, with screen shot of the relevant section of the admin/configure page. I realized the description on the Configuration page was also bogus, so fixed that too.

I am not willing to open up this discussion to the categories on admin/config -- if you want to open up that discussion, please do so in another issue.

Here's another patch that changes things to say "Configure search" instead of just "Search" , even though this makes the Search config page (while complying with our UI text standards) different from every other page on admin/configure. Is that OK or do you have objections to this as well?

I do not think that the team that decided on this structure for Administer really wants to have it broken down into even smaller categories than what it is. On the other hand, I think they just lumped a bunch of random stuff into "Search and metadata"... But I don't want to mess with it, at least on this issue. I also don't want to just "won't fix" this issue.

So, let's see...

What if we changed the link and page title to be "Search pages"? That way, you're in "Search and metadata" area, and you're configuring Search pages, as opposed to the meta-data stuff like URL aliases. That is mostly what that Search config page is for... that and the overall indexing settings, which we can make clear in the Description.

Would that be a viable solution? Here's a patch, just in case. :)

Yes, we should probably change the URL too... So it should be admin/config/search/pages ? That actually makes things about 1000% more rational. We'd have:

search.settings:
  path: '/admin/config/search/pages'
search.reindex_confirm:
  path: '/admin/config/search/pages/reindex'
# and this pattern for all the CRUD etc. operations on search pages:
search.add_type:
  path: '/admin/config/search/pages/add/{search_plugin_id}'
# ... for delete, edit, enable, etc.

I don't think we actually want to change the route machine name in:

--- a/core/modules/search/search.routing.yml
+++ b/core/modules/search/search.routing.yml
@@ -1,13 +1,13 @@
-search.settings:
-  path: '/admin/config/search/settings'
+search.pages:
+  path: '/admin/config/search/pages'

Just change the paths. That might fix some of the test failures?

yoroy: The point here is that what the page is configuring is "search pages". You can add a new search page, delete search pages, and configure search pages. That is what the config entities SearchPage that you are configuring are called in the UI, and that is what they actually are: individual pages where you can do searches. That is the bulk of the configuration that you can do at URL admin/config/search/settings ==> admin/config/search/pages in this patch. (There are also a few settings having to do with indexing on this config page.)

So the idea here is actually that, in line with other Config menu entries, we list on the page the noun of what we are configuring, which is "search pages" in this case.

You'll also notice in this patch that the menu entry in hook_menu_link_defaults becomes:

Search pages

Configure search pages and search indexing options.

and the URLs for the SearchPage config CRUD options make a lot more sense, as they are now:

-  path: '/admin/config/search/settings/manage/{search_page}/disable'
+  path: '/admin/config/search/pages/manage/{search_page}/disable'

I think this patch is good... I'm going to go ahead and mark it RTBC, although there may be some usability reviews and/or yoroy may still disagree and mark it back to needs work or needs review. Thanks!

I also made an issue summary outlining this.

Looks like the patch needs a reroll.

I also just noticed that the previous patch has a bunch of:

diff --git a/core/modules/search/search.routing.yml b/core/modules/search/search.routing.yml
old mode 100644
new mode 100755

in it. Please take those out in the reroll. We don't want to change the mode of files (making them executable etc.). Thanks!

Thanks! Back to RTBC then. Reroll is clean.

Thanks for the reroll!

The patch apply failure was due to hook_menu_link_defaults() vanishing from search.module. But it was replaced by file search.menu_links.yml.

So the changes in the #22 patch to the function search_menu_link_defaults():

@@ -148,9 +148,9 @@ function search_menu_link_defaults() {
     'type' => MENU_SUGGESTED_ITEM,
   );
   $links['search.settings'] = array(
-    'link_title' => 'Search settings',
+    'link_title' => 'Search pages',
     'parent' => 'system.admin_config_search',
-    'description' => 'Configure relevance settings for search and other indexing options.',
+    'description' => 'Configure search pages and search indexing options.',
     'route_name' => 'search.settings',
     'weight' => -10,
   );

now need to be made in search.menu_links.yml now. This is missing from the #25 patch.

Perfect, thanks!

I decided I should give this patch a manual test. So I applied the patch and did a "drush cr".

* The Configuration page looks good.
* Clicking through to admin/config/search/pages looks good.
* I tried various operations on the Content and User search pages:
- Edit - fine
- Set as default - fine
- Disable - fine
- Delete - fine

However:
- Enable and add - I got an exception:
Symfony\Component\Routing\Exception\RouteNotFoundException: Route "search.view_user_search" does not exist. in Drupal\Core\Routing\RouteProvider->getRouteByName() (line 150 of core/lib/Drupal/Core/Routing/RouteProvider.php).

Ah. This is a separate issue, not due to this patch.
#2229303: Enable/add of search pages leads to route not found exception

This patch is RTBC. Thanks!

(By the way, just to make sure, I just un-patched my site, did a drush cr, and verified that #2229303: Enable/add of search pages leads to route not found exception occurs without the patch as well. I was pretty sure it wasn't due to this patch, but now I actually tested it too.) (So this patch is still RTBC.)

27: 2156661-27.patch queued for re-testing.

That was some random test failure...

Reroll is good, thanks!