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.
Comment | File | Size | Author |
---|---|---|---|
#40 | 3086794-40.patch | 6.38 KB | andypost |
#40 | interdiff.txt | 607 bytes | andypost |
Comments
Comment #2
jhodgdonComment #3
andypostI see that is like block (search box) should control is the theme should be admin or not (basically inherit theme to search results)
Comment #4
andypostI bet the theme change affects search within help
Comment #5
jhodgdonI 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
or something like that?
Comment #6
andypostWorking on annotation
Comment #7
andypostInitial working patch, would be great to file follow-up to extract route boilerplate definition to protected method
Comment #8
andypostLooking for ideas how to test current theme...
Comment #9
jhodgdonI 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...
Comment #11
andypostHere's a test and I'm sure it 8.8 target as bugfix
Comment #12
jhodgdonThe 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)
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.
Comment #13
andypostHere's improved test, also used to rename method to
needsAdminTheme()
anduse_admin_theme
annotation to request admin theme from plugin definitionComment #14
jhodgdonOK, 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.
Comment #15
andypostI will file followup in 3-4 hours
Comment #16
jhodgdonI 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.
Comment #17
andypostFiled follow-up #3087879: Use admin theme annotation for help topics search
Comment #18
jhodgdonGreat, updating summary. Thanks!
Comment #19
Amber Himes MatzFWIW, I also manually tested the patches in #13 and works for me too. +1 on the RTBC.
Comment #20
alexpottWe 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.
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.Comment #21
alexpottHere'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.
Comment #22
jhodgdonYeah, 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.
Comment #23
jhodgdonSo, this patch is at Needs Work due to
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.
Comment #24
andypostBC 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
Comment #25
jhodgdonActually, our BC policy for interfaces (as I learned on a different issue recently) is here:
https://www.drupal.org/core/d8-bc-policy#interfaces
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.
Comment #27
jhodgdonTest fail does need to be addressed though...
Comment #28
andypostFixed new test requirements
Comment #30
andypostChanged title to point current implementation
PS: tests now pass
Comment #31
jhodgdonLooks 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
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.
Comment #32
catchWhy not
::usesAdminTheme()
to match the property?Comment #33
jhodgdonThat does sound like a good idea. @andypost want to patch and I'll review?
Comment #34
andypostRenamed the method ++
Comment #35
jhodgdonThanks! Tests still pass with the rename, and all agree it is a better name. Back to RTBC.
Comment #37
jhodgdonUnrelated test failure in QuickEdit.
Comment #39
xjmThe patch fails on 9.1.x with:
Comment #40
andypostFix #39
Comment #41
jhodgdonThanks for the quick fix! Looks good to me. Tests pass. No Coder errors. One line change to previously reviewed patch, so back to RTBC.
Comment #42
alexpottI 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!