Problem/Motivation

From #2664830-150: Add search capability to help topics (ambermatz): It is jarring [i.e., a usability bug] to enter a help search query at admin/help in the Seven (or administrative theme), then have the search results show up in the default theme (e.g. Bartik), then click on a help topics result going back to Seven (admin theme). (This is when you have the experimental Help Topics module enabled.)

For the same reason, user and node searches should continue to display in the default theme, because you usually initiate a search from that theme, and if you click through to a content item or user profile in the search results, you will be in the default theme.

So, search page plugins need the ability to specify that their results should show in the admin theme, or not.

Proposed resolution

Add annotation to search page plugins to make them display in the admin theme or (if omitted, the default) the regular theme.

Remaining tasks

[Done] Make a patch, including an automated test.
[Done] Test manually.
[Done] Create a follow-up issue to use this new annotation for the Help Search plugin that is part of the experimental Help Topics module (patch has already been verified to work with this issue's patch)
#3087879: Use admin theme annotation for help topics search

User interface changes

Some search page plugins (namely the Help search provided by the experimental Help Topics module) will have display their results in the admin theme, while all others will continue to display in the regular theme.

API changes

Nothing changes for existing plugins, as the default is still to display in the default theme. Search page plugins that want their results to display in the admin theme can add the "use_admin_theme" annotation to the annotation header in the plugin class (a follow-up issue will make this one-line change to the Help Topics module).

Data model changes

None.

Release notes snippet

New feature added for Search Page plugins: They can add "use_admin_theme = TRUE" to their plugin annotation and their results will be displayed in the admin theme instead of the default theme. Existing plugins (without this annotation) will default to FALSE, so their results will be displayed in the default site theme.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon created an issue. See original summary.

jhodgdon’s picture

Issue tags: +Usability
andypost’s picture

I see that is like block (search box) should control is the theme should be admin or not (basically inherit theme to search results)

andypost’s picture

Issue tags: +Accessibility

I bet the theme change affects search within help

jhodgdon’s picture

I don't think the block should control the theme. I think the SearchPage plugin should control the theme. There can be many many blocks on a page, and a block can be displayed all over the site... But what we want is that when you submit the search form, the *results* from the SearchPage plugin are displayed in the admin or non-admin theme (so HelpSearch would choose Admin, and NodeSearch and UserSearch would choose non-admin).

It could either be a configuration option on the search page config entity, or something built into the SearchPage annotation in the SearchPage plugin class docs header. I think it's fine if it is annotation actually. It doesn't really need to be configurable by the admin.

So if we just added something to HelpSearch annotation saying

  * use_admin_theme = TRUE,

or something like that?

andypost’s picture

Assigned: Unassigned » andypost

Working on annotation

andypost’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
3.42 KB

Initial working patch, would be great to file follow-up to extract route boilerplate definition to protected method

andypost’s picture

Assigned: andypost » Unassigned

Looking for ideas how to test current theme...

jhodgdon’s picture

I found a test. Take a look at
core/modules/system/tests/src/Functional/System/ThemeTest.php
in the testAdministrationTheme() method.

It looks like they are going to admin/config and looking for a raw string in the page source like 'core/themes/seven' (for pages that should have admin theme) or 'core/themes/classy' (for pages that shouldn't). They've set Classy as the default theme and Seven as the admin theme.

That should help...

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

Version: 8.9.x-dev » 8.8.x-dev
Issue tags: -Needs tests
FileSize
1.97 KB
5.39 KB

Here's a test and I'm sure it 8.8 target as bugfix

jhodgdon’s picture

Status: Needs review » Needs work

The test looks perfect to me! The code is nearly perfect (see nitpicks below).

The only problem is that for now, everything that mentions Help Topics needs to be under core/modules/help_topics, and also I believe they want patches/issues separated out into Help Topics and Not Help Topics, so that all the commits are cleanly in or out of help topics.

So... We probably need to split this issue into two pieces:
a) search module issue to add the annotation and code, and test using NodeSearch, UserSearch,, and SearchExtraTypeSearch.php
b) Help topics issue to add the annotation to help topics, and a few lines added to an existing Help Search test to verify the admin theme.

Code nitpicks:

a)

+++ b/core/modules/search/src/Plugin/SearchInterface.php
@@ -145,4 +145,12 @@ public function searchFormAlter(array &$form, FormStateInterface $form_state);
    */
   public function buildSearchUrlQuery(FormStateInterface $form_state);
 
