Problem/Motivation

On #2003482: Convert hook_search_info to plugin system, we converted the Search module to use plugins. However, the way that was set up, modules can each provide one plugin. For better flexibility, we should instead allow people to configure multiple instances of each search plugin type provided by a module. Which means a ConfigEntity.

Proposed resolution

Allow users to define "search pages", each of which is based on a particular Search plugin type provided by a module. The user interface should be similar to this:
proposed UI for search plugin config

When defining a search page, you will be able to set up plugin-specific configuration options (such as the node ranking settings for the Node plugin). Also, you'll choose a title (used as the tab title and page title), a URL suffix (you will search at search/(suffix)), and a machine name (used in the search index).

Remaining tasks

Viable patch, with tests, is in progress.
See #92.

User interface changes

The administration of Search will change:
1. The Search Settings page will have all of its plugin-type-specific settings removed from the page and put into plugin-specific configuration pages instead.
2. The Plugins section on the Search Settings page will be replaced by a Search Pages section similar to the UI mockup above.
3. You will be able to have multiple copies of each search plugin, each as a "search page".

API changes

- A few methods on the Search Plugin interfaces will be changing (to be determined), around the area of configuration.
- Search plugin annotation will need to change a bit, since the URL is not per-plugin-type any more.
- Plugins will need to be written so that they do not hard-wire their plugin IDs or URLs into queries, redirects, and such.

CommentFileSizeAuthor
#109 interdiff-109.txt1.41 KBianthomas_uk
#109 search-2042807-109.patch150.58 KBianthomas_uk
#107 content-ranking-config.png23.99 KBianthomas_uk
#107 interdiff-107.txt3.65 KBianthomas_uk
#107 search-2042807-107.patch150.59 KBianthomas_uk
#106 search-2042807-106.patch151.94 KBtim.plunkett
#106 interdiff.txt3.59 KBtim.plunkett
#104 interdiff.txt1.07 KBtim.plunkett
#104 search-2042807-104.patch151.8 KBtim.plunkett
#102 search-2042807-102.patch151.63 KBtim.plunkett
#97 search-2042807-97.patch150.85 KBtim.plunkett
#94 search-2042807-94.patch151.03 KBtim.plunkett
#94 interdiff.txt8.13 KBtim.plunkett
#93 interdiff-93-indexes.txt951 bytesianthomas_uk
#93 interdiff-93-config.txt4.14 KBianthomas_uk
#92 interdiff-92.txt5.92 KBianthomas_uk
#90 interdiff.txt1.77 KBtim.plunkett
#90 search-2042807-90.patch148.18 KBtim.plunkett
#89 search-docs-interdiff.txt7.93 KBjhodgdon
#86 search-2042807-86.patch144.95 KBtim.plunkett
#85 interdiff.txt4.03 KBtim.plunkett
#85 search-2042807-85.patch145 KBtim.plunkett
#83 search-2042807-83.patch145.03 KBtim.plunkett
#83 interdiff.txt38.76 KBtim.plunkett
#80 search-pages-default.png24.89 KBianthomas_uk
#77 interdiff.txt14.47 KBtim.plunkett
#77 search-2042807-77.patch135.53 KBtim.plunkett
#73 interdiff.txt7.34 KBtim.plunkett
#73 search-2042807-73.patch133.79 KBtim.plunkett
#67 interdiff.txt27.44 KBtim.plunkett
#67 search-2042807-67.patch129.88 KBtim.plunkett
#63 search-with-tabledrag.png45.76 KBtim.plunkett
#62 interdiff.txt14.88 KBtim.plunkett
#62 search-2042807-61.patch120.66 KBtim.plunkett
#58 interdiff.txt12.29 KBtim.plunkett
#58 search-2042807-58.patch117.94 KBtim.plunkett
#55 interdiff.txt1.54 KBtim.plunkett
#55 search-2042807-55.patch118.8 KBtim.plunkett
#52 search-page-ui.png34.11 KBtim.plunkett
#51 search-2042807-51.patch118.27 KBtim.plunkett
#49 search-2042807-49.patch117.82 KBtim.plunkett
#44 search-2042807-44.patch105.97 KBianthomas_uk
#44 search-settings-node.png27.23 KBianthomas_uk
#44 search-settings.png65.42 KBianthomas_uk
#43 search-2042807-43.patch106.67 KBtim.plunkett
#41 search-2042807-41.patch97.64 KBtim.plunkett
#40 search-plugins-ui-40.png42.82 KBjhodgdon
#38 search-2042807-37.patch87.86 KBtim.plunkett
#36 search-plugins-ui-36.png42.48 KBjhodgdon
#35 search-plugins-ui-35.epz_.tgz4.34 KBjhodgdon
#35 search-plugins-ui-35.png36.8 KBjhodgdon
#35 currentplugins.png19.23 KBjhodgdon
#28 search-2003482-104.patch125.08 KBpwolanin
#27 2003482-104.patch123.34 KBpwolanin
#26 2003482-104.patch123.34 KBpwolanin
#25 search-2042807-25.patch128.85 KBpwolanin
#23 search-2042807-23.patch139.07 KBpwolanin
#23 2042807-22-23.increment.txt3.84 KBpwolanin
#22 search-2042807-22.patch137.3 KBpwolanin
#20 search-2042807-20.patch137.28 KBpwolanin
#20 search-2042807-19-20.increment.txt1.37 KBpwolanin
#19 2042807-19.patch137.27 KBpwolanin
#12 search-2042807-12.patch139.31 KBtim.plunkett
#12 interdiff.txt24.66 KBtim.plunkett
#10 diff-from-2003482.txt74.23 KBtim.plunkett
#9 search-2042807-9.patch135.87 KBtim.plunkett
#9 interdiff.txt14.99 KBtim.plunkett
#8 search-2042807-8.patch135.42 KBtim.plunkett
#8 interdiff.txt17.59 KBtim.plunkett
#6 search-2042807-6.patch129.57 KBtim.plunkett
#6 interdiff.txt3.14 KBtim.plunkett
#4 search-list.png159.91 KBtim.plunkett
#4 search-2042807-4.patch130.32 KBtim.plunkett
#4 interdiff.txt66.68 KBtim.plunkett
#1 search-2042807-1.patch113.33 KBtim.plunkett
#1 interdiff.txt35.53 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
35.53 KB
113.33 KB

Pushed up to https://drupal.org/sandbox/tim.plunkett/1698392, branch 2042807-search

tim.plunkett’s picture

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
66.68 KB
130.32 KB
159.91 KB

Okay, here we go! This should pass most tests.

Interdiff is against the other issue.

I've removed the "share code with Actions" part for now.

search listing

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.14 KB
129.57 KB

This might be green, but I have refactoring/injection to do

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
17.59 KB
135.42 KB
tim.plunkett’s picture

