Use admin theme annotation for help topics search

Assuming that the patch from #13 on
#3086794: Search results pages plugins could display results in administrative theme
gets committed, I have already reviewed and tested this patch, so am +1 on marking this RTBC immediately after that gets committed.

We'll need to retest if a different patch gets committed though.

Parent not actually committed yet, but we can still write tests. Keep in mind the test for this one has to stay in help topics module. Probably could add it to the existing Help Search test, as it would only add a line or two to that test?

This looks very good! I am +1 on RTBC, although I have not tested this manually to verify that help search does stay in the administration theme, so adding a "Needs manual testing" tag. (I believe the test is accurate, but I still think we should test it manually.)

One other idea: I think that when we get to the point of merging Help Topics into Help, we should also remove this separate test class, and add the help search to the base class SearchAdminThemeTest instead? We cannot do that now, because we don't want Experimental Module code mixed with the base code.

If that sounds like a good idea, then we need to add this to the final merge plan on
#3037230: Finalize the merge of Help Topics into Help
and/or make a sub-issue.

Thoughts?

I'm only +1 on RTBC when the automated tests pass, and when someone does a manual test. Any thoughts on #10?

I just did a manual test, and the patch works as expected: searching Help results are shown in the admin theme.

RTBC assuming that the automated tests pass. We should still figure out about #10.

The code in this latest patch is exactly the same 1-line change as in the patch I tested manually, so that should still work.

The test code in help_topics module looks good -- very clean.

The additional/modified testing code in the search module is very nice and clearly written. It makes the code more reusable, and also adds tests for what happens if the user does not have permission to use the admin theme and the search plugin is supposed to use the admin theme (in which case it uses the default theme).

The only thing I would change is this documentation on the new method in the base test class:

+ * @return array
+ * Keyed array by $page_id of supported plugin and array of permissions to
+ * access the page content and expected admin theme requirement to allow
+ * the plugin to display results.
+ *
+ * @see \Drupal\search\SearchPageAccessControlHandler::checkAccess()
+ * @see \Drupal\search\Plugin\SearchInterface::usesAdminTheme()
+ */
+ protected function getSearchPagePlugins() {


I think the documentation should be:

Array whose keys are search page plugin IDs to test, and whose values are an associative array with two keys:
- use_admin_theme: (optional) TRUE if the admin theme should be used to display search results (default is FALSE).
- permissions: (optional) Array of permissions required for searching using this type of page. If omitted, the test verifies that this search plugin type has no access check.

The last line of that documentation needs to be wrapped at 80 characters. Other than that, I think this is RTBC, unless you wanted to refactor the test in some way?

+   *   - permissions: (optional) Array of permissions required for searching
+   *     using this type of page. If omitted, the test verifies that this search plugin type has no access check.

OK, I think this is ready to go. Has been manually tested by me, the code looks good (1 line change), the test is comprehensive and the base test was improved.

Also added follow-up issue as part of roadmap #3144933: Add help_search plugin to the base admin theme test

1. +++ b/core/modules/help_topics/tests/src/Functional/SearchAdminThemeTest.php
   @@ -0,0 +1,33 @@
   +class SearchAdminThemeTest extends SearchAdminThemeTestBase {
   ...
   +        'permissions' => ['access administration pages'],
   

   Let's not call this SearchAdminThemeTest - then you don't need to rename the other class to base class.

   Also this get's a bit strange because we're adding the ['access administration pages'] permission but the underlying test adds and removes a role with that permission. I think this test inheritance is making things more complex that it should. I don't really support DRY principles in test code. It often makes it harder to understand what is actually being tested.

2. +++ b/core/modules/search/tests/src/Functional/SearchAdminThemeTest.php
   @@ -65,25 +63,47 @@ public function testSearchUsingAdminTheme() {
   +    $role_admin_theme_access = $this->drupalCreateRole(['view the administration theme']);
   +    $account = $this->loggedInUser;
   ...
   +      if (isset($expected['permissions'])) {
   +        $this->assertInstanceOf(AccessibleInterface::class, $plugin);
   +        $role_plugin_access = $this->drupalCreateRole($expected['permissions']);
   +        $account->addRole($role_plugin_access);
   +      }
   +      else {
   +        $this->assertNotInstanceOf(AccessibleInterface::class, $plugin);
   +        $role_plugin_access = FALSE;
   +      }
   +      foreach ([TRUE, FALSE] as $admin_theme_access_allowed) {
   +        if ($admin_theme_access_allowed) {
   +          $account->addRole($role_admin_theme_access);
   +        }
   +        else {
   +          $account->removeRole($role_admin_theme_access);
   +        }
   +        $account->save();
   ...
   +      if ($role_plugin_access) {
   +        // Remove extra permissions to test next plugin with different set of
   +        // required permission.
   +        $account->removeRole($role_plugin_access);
   +      }
   

   Let's create a new account each time with the correct set of permissions. It'll be neater and less code and less to go wrong if this test is extended.

The two errors I see in the test results are supposed to be verifying that the user can see the "more help with searching" links.

That's weird... those links have the exact same permissions as the parent search pages.

While looking at this, by the way, I noticed:

+        // Make sure help plugin rendered help link.
+        // @todo Test label in https://www.drupal.org/node/3086795

That @todo should be removed -- that issue is done and the label was tested.
#3086795: "Search help" link in search form is ambiguous and confusing

Actually I don't know why we are testing that this link is there in this patch. It is tested in that other issue with a clickLink. I think we can take these lines out of the test:

+        // Make sure help plugin rendered help link.
+        // @todo Test label in https://www.drupal.org/node/3086795
+        $path_help = $path . '/help';
+        $session->linkByHrefExists($path_help);

The point of this test is to verify that if you go to those pages, the admin theme is used or not used. I do not know why these lines are failing in this test, but I don't think we need to have them here and the other test verifies you can click the link.

Oh, that's not good.

I agree we need to test that the search help page is rendered in the correct theme.

What I don't think we need to verify is that the search help link is present on the page. We could check that the response code when visiting both search and search help is 200.

Maybe to get around the 403 problem you could make all the needed user accounts in the setUp() function and then clear the cache? And make separate accounts with and without the role access. Cache stuff is annoying in tests often, and isn't that relevant when it comes to equating test results with how things actually behave on a real site.

Couldn't the test have a similar for loop in the setUp that does something like this:

foreach ($this->getSearchPagePlugins() as $page_id => $expected) {
  $this->users[$page_id]['admin'] = ; // create user who can search and use admin theme
  $this->users[$page_id]['no admin'] = ; // create user who can search but not use admin theme
}
// clear cache

Then in the main test function, it could reference these users? I think it would still be reusable that way..

That looks like a good course of action. You are right, the Search module is already verifying that search plugins that return TRUE for usesAdminTheme() actually use the admin theme for the search page and the help page. We also already have an issue to add HelpSearch into the existing Search admin test when we get out of Experimental for Help Topics, so we can integrate this plugin into that test then.

Assuming the tests pass, I have already manually tested the code change (see #24, and there has not been a change in the code since then).

So... I am +1 for RTBC assuming the tests pass.

Sorry, I forgot to come back here and set this to RTBC. Had already reviewed the code, and manually tested in #24.

Committed 33c16bf and pushed to 9.1.x. Thanks!

diff --git a/core/modules/help_topics/tests/src/Kernel/HelpSearchPluginTest.php b/core/modules/help_topics/tests/src/Kernel/HelpSearchPluginTest.php
index 297497a30e..eaea9fe2fc 100644
--- a/core/modules/help_topics/tests/src/Kernel/HelpSearchPluginTest.php
+++ b/core/modules/help_topics/tests/src/Kernel/HelpSearchPluginTest.php
@@ -7,7 +7,7 @@
 use Drupal\search\Plugin\SearchIndexingInterface;
 
 /**
- * Tests search plugin behaviours.
+ * Tests search plugin behaviors.
  *
  * @group help_topics
  *

Changed spelling to american spelling on commit.