+  /**
+   * Whether the search results should be displayed in admin theme.
+   *
+   * @return bool
+   *   When TRUE to display search results in the admin theme, FALSE by default.
+   */
+  public function isUsingAdminTheme();

First line: Returns whether or not search results should be displayed in the admin theme.
Return docs: TRUE if search results should be displayed in the admin theme, and FALSE otherwise.
Method name: I would call it useAdminTheme() instead of isUsingAdminTheme().

b) Let's also have NodeSearch be part of the test.

andypost’s picture

Here's improved test, also used to rename method to needsAdminTheme() and use_admin_theme annotation to request admin theme from plugin definition

jhodgdon’s picture

OK, this looks perfect for the main patch, with tests. I am +1 for RTBC based on looking at the patch, including its tests and test results, but I haven't actually tested it manually myself.

So... We still need to:
- Update the issue summary
- Write a change record
- Test manually by applying both the main patch and the follow-up patch, and verify that Node and User search go to the regular theme and Help search goes to the admin theme
- File a follow-up issue to make HelpSearch use the new functionality, and with the follow-up patch here.

I can do all of the above later today, although I suggest @andypost make the follow-up issue and put his patch there, since it is his patch.

andypost’s picture

I will file followup in 3-4 hours

jhodgdon’s picture

Title: Help search results should display in admin theme » Search results pages should be in the correct theme
Component: help.module » search.module
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +blocker

I have manually tested the patch above -- applied both the main patch and the -do-not-test patch, and I verified that when you search Nodes or Users, the results show in the main site theme, but if you search Help, the results show in the admin theme. This is what we want.

So this issue is now in the search module. Updating the user summary and title.

Also created a change record.
https://www.drupal.org/node/3087853

I think this is ready to go, aside from the follow-up issue.

andypost’s picture

jhodgdon’s picture

Issue summary: View changes

Great, updating summary. Thanks!

Amber Himes Matz’s picture

FWIW, I also manually tested the patches in #13 and works for me too. +1 on the RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/search/src/Plugin/SearchInterface.php
    @@ -145,4 +145,15 @@ public function searchFormAlter(array &$form, FormStateInterface $form_state);
    +  /**
    +   * Returns whether or not search results should be displayed in admin theme.
    +   *
    +   * @return bool
    +   *   TRUE if search results should be displayed in the admin theme, and FALSE
    +   *   otherwise.
    +   *
    +   * @see \Drupal\search\Annotation\SearchPlugin::$use_admin_theme
    +   */
    +  public function needsAdminTheme();
    

    We need to account BC - are we saying this is okay because of the base class. I think I agree but a +1 from a release manager would be a good thing.

  2. +++ b/core/modules/search/tests/src/Functional/SearchAdminThemeTest.php
    @@ -0,0 +1,102 @@
    +    $repository = \Drupal::service('search.search_page_repository');
    +    $pages = $repository->getActiveSearchPages();
    +    // Test default configured pages.
    +    $default_config = [
    +      'node_search',
    +      'search_extra_type_search',
    +      'user_search',
    +    ];
    +    foreach ($pages as $page) {
    +      if (in_array($page->id(), $default_config, TRUE)) {
    +        $plugin = $page->getPlugin();
    +        $path = 'search/' . $page->getPath();
    +        $this->drupalGet($path);
    +        $session = $this->assertSession();
    +        // 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);
    +        $is_admin = $plugin->needsAdminTheme();
    +        $this->assertAdminTheme($is_admin);
    +        $this->drupalGet($path_help);
    +        $this->assertAdminTheme($is_admin);
    +      }
    +    }
    +  }
    

    This test is prone to false positives and could be more readable. Firstly we need to assert that $pages is not empty. Secondly we should create a method that takes the argument $plugin and $use_admin_theme and not loop. If if (in_array($page->id(), $default_config, TRUE)) { is always false this test is green.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
6.31 KB

Here's #20.2 addressed. The test is not more explicit and will fail is $pages is empty or one of the list of things to test does not exist.

jhodgdon’s picture

Status: Needs review » Needs work

Yeah, right -- I don't think we can add functions to an interface. That would break BC, as a hypothetical contrib module with a class that said it implements the interface would break, if it didn't happen to extend the base class. Setting to Needs Work for that.

Interdiff in #21 looks good.

jhodgdon’s picture

So, this patch is at Needs Work due to

+++ b/core/modules/search/src/Plugin/SearchInterface.php
@@ -145,4 +145,15 @@ public function searchFormAlter(array &$form, FormStateInterface $form_state);
    */
   public function buildSearchUrlQuery(FormStateInterface $form_state);
 
+  /**
+   * Returns whether or not search results should be displayed in admin theme.
+   *
+   * @return bool
+   *   TRUE if search results should be displayed in the admin theme, and FALSE
+   *   otherwise.
+   *
+   * @see \Drupal\search\Annotation\SearchPlugin::$use_admin_theme
+   */
+  public function needsAdminTheme();
+

We can't add functions to an interface, as that breaks BC and would cause any contrib modules implementing this interface to be broken PHP. This needs to be done in another way.

Other than that, this looks good.

andypost’s picture

Version: 8.8.x-dev » 8.9.x-dev

BC is blocker - sounds like extra interface to add, but then it leads to 9.1 as feature yo stable module... Then help topics needs to use workaround

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

Actually, our BC policy for interfaces (as I learned on a different issue recently) is here:
https://www.drupal.org/core/d8-bc-policy#interfaces

Interfaces within non-experimental, non-test modules not tagged with either @api or @internal:

Interfaces that are not tagged with either @api or @internal can be safely used as type hints. No methods will be changed or removed from these interface in a breaking way. However, we reserve the ability to add methods to these interfaces in minor releases to support new features.

This interface is not tagged with either @api or @internal, so even though it seems like a BC break, we can actually add a method. We also have a base class, so it probably won't break actual implementations (assuming they're using the base class, as suggested later in that section). So... I think we can proceed on this.