Issue tags: -Approved API change
FileSize
14.99 KB
135.87 KB

Okay, this is truly ready for review.

tim.plunkett’s picture

FileSize
74.23 KB

Here's a diff against the other issue.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
24.66 KB
139.31 KB

Okay, I took some of jhodgdon's good feedback in IRC.

I added the extra functionality I wanted and some validation.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Upgrade path time...

pwolanin’s picture

Issue tags: -Plugin-conversion

There is an outstanding issue I think to convert node's variables to config?

Also, I assume the tests are still looking for the old ui?

tim.plunkett’s picture

I updated most of the UI tests. I'll look into this later tonight.

pwolanin’s picture

Issue tags: +Plugin-conversion

oops, adding back tag

pwolanin’s picture

Bah, patch doesn't apply

pwolanin’s picture

FileSize
137.27 KB

This is my re-roll attempt, but it's not finding the plugins.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
137.28 KB
pwolanin’s picture

Status: Needs work » Needs review
FileSize
137.3 KB

fixed conflict.

pwolanin’s picture

Fixed test fails (some due to a hunk of conflict I missed in #19)

pwolanin’s picture

FileSize
128.85 KB

Removes a stray .rej file

however, I'm not sure this approach really makes sense, or at least I cannot wrap my head around it. E.g. For user module there should probably not be more than one.

pwolanin’s picture

FileSize
123.34 KB

Here's a patch just to key per plugin, rather than using config entities. Will post back to the other issue if tests pass.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
123.34 KB

oh, bah - forgot to set status - that patch had local test fails anyhow.

pwolanin’s picture

FileSize
125.08 KB

bah, posted the wrong patch

jibran’s picture

Please post do not test patch without the code of #2003482: Convert hook_search_info to plugin system it makes review easy.

pwolanin’s picture

Status: Needs review » Closed (won't fix)

Marking this "won't fix" so we can just get the other issue in.

xjm’s picture

Status: Closed (won't fix) » Closed (duplicate)

Better status.

alexpott’s picture

Issue summary: View changes
Status: Closed (duplicate) » Needs work
Issue tags: +beta blocker
Related issues: +#1831632: Convert node_rank_ $rank variables to use configuration system

After discussing search plugin configuration in #2132649: Inject a configuration into SearchPluginBase and #1831632: Convert node_rank_ $rank variables to use configuration system and in IRC with @timplunkett I think we should do this because (a) it is consistent (b) we can support the following use-case:

let's say you had a search plugin that could restrict by content type, and you wanted to have more than one running at a time

tim.plunkett’s picture

Title: Convert hook_search_info to ConfigEntity » Convert search plugins to use a ConfigEntity and a PluginBag

This will be interesting to try and salvage the patch, I might have to just start over.

pwolanin’s picture

@alexpott - the UI we got out of trying this before was very confusing.

jhodgdon’s picture

OK, so we need a UI. I am not a UI expert, but here's something to at least start the discussion.

So... What we have now on the Search Settings page is this:
screen shot of Search Settings page, plugins section

And then individual search plugins are allowed to add stuff to the Search Settings page.

What we need instead is something maybe like this:
proposed plugin section
(I created this with the Pencil plugin for Firefox, so I've also attached the zipped Pencil source file in case someone wants to edit this.)

My idea is this. In the Plugins section, you'll have a drop-down list of the plugins provided by various modules. You choose one and click the "Add new plugin" button.

This takes you to a plugin-specific configuration page. For instance, for Node search, you would choose the minimum word size and the ranking settings (I didn't make a mockup of that page). And you would assign it a name of your choice.

Once you click Save, the plugin you configured would show up (with the name you chose) in the Plugins table below. You could click "edit" to return to the config screen, and either enable/disable to change the status. And you would then choose which search plugin is the default, from that table.

Does that make any sense?

jhodgdon’s picture

FileSize
42.48 KB

I see this is essentially the same UI as previously proposed in #4... Here's an updated proposal...

And to clarify, this would replace the existing "Plugins" section, and also any existing plugin-specific settings (like node rank settings) would be moved off Search Settings and into the individual Configure pages.
search plugins mockup

jhodgdon’s picture

And it would be a big +1 if this also had row weights, so that you could order the plugin tabs. There is actually a separate issue about that though.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
87.86 KB

So far I just rewrote the last patch I had, which looks like #4. I'll work on fixing test fails and switching to that select list in #36 tomorrow.

jhodgdon’s picture

Issue summary: View changes
Related issues: +#2003482: Convert hook_search_info to plugin system, +#2033383: Provide a default plugin bag
FileSize
42.82 KB

After a discussion in IRC, timplunkett, pwolanin, and I agreed that a better UI name for the plugin instances would be "search pages" (good thinking pwolanin!), so I've updated the UI sketch.

And the issue summary was kind of missing, so I've added one.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
97.64 KB

Okay I renamed the entity to SearchPage, but there are still some doc lines to be cleaned up.
I also switched from the link/page to select/button for adding a new one.
Unfortunately we'll need some CSS to float the button, and the new button styling makes this whole thing ridiculous.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
106.67 KB

Rerolled, finished renaming everything, injected all the things, and wrote a bunch more tests for the UI.

ianthomas_uk’s picture

FileSize
65.42 KB
27.23 KB
105.97 KB

This has conflicted with #1559244: Clean up search settings page, which made several changes to \Drupal\search\Form\SearchSettingsForm.

The attached patch is a reroll to take into account that this file has changed, but we still need to review that issue and see if any of the changes should to be made to the new UI.

I've also attached some screenshots of the new UI.

tim.plunkett’s picture

I have this in a long running sandbox, so without an interdiff that last patch is useless. Also, it fails...

ianthomas_uk’s picture

#43 and #44 are identical except for the contents of SearchSettingsForm, which both of them delete in full. It's probably easiest for you to just merge in 8.x and delete SearchSettingsForm after doing so.

I don't understand why it failed though, and I can't look into that at the moment.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
117.82 KB

Got that one reverted and fixed a bunch more stuff.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +API change
Related issues: +#2155635: Allow plugin managers to opt in to cache clear during module install
FileSize
118.27 KB

Okay! This version of the patch is actually ready for review.

A close review would show that SearchForm (now SearchPageForm) has lost is $action and $prompt params.
Interestingly enough, those have been dead code since D5, #40934: remove admin/content/search - it's broken.

tim.plunkett’s picture

FileSize
34.11 KB

By the way, I have this in a sandbox (ignore my commit messages http://drupalcode.org/sandbox/tim.plunkett/1698392.git/shortlog/refs/hea...)

So please refrain from helpful rerolling, or at the least provide an interdiff.

Here's a screenshot!

tim.plunkett’s picture

FileSize
118.8 KB
1.54 KB

Whoops. Yay for test coverage catching refactoring mistakes.

jhodgdon’s picture

Wow, that's a big patch. :)

I tried out this patch today, in a new install (couldn't get the old one reset etc. so had to wipe db out and start over, probably a good thing anyway).

A few thoughts on the user experience -- I installed with the Standard install profile by the way:

a) On the Search Settings page, could we make the URL column into links to the search pages? I guess that didn't make it into my UI mockup.

b) On the Search Settings page, it is not clear to me what the radios in the left column (for choosing the default search page) in the Search Settings page mean. I think in my UI mockup I suggested a column heading of "Default" but even then I am not sure that is clear enough? Anyway, as it is there are just those radios and no indication of what they are for. I also tried hovering over them, but that didn't help. Maybe a "set as default" link/button would be clearer and it could go under Operations? And then you could indicate "default" in the Status column? I'm not sure what would be best. It seems like "set as default" is an operation though, now that I think of it and try out the UI (as opposed to just making a mockup).

c) I do not think the default label for the content search should be "Node search". It should be "Content search".

d) The message I got when I edited and saved was "The search has been successfully saved. " Should be "Search page updated" perhaps? And I got the same message if I had just created a new one, so maybe that should be "Search page added"?

e) When editing the Content search, we've lost the help explaining node rankings that used to be there. Better yet, incorporate
#535616: Make Content Ranking settings for Search not say "weights" to avoid confusion
and make it actually sensible.

f) The help text for the label field when editing a search page config says "The administrative label for this search" should be "... search page" right?

g) The help text for the menu field when editing a search page config says "The title used by the local tab on the search page"... I thought we didn't want it to say "local tab" -- will people know what that is?

h) How are the tabs/pages ordered on the Search Settings form and on /search? Can we have that configurable?

i) The "Label" column is kind of confusing on the Search Settings page table, now that I look at it. That is showing the administrative label for the search page. I think this should be called "Name" in the table column header and also when you are editing the search page config, because "Label" made me think of the label a user would see when viewing the search. Actually... why do we need two different labels anyway? Maybe we can just have one Label field and have it be displayed both on the Search Settings page and on the Search tabs? Seriously I don't see why we need to have two, it just makes it confusing.

Anyway... These are all relatively minor/cosmetic things. I think it is pretty clear UI and works great! Of course, I kind of knew ahead of time how it should work so my testing of the UI may not be entirely valid.

I'll look at the patch itself next...

jhodgdon’s picture

OK, I also took a look at the patch, and have a few minor comments about documentation... you asked for no patches so I'm just presenting them here ==> overall looks good!! Here are the comments:

a) You've added something to core/lib/Drupal/Core/Entity/EntityListController.php -- can you update the issue summary about why, because it's not clear to me currently whether this is an artifact left over from another issue's patch or a real addition to the core entity API that this patch needs for some reason.

b)

+++ b/core/modules/node/config/search.page.node_search.yml
@@ -0,0 +1,9 @@
+id: node_search
+label: 'Node search'

see item (c) in comment #56.

c) Minor docs nitpick. In the NodeSearch plugin class, where it mentions "rankings" it says they come from "other modules". But mostly they come from the Node module itself. So can we change these docs:


  /**
   * An array of additional rankings from other modules.
   *
   * @var array
   */
  protected $rankings;
...
  /**
   * Gathers additional rankings from other modules.
   *
   * @return array
   *   An array of ranking information from other modules.
   */
  protected function getRankings() {
 

Probably "from other modules" should just say "from hook_ranking()"

d) In SearchController... I was pretty confused by the viewDefaultSearch() method. It says it redirects the generic search page to the default search page, but then it requires $entity as a parameter. So it looks like it really redirects to any passed-in plugin's search page? The documentation, name, and arguments are very confusing. Maybe the whole thing could be cleared up by making the @param description be "The default search page entity." which would make it clear that is what you specifically have to pass in, or else making the method name "viewSearchPage" and taking "default" out of the rest of the docs?

e) Same SearchController class - the editTitle method should say "The title for the search page edit form" in its @return (the word "page" is missing). I think we have to be pretty careful about the docs, because there are Search plugins (like NodeSearch) and SearchPage entities.

f) In \Drupal\search\Entity\SearchPage there are a few properties with no docs: $path, $title -- see also comment #56, item (i).

g) SearchPageForm has a couple of undocumented properties too: $entity. OK, just one. :)

h) core/modules/search/lib/Drupal/search/Plugin/Derivative/SearchLocalTask.php
I think the description of this class is now that it provides local tasks for search *pages* not plugins?

i) core/modules/search/lib/Drupal/search/Plugin/SearchPluginBag.php
I don't see why we need this specific class. Can't we just use the base class here?

tim.plunkett’s picture

FileSize
117.94 KB
12.29 KB

56a) Done! (only when it is enabled)
56b) That's just how tableselect works. I think we want to come up with a new mechanism for that while switching to tabledrag (so sorting can be done).
56c) That's what it is in HEAD. Think of the label as the admin label, the tab title is "Content"...
56d) Done
56e) @todo. I'll have a look at #535616: Make Content Ranking settings for Search not say "weights" to avoid confusion tomorrow
56f) Fixed
56g) I mean, that's the only place it's used... I don't know what else to call it.
56h) That should be part of the tabledrag follow-up
56i) Ehhhhhhh... Didn't touch this yet, will sleep on it.

57a) Removed. It was being used, but not super important.
57c) Fixed
57d) Fixed
57e) Fixed
57f) Fixed
57g) Fixed
57h) Fixed
57i) This, and several other plugin bags, exist for 2 reasons: type-hinting and future proofing. We want to know what type of plugin it will return, hence the overridden get(), and we might need to expand it in the future (like BlockPluginBag was).

ianthomas_uk’s picture

56b/h) I couldn't see a tabledrag followup mentioned, but a search found #1762848: Add ability to set order of search tabs. I think it makes sense to add a column heading of 'Default' for now though.
56g) Local tab doesn't mean anything to me. How about "Active tab"?
56i) The convention seems to be "title" or "administrative title". It can be useful to have separate admin and public titles, for example you might always want to just say "Search" on the public side, but something more descriptive for admin. Maybe something to discuss in a followup.

jhodgdon’s picture

Regarding "Node search" vs. "Content search", since 7.x we have had a UI text standard that we would not use the word "Node" in the UI:
https://drupal.org/node/604342#wording
"Use 'Content' or 'Piece of content' not 'Node'. Post should only be used as a verb (since Drupal 7)." So even if it's that way in HEAD currently, it is wrong and can we just call it "Content search" for both the admin and tab label?

Regarding the radio buttons, I think we either need to have a heading there saying "Default" on that column, or we should simply not use a tableselect, and instead have a separate drop-down select list below where you select the default page. That would actually possibly be better, because we can have a label and a help description to explain what it means. With no header it is just impossible to tell what the radios are for -- it's not like a Bulk Operations tableselect where there's a button telling you what you're selecting for.