Given that, I took another look at the patch on #21. I think it looks really good! It has a good test that passes, the API docs are clear, and it's actually fairly simple.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 3086794-21.patch, failed testing. View results

jhodgdon’s picture

Test fail does need to be addressed though...

andypost’s picture

Status: Needs work » Needs review
FileSize
612 bytes
6.38 KB

Fixed new test requirements

The last submitted patch, 21: 3086794-21.patch, failed testing. View results

andypost’s picture

Title: Search results pages should be in the correct theme » Search results pages plugins could display results in administrative theme

Changed title to point current implementation

PS: tests now pass

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good! Tests now pass. Code looks good. I've verified that it's OK by BC policy to add methods to an interface that is neither @api nor @internal, and we do have a base class to aid people implementing the interface.

I also just manually tested this again by adding

use_admin_theme = TRUE

to the Help Topics Search plugin in my dev site, and when I search help, it shows results in the Seven theme, but when I search content and users, they show up in Bartik, which is correct. (This was the same as the patch in
#3087879: Use admin theme annotation for help topics search
which is the follow-up issue.)

So, this looks good! Let's ship it.

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/search/src/Plugin/SearchInterface.php
@@ -145,4 +145,15 @@ public function searchFormAlter(array &$form, FormStateInterface $form_state);
+  public function needsAdminTheme();

Why not ::usesAdminTheme() to match the property?

jhodgdon’s picture

That does sound like a good idea. @andypost want to patch and I'll review?

andypost’s picture

FileSize
2.4 KB
6.38 KB

Renamed the method ++

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Tests still pass with the rename, and all agree it is a better name. Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: 3086794-34.patch, failed testing. View results

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated test failure in QuickEdit.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

The patch fails on 9.1.x with:

Search.Drupal\Tests\search\Functional\SearchAdminThemeTest
✗	
Drupal\Tests\search\Functional\SearchAdminThemeTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-1184.xml:
PHPunit Test failed to complete; Error: PHPUnit 8.5.3 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\search\Functional\SearchAdminThemeTest
.                                                                   1 / 1 (100%)

Time: 9.94 seconds, Memory: 4.00 MB

OK (1 test, 19 assertions)

HTML output was generated

Remaining self deprecation notices (1)

  1x: Declaring ::setUp without a void return typehint in Drupal\Tests\search\Functional\SearchAdminThemeTest is deprecated in drupal:9.0.0. Typehinting will be required before drupal:10.0.0. See https://www.drupal.org/node/3114724
    1x in DrupalListener::startTest from Drupal\Tests\Listeners
andypost’s picture

Status: Needs work » Needs review
FileSize
607 bytes
6.38 KB

Fix #39

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the quick fix! Looks good to me. Tests pass. No Coder errors. One line change to previously reviewed patch, so back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I discussed this with @catch. Because this is adding a method to an interface we should only add this to 9.1.x. We can add this method under the 1-to-1 interface to base class rule.

Committed 40d62a6 and pushed to 9.1.x. Thanks!

  • alexpott committed 40d62a6 on 9.1.x
    Issue #3086794 by andypost, alexpott, jhodgdon, Amber Himes Matz, catch...

Status: Fixed » Closed (fixed)

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