Regarding #56i:
- I guess we probably do need two labels. Use case: hypothetical contrib search plugin that has permissions. Admin creates search A with tab name "Search Foo" for users of role A, and search B with tab name "Search Foo" for users of role B. And I was going to suggest just having a machine name, but our convention elsewhere in Core seems to be to allow people to have a nice human-readable admin name as well as a machine name.
- So I looked at what we're doing for content types, views, image styles, etc. Everything I looked at called the admin label "Name" on the table and "Foo name" in the edit screen, except for Display Modes, which call them label. So it seems like we should use "Search page name" in the edit screen, and "Name" in the table? Then in the edit screen, we can have help saying it's a label for admin use only.
- Then let's call the field currently called "Menu title" (which would only make sense to programmers I think?) "Tab label" or "Tab title", in which case that field will probably not need a description/help text.

jhodgdon’s picture

tim.plunkett’s picture

FileSize
120.66 KB
14.88 KB

59.2 I reworded the description, hopefully that helps.
59.3 Yes, I discussed this with @pwolanin and I think it is best to keep the admin label and title separate.

60 "Node" appears in the search UI in D7, and already in D8. This patch does not change the usage of "Node search".

After discussing more about the tableselect, I just went ahead and did the tabledrag part. I also added a select list to choose the default search. It won't directly override the weights of the tabledrag, but it will set the weight to -10 if it is the first one created, and it will affect what /search and the search block use.

I explored trying to change how /search redirected, but it messed with the tabs too much to be worth it. I'm just leaving the current behavior.

I moved one call to clearing local tasks into menu.inc, that is a purposeful change and needs to be done.

tim.plunkett’s picture

FileSize
45.76 KB

Screenshot of the new dropdown and tabledrag:

ianthomas_uk’s picture

Sorry I still haven't had a chance to look at this properly yet, great work though Tim and Jennifer.

What does 'default' actually mean? Is it still needed if we're introducing the ability to order them, or should the first automatically become the default?

If we do need a default, then what do you think jhodgdon's suggestion of adding a 'Set as default' operation, rather than a separate dropdown, and showing the default in the status column. There could also be an 'Enable and set as default' operation.

jhodgdon’s picture

RE #64 - that is what Tim just did in #63 -- see screen shot. And no we don't want the "first" to be the default necessarily. At least, I don't think we do. Separate setting is better.

Tim: I'll test this new patch out tomorrow.

tim.plunkett’s picture

We still need a default to choose which one is used by the search block and what loads when you go to /search.

Removing that default in favor of a purely weight-based system is fine for a follow-up. It might make more sense in the long run, but its out of scope here.

tim.plunkett’s picture

FileSize
129.88 KB
27.44 KB

msonnabaum pointed out that none of the code I had in the storage controller was storage specific, and suggested it be moved to a search page repository service.
So I did just that. And because it is now decoupled from the entity storage, it is unit testable!
100% coverage is a nice feeling.

The last submitted patch, 67: search-2042807-67.patch, failed testing.

jhodgdon’s picture

67: search-2042807-67.patch queued for re-testing.

jhodgdon’s picture

Status: Needs review » Needs work

I tested out the latest patch. It seems mostly fine. A few thoughts:

a) That search settings page has a lot of settings on it. So let's say you start at the top and are setting things up. Then you get down to the bottom and decide to disable User search. You go over to the drop-down and choose Disable. Oops, you've just lost all the settings on the page!

I don't think this is a problem for the Views page for instance, which uses similar drop-down mini-menus, because there aren't any settings on that page that you would have lost. But I don't think we should be mixing up settings on a page with instant go-to links in a drop-down like that on the same page... are we doing that anywhere else? It just seems kind of bad.

It would not be so bad if the selection of search plugins was at the top -- at least you would be less likely to lose a lot of work. Anyway... not sure what should be done about this (if anything) and I'm probably willing to let it go (maybe we should get the Usability folks involved?) but it didn't seem right to me.

b) I noticed that if I disabled User search then enabled it again, the order of the tabs I saw on the Settings page was Content then User, but when I went to /search itself, the order was User then Content. After dragging and saving, the order matched the settings page.

I then uninstalled Search and started over. Same thing: without changing the settings from their (reinstated) defaults, the Settings page shows Content above User, but when I go to /search, the User tab is to the left of Content.

I suspect the default weights are both zero and somehow the sorting is different on the settings page vs. the tabs?

c) Extremely minor thing: There is a : after "Search page type:" but not after "Default search page" (form field labels).

d) The system let me add a new search page with the same URL as a previously existing search page. (It didn't let me add duplicate machine names.) If you add a check for duplicate URLs, that should also apply to editing (you shouldn't be able to edit the User page for instance and change its URL to "node"). Hm... Well maybe you should be able to have two at the same URL? They might in principle for some unknown type of search plugin have different permissions. Maybe it should at least warn you though? They don't behave well in the tabs if both are visible -- if you try to click on one of the tabs, it just takes you to the other. Probably you should just not be allowed to have two with the same URL.

The last submitted patch, 67: search-2042807-67.patch, failed testing.

tim.plunkett’s picture

a) While this is true, I think any reorganizing should be left to #1559244: Clean up search settings page, and we should absolutely loop in the UX team for that.
b) Looking into this.
c) That's FAPI :\
d) That's a bug, looking into this.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
133.79 KB
7.34 KB

70b) was due to a bad default config. Fixed and added a test.
70d) was because my call to setFormError was missing the $form_state. Fixed and added test coverage.

While fixing the default config, I realized we didn't have a config schema. Added that with some help from vijaycs85

ianthomas_uk’s picture

65) In #63 Tim moved the default selection from a radio button on the row to a select box at the top. The earlier suggestion was that setting as default should be done from the Operations column instead.

70a) That's a pattern I've been bitten by on other forms in Drupal 7. It's not good, but I'd say it's out of scope for this issue. Similarly when I was testing #67 I got annoyed by having to repeatedly scroll down to the Search pages section each time I selected an operation.

70b) Again, this is a Drupal problem rather than specific to this issue. Multiple rows can have the same weight. The tabledrag widget picks one order, other places in Drupal may pick a different order (in my testing of search pages it was the opposite order). Defaulting new pages to a weight of -10 doesn't really help, as they are still all the same weight, just now it's -10 instead of 0.

tim.plunkett’s picture

No that specific problem was having the default node search not have -10 in its default. The rest should be and are 0 by default.

jhodgdon’s picture

So you're saying that now with the weights all zero by default, that the sorting in the tabs and the sorting you see on the Search Settings page is guaranteed to be the same?

tim.plunkett’s picture

Hmmm, that may not be 100% our fault. I opened #2159471: Ordering of local tasks is arbitrary for derivative-based tasks of the same weight.
I did add more test coverage for our sorting.

After further discussion with @ianthomas_uk and @pwolanin, I decided to remove the extra "title" after all. We just have label now, and the search path + search type in the table should be enough to differentiate.

Also massive reroll after #2145041: Allow dynamic routes to be defined via a callback.

ianthomas_uk’s picture

UI: /admin/config/search/settings
a) Default search page option could be integrated into main table, as mentioned earlier.
b) 'Search page type' label doesn't make any sense without the context of the 'Add new page'
button. The convention elsewhere would suggest a blue button labelled '+ Add search page'
at the top of the form. We could either have a series of buttons for each search page type,
or open a dialog for the user to choose the search page type (containing a list of links,
rather than a dropdown + submit, to reduce clicks).

UI: /admin/config/search/settings/manage/node_search
c) Page title would be better as 'Edit label search page' (there are several places in the code that refer to search rather than search page.)
d) IMO the field set around Content Ranking redundant now it's been moved to a shorter page
e) A 'Delete search page' option would be nice (compare: /admin/structure/types/manage/article)
f) I assume not being able to change the machine name is deliberate

search.page.node_search.yml
g) I think we decided not to include default UUIDs
h) The path would be better as content instead of node.

node.schema.yml
i) Do we want to use search.plugin.node_search.node_rank_comments, or would
search.plugin.ranking(s).comments be better? (ranking because it matches hook_ranking)
At the very least the second node seems redundant.
j) Settings for comments don't belong in the node module, but I'm not sure if a better
solution is possible (see #1831632: Convert node_rank_ $rank variables to use configuration system). Maybe just have search.plugin.ranking defined
as an associative array modulename => influence.

NodeSearch.php
k) When I first read the getRankings() comment I thought it would return an array of
rank_name > influence. Possibly change to "Gathers ranking definitions from hook_ranking()."
l) addNodeRankings() also needs a new docblock. Maybe "Applies the rankings defined by
hook_ranking() implementations with the influences configured for this search page."
m) Why do we need to add the node_rank prefix, can't we just return
$this->moduleHandler->invokeAll('ranking')? See my first comment for node.schema.yml.
Forms can rewrite keys if they need to.

Most of the above is fairly minor, the only thing I'd consider beta-blocking is deciding on the naming of the config (i and possibly j).

jhodgdon’s picture

#78a -- we do NOT want to merge the default into the table. we discussed this earlier. It was too confusing of a UI because tableselect does not have the ability to have a label on the select column.

#78h) We do not want the path to be search/content. It has been
search/node for many many many many versions of Drupal.

The rest of #78 seems to be good ideas.

Regarding the UI... my thoughts:

I like having just one label (as you might expect from comments above). Do we need to say in the form field description anything about it being used as the tab title?

I'm still seeing ordering weirdness. After adding a new page... how is that table ordered? It showed up in the middle on Search Settings, but then at the right end on Search. Guess that's the other issue.

ianthomas_uk’s picture

FileSize
24.89 KB

@jhodgdon I don't think you've understood what I meant when I said "merge the default into the table", hopefully this mockup should explain it better. I thought this is what you were proposing in 56b and haven't seen any arguments against it from a UX point of view. Maybe something was discussed in irc?

Mockup showing proposed default options

RE search/node vs search/content, I picked up on this following the comments in #60 explaining the UI should "Use 'Content' or 'Piece of content' not 'Node'." It seems really weird that we don't want to use the word node in the admin UI, but we're happy to use it in public-facing URLs. I guess the same applies to paths like node/123 though, and I don't see that pattern being changed any time soon.

jhodgdon’s picture

That screen shot in #80 looks great to me.

Regarding the URL, yes I agree in principle if D8 was a completely new piece of software. However since the URL has been search/node since at least Drupal 5, I think we need to leave it that way. People can change it now if they want to in the config.

Oh and regarding the fieldset around the search rankings (#78d) there is some text that is supposed to be there that has currently been lost, so I think we want to keep the fieldset.

And regarding #78f - You do not want the machine name to change if the search has an index, since the machine name is stored in the table. It's also used in the config file name [which is search.page.(plugin id == machine name).yml]. So it would be a pain to have it change. It makes sense not to allow it to change after it is set up.

jhodgdon’s picture

We just had a discussion in IRC about indexing here.

So. I was wrong in #81. The indexing for NodeSearch is all stored with 'node_search' as the 'type' column in the search index. So there is one shared node index for all NodeSearch page instances. Which is fine, because the config we have currently for NodeSearch is only the ranking information, which is only used at search time and does not affect the index in any way.

However, other search plugins may need to have access to their page-specific config in order to index. So what we're going to do (or rather Tim is going to do) is to make it so that in search_cron() when it does:

  foreach ($search_page_repository->getIndexableSearchPages() as $entity) {
    $entity->getPlugin()->updateIndex();
  }

that it will get set up with access to find its search page ID (the config entity ID), and it will also have access to the configuration array (the plugin-specific config info). That way plugins that do have page-specific indexing requirements can read config info at indexing time, and they can use the page ID to store in the search index as the 'type' (probably prefixed by their plugin type ID) instead of just using their plugin type ID as the 'type' as NodeSearch is doing.

Hope that made sense.

tim.plunkett’s picture

FileSize
38.76 KB
145.03 KB

78
a) Done
b) This was how it was in earlier patches, I think the current way is best. It is much clearer without the default option right above it
c) Done
d) I left this in, more settings that aren't related to just ranking (think per content type) might be added in later.
e) Done
f) Yes, it is the entity ID, and those are never changed
g) I checked in IRC, we should continue to provide them until we actually decide to remove them everywhere
h) I left this as is, per 79
i) I have no control over that naming, it is a schema describing the structure.
j) The settings aren't provided by node module, but they are gathered by node module and stored in NodeSearch, and the schema doesn't allow more granularity. See the block schemas for the same problem.
k) Done
l) Done
m) This is to prevent any conflict with non-ranking settings that could be added in later.

I addressed 82 in here. The new functionality is not utilized by any core plugins, but it has test coverage.

I also cleaned up direct access to the default_page setting to be always through the repository.

Status: Needs review » Needs work

The last submitted patch, 83: search-2042807-83.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
145 KB
4.03 KB

Made a mistake in my save logic.

tim.plunkett’s picture

jhodgdon’s picture

The Delete link on the page Edit page doesn't seem to work. It just takes me back to the edit page. This is after a fresh install with the -85 patch applied.

Hm. There must be something else wrong. I got:

InvalidArgumentException: The entity type (search_page) form controller "Drupal\search\Form\SearchPageForm" does not exist. in Drupal\Core\Entity\EntityManager->getControllerClass() (line 196 of /home/jhodgdon/gitrepos/drupal_mypatches/core/lib/Drupal/Core/Entity/EntityManager.php).

when I tried to go to search/node or search/user after Standard install.

This is possibly due to not being able to use git apply to apply the patch (sometimes the patch command might not create new files I think?). I'll try again with the patch in #85.

jhodgdon’s picture

Status: Needs review » Needs work

OK, I git applied the patch in #86 but the Delete link at the bottom of the Edit form still does not work. I just get back to the Edit form. The Delete operation from the Search Settings page works fine.

Everything else I think is great! I definitely like the new UI with the "set as default" option in the drop-down, and the status showing Enabled, Disabled, or Default.

jhodgdon’s picture

FileSize
7.93 KB

I also took a careful look through the code/comments in the #86 patch, since it has changed a lot since the last time I looked at it.

Wow, that is some nice code. It was overall very readable and I would hardly change a thing. I did have a couple of minor suggestions on API docs (surprise surprise). Interdiff attached since that is easiest for tim.plunkett to deal with.

Also note there were things addressed by some coding standards issues that the community has not decided on:
#1994890: Allow {@inheritdoc} and additional documentation
#2158497: [policy, no patch] Use "$this" and "static" in documentation for @return types

And I had a few questions about modifications to tests that I didn't understand and/or think may be in error... I ran out of time but I think the tests need more attention and review before we commit this.

And I didn't fix this:

+ * @group Drupal
+ * @group Search
+ */
+class SearchPageRepositoryTest extends UnitTestCase {

Should that be @ingroup? I don't know what @group is here.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
148.18 KB
1.77 KB

@group is a PHPUnit thing.

Added the interdiff, thanks. Here's the delete fix.

tim.plunkett’s picture

90: search-2042807-90.patch queued for re-testing.

ianthomas_uk’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
5.92 KB

Reviews Review

I've gone over the recent reviews and tried to summarise them so nothing is missed.

56: all implemented / no longer applicable, except
e) (node weight/influence table) I don't think this patch causes any regressions here. Lets address the other concerns in #535616: Make Content Ranking settings for Search not say "weights" to avoid confusion
h) (tab order) There are still issues when weights are equal, but these can be addressed in #1762848: Add ability to set order of search tabs and #2159471: Ordering of local tasks is arbitrary for derivative-based tasks of the same weight

57: All addressed in #58
59-60: All addressed / no longer applicable
61-63: n/a
64-66: Agreed we will keep the concept of a default search page, at least for now.
67-69: n/a

70:
a) (length of page) This will be improved in #2123073: Move index.cron_limit setting to NodeSearch and can be also be reviewed in #1559244: Clean up search settings page.
b-d) Addressed / no longer applicable

71-77: n/a / mentioned elsewhere

78:
a) Implemented
b) (add new page UI) This is better, but I still don't like it for the reasons mentioned. That's personal opinion though, and not a biggie.
c) [NEEDS WORK](edit page title) The text has been changed, but the name of the search page being edited should have emphasis. [fixed in attached interdiff]
d) Personal opinion again.
e) Implemented, but see new issues b and d.
f) Addressed - this is deliberate
g) Addressed - we are keeping default UUIDs for now
h) Addressed - keeping default path at search/node for consistency with older Drupal versions
i/j/m) [NEEDS WORK?] (hook_ranking config naming) What we've got works, but I think could be improved. I'll come back to this in a later comment.
k/l) Implemented

79-81: Implemented / n/a / mentioned elsewhere

82: [NEEDS WORK?] I'm still not convinced we've got a clear api for shared indexes vs index-time settings. I'll come back to this in a later comment.
83/86: n/a
87: Dodgy install? I don't think there is anything to address here.
88: Fixed in #90.
89: [NEEDS WORK] Tests need further review
90-91: n/a

I think I've marked above everything that we've already spotted that [NEEDS WORK] before RTBC. Everything else can be discussed in the referenced issues.

New issues

a) No message is set when enabling/disabling a search page. [fixed in attached interdiff]
b) It would be nice if "The search page has been updated"(/added,/deleted) included the name of the search page. [fixed in attached interdiff]
c) There are still some places that say "search" instead of "search page". [fixed in attached interdiff]
d) When clicking the delete link on the edit search for the default search you get an access denied warning.
e) When adding a search page it doesn't say what type of page you're adding, but I'm not sure where this could be added.
f) I delete all searches except one node search, then disabled the node module to see what would happen if there were no searches enabled. I got an exception on the search settings page: "The plugin (node_search) did not specify an instance class."

ianthomas_uk’s picture

My first attachment, interdiff-93-config.txt, shows how I would structure the node_ranking configuration.

First there's a couple of issues in node.schema.yml that I fixed in passing:
a) It was labelled 'Node search' which I've changed to 'Content search' as previously discussed
b) Each of the different ranking types is now defined as an integer. These store the numbers between 1 and 10 that are defined on the admin screens. Previously they were defined as the data types for the rankings that were being searched on, but the 'score' defined in hook_ranking should actually return a decimal between 0 and 1 which then gets multiplied by this number. In #90 the rankings for sticky and promote were being silently cast to booleans, so if you set it to 5 in the UI it would return true (shown in the UI as 1).

Second is my refactoring:
c) search.page.node_search.configuration.node_rank_comments becomes search.page.node_search.configuration.ranking.comments etc.
d) Ranking is now in an array called 'rankings' that maps 'the internal name of the ranking mechanism' to the associated weight/influence configured for that search page.
e) A prefix is still added for the edit form to avoid potential conflicts, but this prefix no longer leaks into the configuration itself.

f) This doesn't quite solve the problem of schema being defined in the wrong place (e.g. comments being defined in the node module rather than the comment module) and rankings potentially appearing/disappearing unpredictably, but it is a step towards a solution for those problems. If it's agreed that this is a good approach then I'll continue to work on those problems.

g) Regarding the discussions about sharing/not sharing search indexes. This is all handled internally in each plugin's updateIndex and execute methods. I think all we can do is add a warning to the documentation (including any documentation we have explaining how to implement/upgrade a search plugin) reminding people that they might have multiple search pages using the same plugin, and if they don't want them to share an index then they are responsible for differentiating the indexes they use. If people forget to do this it will lead to some weird bugs that are difficult to identify.

interdiff-93-indexes.txt adds a comment about this to SearchIndexingInterface.

tim.plunkett’s picture

Neither of the last two interdiffs worked as is (meaning, passed tests), but I was able to clean them up.
Also fixed 92d.
92f is another example of why we need #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall.

This is an interdiff on top of the last two, which I've applied.

jhodgdon’s picture

I just looked through all the interdiffs since the last time I looked, and it all looks good! I haven't tested manually though... tim.plunkett suggested I wait until ianthomas_uk is satisfied with the latest changes to look again, since he's been reviewing lately, so I might just do that.

Regarding the rankings configuration question... I personally don't see a huge problem here? The situation, as it currently stands:
- Modules can implement hook_ranking() to say "Hey, I have a search ranking that can be used".
- NodeSearch config allows users to define multipliers for each defined ranking.
- At search time, NodeSearch looks through the defined rankings from hook_ranking and intersects that with the ones that have been assigned non-zero multipliers in its config, and uses those to affect the order of search results.

This seems like the right approach. If a module providing a ranking is enabled or uninstalled, I don't think we really have to worry about anything. An unavailable ranking definition won't be used in search, and a newly available ranking will default to zero multiplier until the admininstrator edits the search page settings to assign it some other value. What's the problem?

It seems to me that trying to store the config for the ranking multiplier somewhere other than the config for the individual page would just make things way more complicated than we need, and the fact that there are comment and statistics configs in the default node search page config, well, who cares really?

But... The proposed schema for the rankings config in core/modules/node/config/schema/node.schema.yml seems wrong to me, because we should not hard-code any particular types of rankings (any contrib module can provide more).

+  mapping:
+    rankings:
+      type: mapping
+      label: 'Content Ranking'
+      mapping:
+        comments:
+          type: integer
+          label: 'Number of comments'
+        relevance:
+          type: integer
+          label: 'Keyword relevance'
+        sticky:
+          type: integer
+          label: 'Content is sticky at top of lists'
+        promote:
+          type: integer
+          label: 'Content is promoted to the front page'

These are basically the entries from hook_ranking -- they should not be hard-wired into the config schema. So... I'm not an expert on config schema, but it seems to me that rather than a mapping with fixed keys for the rankings section, we need the config to be more flexible... Really we just want an array of rankings, where each one needs to have a 'type' string value (which would be the machine name from hook_ranking), and an integer 'multiplier' value? Can you do that with config schema?

pwolanin’s picture

In general I think this looks better in as much as it's aligning with other plugins.

It seems a little weird that you can configure multiple user search pages that will be the same expect for title and path?

tim.plunkett’s picture

FileSize
150.85 KB

Attempted reroll, I did not address #95 yet.

ianthomas_uk’s picture

An unavailable ranking definition won't be used in search, and a newly available ranking will default to zero multiplier until the admininstrator edits the search page settings to assign it some other value. What's the problem?

With the current code, I assume a newly available ranking will cause a notice because we'll try to read a piece of configuration that doesn't exist. There's also the nuisance factor of configuration potentially appearing / disappearing when save the config entity, even though you haven't edited that piece of configuration.

interdiff-93-config.txt was always intended as a stepping stone - change the config structure in that, and then change the logic behind it in a followup. I can work on that today, but my idea is basically to properly support rankings not being listed in the array, and deliberately not list them if they are set to 0.

we should not hard-code any particular types of rankings (any contrib module can provide more)

Agreed

Other than the rankings config I'm happy with this patch. Tim's applied my three interdiffs which fix most of the points raised. He's fixed 92d, 92e is really minor so we can just ignore it and 92f is being handled in another issue. That just leaves the concerns jhodgdon had about test coverage.

Status: Needs review » Needs work

The last submitted patch, 97: search-2042807-97.patch, failed testing.

pwolanin’s picture

One other thing I'd love to keep in mind (even if it's implemented as a follow-up) is that I'd want to be able to use config add to or replace the $advanced array in NodeSearch, at least via API (not in the core ui).

jhodgdon’s picture

RE #98 - I am not sure about whether you'd get a notice if a new type of ranking was introduced by enabling a module. We should test that and see.

Anyway... So it sounds like our main concerns now are:

a) Test coverage -- this is a comment I made in #89: I had a few questions about modifications to tests that I didn't understand and/or think may be in error... I ran out of time but I think the tests need more attention and review before we commit this.

b) Schema/structure for config of rankings -- currently has some hard-wired ranking types and this is not really OK. The schema/structure needs to be more flexible. And we also need to verify that if you enable a new module with an implementation of hook_ranking(), or disable an existing one (whether or not its multiplier was set to a non-zero value), you do not get errors/warnings on the Search Page edit form or at search time. [A test for this would be good??]

c) Whether it even makes sense to allow some searches to have multiple pages (#96). IMO, this is OK -- yeah there's not much/any difference currently between each NodeSearch or UserSearch page, except the tab title and URL, and probably no one would want multiple UserSearch pages, but having the ability to define multiple UserSearch pages is not really a problem either, so let's just leave that in and not worry about it.

d) #100. Let's make that a follow-up if it is not already a different issue (which I think it may be?).

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
151.63 KB

There are schema problems across all of core right now. There is no easy way for plugin/hook derived config to properly declare its schema. Just see block.schema.yml for the user, path, and node based visibilities being hardcoded in there.
Solving that is out of scope.

I still have no idea what the test coverage problems are. I think I've been pretty clear about the places I've expanded or moved coverage. The only deletions should be for things we no longer support.

Rerolled for #535616: Make Content Ranking settings for Search not say "weights" to avoid confusion (another issue that could have just waited).

The failure seems random, and the bot doesn't report which test...

Status: Needs review » Needs work

The last submitted patch, 102: search-2042807-102.patch, failed testing.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
151.8 KB
1.07 KB

I think this is finished, and I'm not to prepared to continue rerolling this anymore. I can help if truly needed, but please let's just get this in.

The last submitted patch, 104: search-2042807-104.patch, failed testing.

tim.plunkett’s picture

FileSize
3.59 KB
151.94 KB

Wonderful, now we have another critical to deal with that has cropped up in the meantime: #2167323: \Drupal::state()->set('menu_rebuild_needed') is not guaranteed to be checked on the next request

EDIT: By the way, I'm not injecting that. It's a @todo, it can use \Drupal for now.

ianthomas_uk’s picture

FileSize
150.59 KB
3.65 KB
23.99 KB

Here are my ranking config changes. This solves the schema problems mentioned in #93f onwards:
- hook_ranking is free to add/remove rankings whenever it wants to, and it will have no impact on the exported schema until someone assigns a non-zero influence to that ranking.
- side note: If hook_ranking stops returning a ranking that has been given a non-zero influence, then that will stay in the config. This is by design: hook_ranking can return different results from one request to the next, and it's better to have redundant config than to silently lose useful config.
- the config schema now defines a generic sequence, so there is no need for each hook_ranking implementation to define schema for its rankings.

Other minor changes:
- search.page.node_search.yml has been changed to match the format that would be automatically generated.
- my full patch sees SearchPageForm.php as a new file rather than a rename of SearchForm.php. This is probably just because Tim was working from a sandbox and I'm not.

As far as I'm concerned this is ready apart from jhodgdon's concerns about tests, and I know tim.plunkett said he was also happy with it. Do anyone have any other reasons this shouldn't be committed? Who is appropriate to RTBC?

tim.plunkett’s picture

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.php
@@ -578,9 +576,12 @@ public function buildConfigurationForm(array $form, array &$form_state) {
+      if (!empty($form_state['values']['rankings_'.$var])) {

Isn't that going to be wrong for 0? Also this could just be "rankings_$var", the concatenation is wrong.

ianthomas_uk’s picture

FileSize
150.58 KB
1.41 KB

This fixes the coding standards thing from #108.

To clarify the intention of #107, if a ranking has an influence of 0 then it has no impact on the search results, and following my changes will not be included in the config. This stops the config from changing when a new ranking option is made available but not used.

@pwolanin would you be able to RTBC this please?

tim.plunkett’s picture

For the record, I'm fine with this, let's get it in.

jhodgdon’s picture

RE #102 - webchick and I and I think another person as well on that other issue checked that patch carefully and we were pretty sure patch there and the patch here did not conflict, which is why it got committed. We were aware of the possibility, but the contexts did not appear to be a problem. Sorry about that.

Regarding the tests, I do not think there were any specific problems, I just previously felt that I hadn't given them a thorough review (due to lack of time), and I do not like to mark a patch RTBC unless I have actually reviewed it all. Today I made the time to give them a thorough review, and I agree that the tests are fine.

I also read through the recent interdiffs, and I agree that these are fine.

I also tested manually. I found a couple of problems testing manually:

a) I went to admin/config/search and set the default search to the User search page, and then I deleted the Content search page. Went back to /search and both tabs were still there. So it looks like there is a cache clear that isn't happening? After going to admin/config/development/performance and clearing the cache, the Content tab goes away.

b) I added a Content search page back. Then I dragged the Content to the top and saved. The tab order didn't change, so I went back to the search settings page and clicked "show row weights". Both were zero. So it looks like dragging is not setting the row weights right.

I am not sure if these are problems with the current patch or other core bugs...

But other than those two issues, I do think this is ready to go in. If those are existing core issues, can we link them here and make sure they're major/critical/release blockers? I do not see anything currently in "related issues" that appears to be related to these two problems.

tim.plunkett’s picture

Both a and b are a casualty of #2164367: Rebuild router as few times as possible per request, (formerly #2167323: \Drupal::state()->set('menu_rebuild_needed') is not guaranteed to be checked on the next request).
If that doesn't get in before this is RTBC, I can add more workarounds.

jhodgdon’s picture

I don't think we necessarily need to add work-arounds in this code -- that other issue is a critical -- but let's make sure it's marked as a "related" issue.

It is not the cause of (b) in #111 though. What appears to be happening is that when I drag the search pages around on the search settings page, the weights are not changing (when I click "show row weights" the weights are still zero after dragging). That might be a bug in the tabledrag code or in how it is being used? Not sure.

ianthomas_uk’s picture

I've filed #111b as #2168233: tabledrag javascript does not update weight fields when dragging rows, which should not need to block this issue. You can still reorder the plugins by choosing the weights from the dropdown.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
Related issues: +#2168233: tabledrag javascript does not update weight fields when dragging rows

Excellent... well, not excellent, but thanks for tracking that down and filing the issue!

So we now I think have a working system (aside from two critical core issues that are beyond the scope of this issue), and a patch that three of us are happy with. Let's get it in.

jhodgdon’s picture

The tabledrag issue has been rolled into the original issue that broke it.

catch’s picture

Only did a visual review, and not very in-depth, but found very little to complain about.

One issue that's worth changing

+++ b/core/modules/search/lib/Drupal/search/SearchPageListController.php
@@ -0,0 +1,346 @@
+    $count = format_plural($remaining, 'There is 1 item left to index.', 'There are @count items left to index.');

format_plural() is deprecated, however it's also still used 64 times in core, so that shouldn't block commit.

I found some other very small nitpicks that I couldn't bring myself to post about.

I don't fully get how the search plugins really look yet so may have missed something a lot more high level, but to be honest if there are limitations there, it's nothing compared to the issues this one blocks.

Leaving RTBC a little longer.

jhodgdon’s picture

@catch - we have about 8 or 10 other issues postponed on this one. I'm not sure whether you want a change to the patch or are willing to commit it as is, but we'd like to move forward with actually trying to maintain the long-neglected search module, so if you need it changed, let's get on it soon. Thanks!

webchick’s picture

Assigned: Unassigned » catch

Sending back to catch for feedback.

catch’s picture

Title: Convert search plugins to use a ConfigEntity and a PluginBag » Change notice: Convert search plugins to use a ConfigEntity and a PluginBag
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

No I'm fine with this as is. Committed/pushed to 8.x, thanks!

Will need a change notice.

jhodgdon’s picture

Thanks for committing this!

There was an existing change notice for the initial conversion of Search to use plugins:
https://drupal.org/node/2083471
I decided we should edit that rather than creating a new one. But since this was also a change to the existing change notice, I also created:
https://drupal.org/node/2169613
in case people follow change notices via RSS (they would not get notified on an update). I don't know if that is standard practice or not? We can delete that second one if it is not needed.

Anyone want to review the change notice and then (hopefully) set this to "fixed", change the title/category/priority, and remove the "needs change notification" tag?
https://drupal.org/node/2083471/revisions/view/2878497/6823079

andypost’s picture

I'd like to point that hook_ranking() is not described but troublesome in #2101183: Move {comment_entity_statistics} to proper service

jhodgdon’s picture

hook_ranking has not changed since probably Drupal 5, so it is not in the change notice.

jhodgdon’s picture

I have added a note to the change record that the node-specific hooks still exist: hook_ranking, etc. These have not changed since D7 (and probably much longer), but the item in the change record about search hooks going away could have been misunderstood.

andypost’s picture

Title: Change notice: Convert search plugins to use a ConfigEntity and a PluginBag » Convert search plugins to use a ConfigEntity and a PluginBag
Assigned: catch » Unassigned
Priority: Major » Critical
Status: Active » Fixed
Issue tags: -Needs change record

Awesome change notice! restoring issue

jibran’s picture

Please can we add something about core/modules/node/config/search.page.node_search.yml in the change notice. See the last part of https://drupal.org/node/2020549 for reference. IMO ConfigEntity doesn't make sense with out yml file explanation.

jhodgdon’s picture

I am not sure an explanation is completely necessary, because I don't think a search plugin module necessarily needs to provide a YAML file. But I added this to the change notice:

Modules providing search plugins can provide a YAML configuration file that will configure a search page. This file would be named search.page.(machine_name).yml -- you should be able to create a page from the Search admin page and find the config in your site's configuration directory. See files search.page.node_search.yml and search.page.user_search.yml in the node and user modules for examples.

chx’s picture

I filed a doxygen followup at https://drupal.org/node/2171819 please review.

Status: Fixed » Closed (fixed)

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