Updated: Comment #183
Problem/Motivation
Similar to #1950726: Convert hook_archiver_info into the New Shiny(TM) we need to convert hook_search_info to the new shiny plugin system. Using that issue as a template on how this should work.
Proposed resolution
- Convert hook_search_info() to use plugins instead
- Define base classes and interfaces
- Remove outdated and non-working parts of the core Search functionality
- Make the core Search functionality less node-module-specific, by moving node-specific functionality into the node module.
Remaining tasks
The patch exists and has been extensively reviewed.
User interface changes
Minor: in the search admin page, the plugin titles and machine names are listed as setting options as opposed to module names.
API changes
- hook_search_info is replaced by annotation-based plugin discovery using class
\Drupal\search\Annotation\SearchPlugin
- A new interface
\Drupal\search\Plugin\SearchInterface
and a base class that implements all the method as stubs\Drupal\search\Plugin\SearchPluginBase
are defined. A separate interface for plugins that use the Search module's indexing is also defined:\Drupal\search\Plugin\SearchIndexingInterface
(user search does not use core indexing, so the indexing-specific methods were separated out). - All the search module hooks except hook_search_preprocess() are converted to be methods on the plugins that are invoked as appropriate
- The search plugins are accessed via
\Drupal\search\SearchPluginManager
which is available as a service 'plugin.manager.search', e.g.Drupal::service('plugin.manager.search')
- Since these implementations are plugins, a single module can now provide multiple search plugins - in contrast to Drupal <= 7 where there was a limit of one search implementation per module
- Node and User searches are converted to plugins
- Table {search_node_links} and the related functionality are removed since it was causing extra indexing of content and the code addling to this table was broken. The consensus amongst the search module maintainers was that it was a rare edge case. See comment #133 for more information.
- Node advanced search filters are new implemented via query sting ?f[]= parameters instead of being embedded into the search keyword string
- The SearchExpression class, which was added to Drupal 8, is not needed any more, so it is dropped. It had some functionality for injecting/extracting advanced search parameters into search keyword expressions, but this is done with query arguments now, so it is not needed. Correspondingly, since each search plugin basically sets up its own query, the setOption() method on SearchQuery is not needed, so it is also removed. Also, the function search_expression_insert()/extract() are removed.
- Functionality specific to the Node module is moved from Search module to Node module or replaced by more generic functionality. For instance, search_touch_node() is replaced by search_mark_for_reindex() and the node module has a specific function node_reindex_node_search() that calls this.
- In Drupal 6, hook_update_index() was called on all implementing modules. In Drupal 7, this was silently dropped to only being called on active search modules (causing some bugs). In Drupal 8, this is now a method on the SearchIndexingInterface, and thus will explicitly only be called on search plugins that use indexing.
Related Issues
Related:
#1933518: Convert search's system_config_form() to SystemConfigFormBase
#1831632: Convert node_rank_ $rank variables to use configuration system
Comment | File | Size | Author |
---|---|---|---|
#194 | 192-194-interdiff.txt | 6.86 KB | alexpott |
#194 | 2003482-194.patch | 184.08 KB | alexpott |
#192 | 2003482-192.patch | 178.21 KB | effulgentsia |
#192 | interdiff.txt | 756 bytes | effulgentsia |
#191 | 2003482-190.patch | 177.35 KB | jhodgdon |
Comments
Comment #1
Nick_vhShould be a working prototype thanks to pwolanin, tim.plunkett and dawehner!
Comment #2
Nick_vhComment #3
Nick_vhThird time should be good. Forgot to enable the cacheDecorator. There is still a gitignore in there, but as it is a work in progress I'm not removing that yet.
Comment #4
Nick_vhLast spam from me, fixed whitespaces and removed the gitignore thing. There is still a problem when you search for a keyword it shows the title as "error".
Comment #5
pwolanin CreditAttribution: pwolanin commentedfixes for advanced search, etc.
Comment #6
tim.plunkett@Nick_vh mentioned that you have "Error" as your page title, it means you improperly typehinted a method.
admin/reports/dblog should have your true error message.
I'll give it a full review sometime this weekend.
Comment #7
pwolanin CreditAttribution: pwolanin commentedok, I'm seeing such a typehint error I don't understand why it's happening:
The class is:
Comment #8
pwolanin CreditAttribution: pwolanin commentedoh, I see I forgot the "use" declaration.
some other minor cleanup here as well.
Comment #10
Nick_vh#8: 2003482-8.patch queued for re-testing.
Comment #11
BerdirI'm working on #1903346: Establish a new DefaultPluginManager to encapsulate best practices which will change the way plugin managers are defined, might make sense to get that in first.
Comment #13
Nick_vhAdding some documentation to make it more complete. Thanks for pointing us to the discovery changes.
Comment #14
Nick_vhFixes the drupal install problem. Was missing a leading slash.
@Berdir, would you be able to explain us how the change in the discovery would simplify this patch? I'm not 100% following what you are doing there. Also @swentel pointed me to the Routing of field_ui and we could most probably use and copy most of that code to replace the hook_menu magic that we are currently doing.
We should expose a service and give the searchPageManager as injection so that the routing can be created based on all the plugins. This should be a follow-up patch after this patch gets in.
Comment #16
pwolanin CreditAttribution: pwolanin commentedreworked the test code to switch to a plugin and ironed out some bugs.
Comment #17
kim.pepperThis issue touches some of the routing issues found in #1987806: Convert search_view() to a new style controller so added it to the summary.
Comment #18
dawehnerJust wondering whether you have considered to rename it to @SearchPage
The title should be translatable, see EntityType as examples
You could use either ContextFactory or ContainerFactory to inject dependencies into plugins. (Here the database connection).
Just a bunch of other dependencies
You could just "use" this class.
If you have an interface you can just use {@inheritdoc} in your implementations and save all this c&p of docs.
Comment #20
pwolanin CreditAttribution: pwolanin commented@dawehner - thanks
I assumed every plugin annotation class should have "Plugin" in its name? Not sure if there is a standard here?
re: injecting the dependencies, do you have an example of a plugin doing this? So I could (?) replace the node_load() with an injected entity_manager? Not sure about how to replace the other procedural calls.
I opened another issue about the code style of extends/implements using the full namespaced class or interface: #2004178: [Policy, no patch] Coding standard should not require "use" when you are extending a class or implementing an interface
Comment #21
pwolanin CreditAttribution: pwolanin commented@dawehner - also, at what point is the property translated? The one above is being used in hook_menu (and the router in the future) and will get translated there, so I'm not sure if there is a risk of double translation or insertion into the DB of the pre-translated title?
Comment #22
pwolanin CreditAttribution: pwolanin commentedtrying to resolve the fatal errors (they were still calling the procedural functions).
Comment #23
jibranPlease add interdiffs it helps a lot.
Comment #25
dawehnerCore uses a custom annotation class (Translation) in order to make it easy for l10n.drupal.org and the tools to scan for translatable tools. This is done all over the place already, so let's keep consistent.
Comment #26
pwolanin CreditAttribution: pwolanin commentedHere's a significant revision based on discussion with EclipseGC about how to inject dependencies into plugins.
Comment #27
pwolanin CreditAttribution: pwolanin commentedhere's an interdiff too
Comment #28
pwolanin CreditAttribution: pwolanin commentedComment #29
pwolanin CreditAttribution: pwolanin commentedre-roll against current HEAD - hoping the bot actually tests this one. no actual change from #26
Comment #31
pwolanin CreditAttribution: pwolanin commentedfix the fatal error in tests and makes the service naming consistent
Comment #32
pwolanin CreditAttribution: pwolanin commentedHere's a patch to move away from using context per discussion in IRC.
Comment #33
tim.plunkettThis should be
title = @Translation("Content"),
, and remember touse
the Translation class.Otherwise this is looking great.
Comment #34
pwolanin CreditAttribution: pwolanin commentedLooking at the search module code, it seems the optional hook_update_index() was also coupled to hook_search_info(). hook_search_status() was also somewhat coupled, so it might make sense to make that code part of this plugin as well.
the api.php also needs ot be revised in this patch.
Comment #35
pwolanin CreditAttribution: pwolanin commented@tim.plunkett - I was unsure about using @Translation. Does that mean the annotation is translated before it's used? If so, that's potentially wrong since the text will be translated again as part of the menu system.
Comment #36
pwolanin CreditAttribution: pwolanin commentedThis patch moves most of the search hooks into plugin methods and makes the plugin class full-featured rather than just for executing a search.
Comment #37
pwolanin CreditAttribution: pwolanin commentedfixes some notices
Comment #38
tim.plunkettNOTE: I reviewed #36. There is no interdiff :( so I don't know what changed.
These all need docblocks with @var
Also, lowerCamelCase for these.
Usually we've been splitting this out like
With no trailing comma on the last one
Needs a docblock
This should be done in parent::__construct()
One thing we've done is to get the EntityManager injected, but store the different controllers in properties. Something like
No reason to have this variable, $this->module_handler (should be $this->moduleHandler) is fast enough
needs a typehint
If 'search.settings' is the only object used, I would also consider storing that directly in __construct instead of the factory
This can be \Drupal\node\NodeInterface
The params need @param
Trailing whitespace
@param array
Contains \Drupal\...
Nope :)
You can actually do
$this->container->get('...
in testsMissing a space after foreach. Also, search_get_info still?
If you're using CacheDecorator it already has a static cache, this is overkill.
These should be added by the plugin manager or something. We shouldn't wrap the plugin manager getDefinitions()
Probably needs to update the @param
Mostly the same comments as for NodeSearch
Comment #40
pwolanin CreditAttribution: pwolanin commented@tim.plunkett - thanks - the difference was only a couple lines to set a default.
search_get_info (or some renamed version) is still useful since it gives us the list filtered by the ones that are set active. I don't want to have to write that every time.
As we discussed in IRC, changing the keying to be the plugin ID instead of the module would be a good goal here, so then a single module could expose multiple plugins. I guess that will require a small update function to change the list of active things.
Comment #41
pwolanin CreditAttribution: pwolanin commentedComment #43
pwolanin CreditAttribution: pwolanin commentedThis fixes most of the tet failures. interdiff is to 37 again, since that's basically what Tim reviewed last.
Comment #44
pwolanin CreditAttribution: pwolanin commentedok, posting the incremenetal diff from 37 to 43 per Tim.
Comment #46
tim.plunkettRelated:
#1933518: Convert search's system_config_form() to SystemConfigFormBase
#1831632: Convert node_rank_ $rank variables to use configuration system
Comment #46.0
tim.plunkettAdded a link to controller conversion
Comment #47
pwolanin CreditAttribution: pwolanin commentedpossible need to call this blocked on #1831632: Convert node_rank_ $rank variables to use configuration system
Comment #48
effulgentsia CreditAttribution: effulgentsia commentedTagging
Comment #49
kim.pepperNeeds re-roll since #1933518: Convert search's system_config_form() to SystemConfigFormBase removed search.admin.inc
Comment #50
pwolanin CreditAttribution: pwolanin commentedok, so let's re-roll and not wait for config conversion
Comment #51
pwolanin CreditAttribution: pwolanin commentedHere's an initial attempt at rebase and re-roll. The incremental diff may lie a bit due to the rebasing.
Comment #53
dawehnerJust a note: let's document these variables.
As the storage controller is needed in some places it could move to the constructor.
lets use spaces after the 'foreach'
"ssearch"
This could be simplified a lot by using the DefaultPluginManager
Is it right to put in potentially upcasted values in here? I guess that is fine, because it is a non-dynamic page anyway (without placeholders), but the accountinterface object is in there.
Comment #54
jhodgdonpwolanin asked me to take a look at this... It's a big patch, but I just wanted to say that in general, I think the approach is good, and the general outline of the plugin, base, and interface classes.
Obviously there are a few hopefully minor issues to sort out, but I think this is a good idea in general.
Comment #55
jhodgdonpwolanin and I were looking into the two test failures.
It turns out that (we are pretty sure anyway) HEAD is broken and the fact that those two tests pass in HEAD (without the patch) is due to some faulty tests.
Here's the logic:
- When Core runs search_cron(), it invokes hook_update_index() on all modules that are enabled for search. Meaning, generically, node and user modules.
- So normally in Core, the two implementations comment_update_index() and statistics_update_index() are never run.
- These two functions are what set up the normalizations for the comment/statistics search rankings (the ones that are failing in the tests on the latest patch).
- However, this basic bug in Core Search is masked by the fact that all of the existing tests were doing an invokeAll on hook_update_index(), which invoked the comment/statistics hook implementations *during tests* even though they would never be invoked in a real world situation.
So... Basically the tests for search ranking were passing in Core because of this invocation of the comment/statistics hook implementations, which would never happen in real life. And the fact that the same tests fail on the last patch is because that test is not written wrong, and it caught an actual Core bug.
Sigh.
If we want comment and statistics search rankings to work, a different mechanism for normalizing the search rankings during cron runs needs to be found. hook_update_index() is not the right answer. hook_node_update_index() might be.
Comment #56
pwolanin CreditAttribution: pwolanin commentedThis patch cleans up the code as suggested and removes the 2 failing rankings from the test since the underlying functionality is actually broken in core as jhodgdon says. Probably are broken in 7.x too since release.
This patch also fixes the search config form for node module so it actually sets the ranking variables.
@dawehner - the potential of having the account object was one of the reasons for doing
e.g. if you want to do some kind of personalized search.
re: the entity controller, we are using at some points one or both the storage controller and the render controller, so it seemed the current constructor was easier for handling both.
Comment #57
pwolanin CreditAttribution: pwolanin commenteddoesn't need reroll
Comment #58
Nick_vhThis is a little cryptic so getting this in core this way is quite weird. However, I understand it's a complicated situation as explained by jhogdon and it'll get fixed in a separate issue. As soon as the test is green this can be marked as RTBC and I will.
Comment #59
pwolanin CreditAttribution: pwolanin commentedok, so I guess jhogdon and I also have poor memory: #893302: Search ranking based on the factor "number of comments" & "No of Page Views" is broken
apparently this bug got left open for 7.x in 2010 despite the patch from jhogdon.
Comment #60
Nick_vhComment #61
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #62
jhodgdonRE #59 - I've bumped that issue up to 8.x.
Comment #63
tim.plunkettThis patch no longer applied, so I rerolled it.
I also noticed an @todo for cleaning up search_get_info(), and I thought it was important to do that in one fell swoop, and not in another follow-up.
Comment #65
tim.plunkettThat was a really stupid mistake.
Comment #67
tim.plunkett/facepalms
Comment #68
Nick_vhLooking good. Marking as RTBC
Comment #69
pwolanin CreditAttribution: pwolanin commentedI'm wondering why the change to array_filter() + anonymous function? I don't think that's a pattern we should be using.
Comment #70
tim.plunkettWe use it in many places. Why not?
Comment #71
pwolanin CreditAttribution: pwolanin commented@tim.plunkett - I think it's both much less readable and very likely (while minor) less performant.
This patch has the manager provide an array of active plugins which makes the module code cleaner, and makes some other minor changes with an eye towards a possible follow-up to maintain a list of active plugin IDs, instead of allowing only one plugin per module. We need a follow-up in any case in handle the routing conversion.
Comment #72
tim.plunkettI like getActivePlugins(). I don't care about foreach vs closure. This is fine.
Comment #73
jibranWhy this extra line?
we have entityManger available here.
node_type
is entity now so we can also use entityManger here.Can we add @todo here with issue link?
Can we swap location of
__construct
andcreate
?Doc missing.
Doc missing.
we can replace
db_or
with$query->orConditionGroup()
thanks to @chx see #1921426: [Followup] Move node access storage to DIC.we can replace
db_like
with$this->database->escapeLike($string);
just confirmed this with @dawehner on IRCComment #74
jibranThanks @tim.plunkett for the correction no need to swap
__construct
andcreate
.Comment #75
tim.plunkettAs part of #2044203: [meta] Convert info hooks to plugins
Comment #76
kim.pepperApplies fixes as per #73. Think I have it all right.
Made a couple of docs cleanups along the way.
Comment #77
tim.plunkettRe-RTBC, thanks @kim.pepper!
Comment #79
tim.plunkettAh, THAT'S why we had it in a temporary variable. Let's revert that bit.
Comment #80
kim.pepperReverted...
Comment #81
kim.pepperToo hasty! I reverted the wrong line :-(
Trying again.
Comment #82
tim.plunkettThanks.
Comment #83
catchComment #84
alexpottNeeds a reroll
Comment #85
BerdirThat's very likely due to the nid -> id() change, when re-rolling, just make sure that you replace all ->nid with id() in there and you should be good.
Comment #86
kim.pepperThat was a painful re-roll, and my git rerere skills aren't quite there yet.
Pretty sure I got all the nid to id() conversions.
Comment #87
jibran@kim.pepper I know the pain. I have wasted half an hour on re-roll as well and then hit
git rebase --abort
user_access
to$account->hasPermission()
I think we can replace it easily.
Comment #89
kim.pepperFixed the fatals, but not getting any active search plugins registered when building the form now.
Comment #90
kim.pepperRefactored to handle #2043379: Allow plugins to be discovered in any directory
Getting a redirect loop on search/node now.
Comment #91
kim.pepperRe-installed, and don't get the redirect loop issues anymore. Spoke with @timplunkett on IRC and he said we have coverage for that already.
Comment #92
kim.pepperFixes for user_access as per #87
Comment #93
jibranWe can inject request using create method. It should be
Drupal::request()->attributes->get('account')
anyways. See https://drupal.org/node/2049309Comment #94
kim.pepperFixed the issue with missing ->attributes in #93 and injecting the request now.
Comment #95
jibranBack to RTBC after #82.
Comment #96
jibranPatch doesn't apply anymore needs reroll after #2045919: Replace all module_implements() deprecated function calls..
Comment #97
jibranHere is a reroll and back to RTBC.
Comment #99
tim.plunkett#2043781: Drupal::request()->attributes->get('account') may conflict with an account object loaded from the path happened.
Comment #100
pwolanin CreditAttribution: pwolanin commentedfix account to _account due to #2043781: Drupal::request()->attributes->get('account') may conflict with an account object loaded from the path
Tim has pointed out that keying by module name is really an unacceptable bug, so i'll also try to re-roll to key by plugin ID.
Comment #102
jibranIt passes locally after reverting the change of #97.
Comment #103
kim.pepperWould RTBC, but had too much input into patches.
Comment #104
jibranI have only rerolled the patch and reverted the only change I did in #97 so I can RTBC it so back to RTBC.
Comment #105
pwolanin CreditAttribution: pwolanin commentedPer discussion with Tim.plunkett, we shouldn't get this in unless it's keying plugins by plugin ID instead of module. Currently a module can only provide one plugin, which is rather counter to the whole concept.
This patch shifts the settings, keying, etc to be by ID to resolve that bug.
It also adds use of AccessibleInterface and (previously missing) PluginInspectionInterface
Comment #107
pwolanin CreditAttribution: pwolanin commentedbroken due to #2017207: [meta] Complete conversion of users to Entity Field API
Comment #108
pwolanin CreditAttribution: pwolanin commentedSimple fix.
Comment #110
pwolanin CreditAttribution: pwolanin commented#108: search-2003482-108.patch queued for re-testing.
Comment #111
tim.plunkettNice. Berdir would be proud.
Comment #112
jhodgdonI read through all of the documentation in this patch, and some of the code, and it mostly looks great, except:
a) I found a typo in the manager class:
(last word there)
b) in SearchInterface:
These @params should have types and must have descriptions. The @return description is a bit obtuse as well, and the first line should start with Sets instead of Set, and should probably say "parameters" instead of "params". (getSearchParams should have the same change?)
c) It would be nice to have a description somewhere in all of this of what a "param" is vs. an "attribute" of a search. A good place would be in the doc header for SearchInterface maybe? I have no idea when reading this class's docs what these two terms mean in terms of how they affect searches, how they'd be set up, etc.
d) SearchInterface again:
The type should be "bool" here not "boolean". And in the description, it should say something like "TRUE if the search settings are valid, and FALSE if it isn't". Note all caps on TRUE/FALSE.
e) SearchInterface has some more methods whose docs start with the wrong verb person: execute(), buildResults(), etc etc. Tests/SearchConfigSettingsFormTest.php does too. And Search/SearchExtraTypeSearch.php.
f) The word wrapping is not good in updateIndex() docs. Minor problem, maybe don't bother to fix.
g) search_reindex() in search.module has a typo where it says $plugin_is instead of $plugin_id in the docs header:
h) search_cron() docs still refer to hook_update_index():
i) in search_index():
Is this accurate? Wouldn't it be the plugin ID? If not, I think it needs more information on what $type would be, because if I had this question... probably others would too?
j) Somewhere near the end of search.module -- probably in a @defgroup:
ummm... looks unfinished.
k) Member variables for classes need one-line doc summaries:
and the rest of them in this class.
Comment #113
kim.pepperFixes docs in #112 except for i) which needs someone more intimate knowledge of the change to comment on.
Comment #114
jhodgdonLooks good!
Still a few things to fix:
a) SearchInterface:
it isn't ==> they aren't
b) There are still some methods whose first line verbs are wrong, such as in the SearchExtraTypeSearch test plugin's execute() method. Maybe that's the only one.
i) I still also want to know about (i) from #112
j) Can someone explain what search "params" and "attributes" are, in the documentation (and shouldn't they be called "parameters" rather than "params" anyway)?
Comment #115
pwolanin CreditAttribution: pwolanin commentedre: search_index() - while I had thought to change $module to $plugin_id at one point, it seems the intent of the code is to reference a type of content (the DB column name is 'type')
Fixing up the patch now including conflicts with HEAD.
Comment #116
pwolanin CreditAttribution: pwolanin commentedThis changes params to parameters and tries to improve the doxygen.
Comment #117
jhodgdonRE #115 - really? I think that "type" referred to a search type/module, didn't it? Which was "node"? Because when doing a search of a given "type" in the old system, I think the query had a condition on the type column... let's see....
Yes, here's the D7 code:
Comment #118
pwolanin CreditAttribution: pwolanin commented@jhodgdon - yes, it used module before, but it's unclear since there was a 1:1 mapping of module name to entity type to search "type"
Of course, this code predates a clear concept of entity types even!
My initial thought when making hte conversion to key by plugin ID was to use the plugin ID as the "type", and then I changed my mind to assume it was entity type. I can re-roll it the other way if you think that makes more sense in terms of the search module API.
Comment #119
jhodgdonOK, but you're still confusing the search type and entity types.
This is definitely meant to be search type and not entity type, because there could be several different modules providing search plugins/types for searching content of entity type node.
Let me see if I can explain what I mean. For example, let's say I have search plugin A that indexes nodes, and search plugin B that also indexes nodes. Both will use the node ID as "sid". Both call search_index() to index the node, but each one indexes different text (for instance, different fields etc.).
The search module needs to then store each plugin's presumably different interpretation of the text for that node separately, so it cannot use "node" as the "type" field, it needs to use plugin A or B's machine name.
Does that make sense? Each search type/module/plugin needs to have a scheme for having its own search IDs (sid), and then when it passes this to search_index(), the Search module needs to store that information so that when a search result has a match for that "sid" value, the plugin can match that to the particular content item of whatever it's indexing.
Comment #120
pwolanin CreditAttribution: pwolanin commentedThis fixes some silly fails due to param -> parameters change that I missed in the test plugin.
Comment #121
jhodgdonpwolanin and I just discussed this on IRC. We need to make some changes to this patch for sure.
So....
a) Let's document that a Search plugin defines a search "type". Here's proposed SearchPlugin:
Note that $module is removed.
I'll continue with more suggestions in the next comment (pwolanin and I are discussing this in IRC as we go).
Comment #122
jhodgdoncontinuation of (a) Actually, SearchPlugin needs to have an @see to SearchInterface too.
b) Let's add some explanation to SearchPluginBase/SearchInterface:
c) I think we should pare down SearchInterface to the bare minimum methods that the Search module actually requires plugins to have. Which I think are just:
- setSearch
- isSearchExecutable
- buildResults
right? The rest should just be on SearchPluginBase but not on SearchInterface because they aren't needed by all search plugins, so they shouldn't have to implement them.
pwolanin thinks we need a few more but I think there are too many that aren't really needed for every search plugin. Let's cut them down as much as possible.
d) And let's add to the docs for isSearchExecutable() and buildResults() to say when the Search module calls them.
Comment #123
jhodgdone) And then somewhere, we also need to document the correspondence between the index and the search query extender.
search_index() should say any module using the core search index needs to pass in a unique ID there for $type (and suggest using the plugin ID). And then the SearchQuery extender needs to document that in searchExpression(), you pass in the same $type for the second argument.
Comment #124
pwolanin CreditAttribution: pwolanin commented@johgdon - a counter-argument to type being the pluign ID is here:
Clearly here type is entity type - looking through the search indexing code, there is no easy weak to break that correspondence.
So, while I agree that the plugin ID would be preferable in general, I think for NodeSearch we have to use just 'node'.
Comment #125
pwolanin CreditAttribution: pwolanin commentedI think this is getting is much better shape - moving even more code out of node module.
This now puts advanced search filters from advanced search into the URL in the ?f[]= format used by Facet APi in contrib.
Comment #127
jhodgdonRE #124 - I don't think that's the right argument to make.
I think of the index/retrieval part of the Search module as a provider of indexing and retrieval services for other modules that provide searches.... Here's a description of how I think it should work:
The core Search module's Search Indexer (SI) and Search Query Extender (SQE) provide a service that an independent search provider module (SPM) (provider of a plugin that defines a search type) can use to index and search whatever it wants to define as "content":
- The SPM needs a way to identify a piece of content with an ID that it understands, independent of other SPMs.
- The SPM needs a way to add content to the SI, independent of other SPMs, with the ID attached to it.
- The SPM needs a way to tell the SI that content has been invalidated or needs to be reindexed, independent of other SPMs, by providing the ID to the SI.
- The SPM needs a way to use the SQE to search only its own content, independent of other SPMs, and have each result identified by its ID.
- No SPM should interfere with or be allowed to search any other SPM's data.
- Not all SPMs will use the SI/SQE system to do their searching (for instance, the User SPM does not use the SI/SQE system, but instead directly queries the user table).
- If we could manage to define the SI/SQE system using the Dependency Injection Container, then another module like Solr could define itself as a provider of SI/SQE services as well, and you could have the choice of turning on Solr and overriding the core SI/SQE provider. This is probably beyond the scope of what we can do for Drupal 8 though... but it would be cool if it could happen.
So, under this view, the Node module happens to use the node ID as its unique identifier of content, and in the example of #124, what is really happening here is that the node module as a SPM is saying, when a node is updated, tell the SI that the content owned by the node SPM with a particular ID needs to be reindexed. If another SPM happens also to include nodes as content, then that SPM needs to also detect that a node has been updated and invalidate whatever ID it happens to have for that particular node. Each SPM has to take responsibility for the content it gives to the SI, and the node SPM shouldn't be trying to do anything to other SPM's data.
At least... If the core Search module is going to be usable as a generic search index/retrieval framework, that is how I see it working. That is basically how it worked in Drupal 7 as well.
Comment #128
pwolanin CreditAttribution: pwolanin commentedFixes a bug in forming the query string, and a test looking for the filter in the wrong place.
Comment #129
pwolanin CreditAttribution: pwolanin commentedoops, posted the patch 2x, here's the interdiff.
Comment #130
pwolanin CreditAttribution: pwolanin commented@jhogdon - I agree with the threoy of what you said, but I'm not sure how to get there from the existing search.module code.
It's maintaining a table specifically for 'search_node_links', and it's looking to match the 'nid' field. It basically has hard-coded node-indexing logic.
So, I guess we could let the node search plugin use 'node' as the type as a hack, or just leave this type as something likely to be an entity type?
Comment #131
jhodgdonRE #130 - I don't see the problem? Whatever the node search plugin does during indexing (whatever "type" it passes to search_index(), that is) is the same thing that needs to be put in as the 'type' value in that query in #124. And definitely, the ID that node search plugin uses for the "sid" when indexing content should be the node ID.
Comment #132
jhodgdonthat was an unintentional tag change there ?!?.
Comment #133
jhodgdonpwolanin and I had yet another discussion about this in IRC.
So... What's happening (this references D7's behavior) is that search_index() currently has a heck of a lot of code that is specific to D5-era nodes and books in it. What it does is that if someone has a link like:
in node/98776 that is being indexed (or, startlingly, also links like book/12334!?!), then during the indexing of node 98766:
- "pizza" is added to table {search_node_links}
- node/12345 is marked for reindexing.
And then when node/12345 is reindexed, function search_node_update_index(), which node.module invokes when preparing content for indexing, adds "pizza" as a link (with no HREF attribute) to the end of the content of node/12345.
So basically if you then search for "pizza", you will get node 98766 because the word pizza is in it, and also node 12345 because 98766 linked to it using link title "pizza".
A lot of messy code that has outdated cruft in it (like the book/* links?!?), just to accomplish that the 2nd node comes up in searches for pizza, when most likely that second node had the word pizza in its title anyway.
Furthermore, search_index() will do this not only when node.module is indexing nodes, but also when other modules' search providers are indexing their own content, whatever that might be. *!?!?
So, I think pwolanin and I both agree that:
- no one probably knows about this functionality
- no one probably cares about it or is relying on it
- it is not a great idea that search.module is so tied up in node-specific stuff
- it shouldn't be mixing up various search modules's stuff, ever.
- if we really wanted to make this general, search_index() could make a list of links it finds in content it is asked to index, and call a method on the search plugin that requested the search to give it the information and let it deal with it, but this would be kind of complicated. Or node.module could scan through content as it calls search_index(), find links, and do this itself (no reason it needs to be inside search_index()).
===> But really, we probably don't need this functionality at all -- it's an unnecessary complication and an edge case, so let's just drop it. This will greatly simplify search_index() -- it took pwolanin and me about 20 minutes or more in IRC just to figure out what the code was doing anyway. And we could get rid of the search_node_links() table.
Comment #134
jhodgdonOh. We also think that search.module should have a function called something like
search_mark_for_reindex($type, $sid)
and probably one called
search_remove_from_index($type, $sid)
if it doesn't already.
Comment #135
jhodgdonOh, one more thing: I also think that if possible, all of the search-related stuff in both node and user modules should be moved into separate modules. Maybe it's not so necessary for user.module, because I think all the search stuff (after this patch) will be in the plugin class anyway. But for node.module, there will need to be some hooks (like when a node is updated, it needs to be marked for reindex I think maybe?)... and it would be good if all the search-related stuff was in its own module. Maybe?
Comment #136
pwolanin CreditAttribution: pwolanin commentedI'm not sure splitting out search-related code is worth a separate module given that it's only a small amount of code. This patch just moves certain hook implementations from search module to node module, as well as killing all the link-scoring code and removing that table an using the plugin ID as the search index type.
Comment #137
pwolanin CreditAttribution: pwolanin commentednoticed some unintended changes to the settings form context param - this restores them.
Comment #138
jhodgdonI started at the top of this patch and reviewed a lot of it. I think we're very close! I have a couple of suggestions, as usual:
a) The NodeSearch class -- This class does more than just execute searches. Maybe we should make its first-line docs say:
Handles searching for node entities using the Search module index.
b) In NodeSearch:
- Is 'user' the node author? Should we in that case call it "author"?
- In 'term'... isn't there a 'vid' needed here? Maybe not... whoah. I guess that the taxonomy_index table does not use 'vid'. Hm. OK. Scratch this comment.
- Should we document what these array elements are, and maybe what the keys/values represent? The docs for $advanced are not illuminating.
c) In the NodeSearch::execute() method, I don't understand why all the "f" parameters (and can't you document them better than calling them "f" things?) are first concatenated into one string and then parsed out. At a minimum, this needs some explanation in a code comment, because it seems to me (naively?) like we start with an array of key => value, and then have to parse out the keys... why not just not concatenate? But I'm probably missing something. I guess I am not sure what format $parameters['f'] would be in maybe? Anyway the code is a bit obtuse and I can't review it because I don't understand it.
d) I would think we need to take care of the @todos that reference variable_get() (node rankings) before committing this?
e) comment.api.php
Way to decouple search.module from being so node.module centric!! However, shouldn't this be using the node.module function node_reindex_node_search() instead, which verifies search.module is enabled (which is what the actual node_comment_update() implementations are doing)?
f) The first line docs for SearchIndexingInterface do not actually say what it is for. How about:
Defines an optional interface for SearchPlugin objects that use the core Search index.
g) Generally/minor: a lot of the docs are not word wrapped to 80 character lines.
h) SearchInterface
build -> builds in the first line
d) SearchFormAlter: first line should start with Alters
e) Do we still need SearchExpression class?
f) In search.install()... Is it actually OK to edit existing hook_update_N() functions? Shouldn't we just be adding new ones instead?
g) search.module _search_menu_access()
Needs . at end of docs.
h) search_mark_for_reindex docs:
extra * on line
i) I think comments can now be attached to other entities besides nodes. If this is correct, the hooks in node.module that respond to comment CRUD need to check whether the comment is actually attached to a node and only update the node search index if that is the case.
Comment #139
pwolanin CreditAttribution: pwolanin commentedre: d) we just moved the code in here - I'd rather leave it for a follow-up
re: e) we could probably kill it, but it seems advertised as a search.module feature
re: f): HEAD to HEAD updates are not supported at this point in the cycle.
re: g) maybe due to being copied from old code? Point out any obvious places to wrap
Comment #140
jhodgdonOK.
d) I just don't think the committers are going to accept a patch with variable_get/set/@todos in it, but maybe.
g) Don't worry about it.
Comment #141
pwolanin CreditAttribution: pwolanin commentedRe-roll for minor conflict in user.module and to respond to comments above.
re: i) the Comment entity only seems to be able to be associated with nodes.
Added comments to NodeSearch re the f query string, but here's more explanation:
$_GET['f'] is an array that looks like this in the URL:
And this in code:
so imploding that gives a string like we used to have in the advanced search keywords:
doing implode() plus preg_match_all() seemed simpler and more efficient that a foreach loop over all of $f for each possible "option" (field name), especially since we want to group them for the OR condition.
Comment #142
jhodgdonHm. Regarding comments on nodes, tim.plunkett says that the plan for 8.x is that they will not be node specific... it is not done yet though.
#731724: Convert comment settings into a field to make them work with CMI and non-node entities
Regarding $f...
It seems like the parsing/code would be clearer if it did something like this:
(might need some testing to see if $parts is really 2 elements etc.) That would turn $f into an array something like:
etc.
Comment #143
pwolanin CreditAttribution: pwolanin commented@jhodgdon - well, if it's not done we can't really account for it.
Here's a approach with a foreach and matching on each one. Also fixes a problem with the 'term' advanced entry.
Comment #144
jhodgdonAgreed, we cannot deal with comments on non-node entities yet. Just something to be aware of if that patch lands before this one; alternatively, that other issue will need to be updating search-related hooks if comments can be added to non-node entities.
Anyway... It looks like we have a working patch. I'm going to have to make some time to give it a full review.
Comment #145
jhodgdonI reviewed this patch carefully from top to bottom, and found a couple of very minor things, which I went ahead and fixed:
a) In comment.api.php, hook_comment_insert() and hook_comment_update() the sample function bodies should both be calling node_reindex_node_search(). Only one was changed. I fixed that one line.
b) The first-line docs for UserSearch plugin said the search was executed against the search index, which isn't true. I fixed that one line.
c) There were also a couple of other minor docs things, like verb tenses in function first lines, typos, etc. I fixed them also.
d) There was an indentation problem in NodeSearch::execute() that I fixed.
e) I added a inline comment to the $f parsing from your notes above.
And there were a couple of things I didn't understand so I didn't fix:
f) In the NodeSearch::indexNode(), there was a line like this at the end:
This is semantically wrong. It should just be calling function search_index(), which is not a hook, just an ordinary function. Why was this done? Either it needs a comment explaining why, or it should just be replaced by an ordinary function call to search_index(). This method should not be called if search.module is not enabled, and there are other things that will fail miserably in the updateIndex() and other methods if it isn't enabled, like queries against tables that will not exist...
g) In NodeSearch::execute(), there is an implicit assumption that everything that comes through in the facets in the URL is a known type of advanced search. I added a comment there to that effect, but it seems like that assumption should be checked in the code?
h) Does NodeSearch::searchFormSubmit() need to be sanitizing the values it is reading directly from $form_state?
i) I noticed that the search tables search_dataset and search_index have langcode fields. Shouldn't search_mark_for_reindex() also take an optional $langcode argument? search_reindex() does... I guess it si not such a big deal because at worst it will be inefficient if several items are marked for reindexing, but still.
j) Along the same vein, I'm wondering if NodeSearch::updateIndex() should be doing some kind of DISTINCT or something because if a node has multiple languages, its nid will show up multiple times in {search_dataset} marked for reindexing, so I think we will be reindexing the node multiple times? I'm not sure about this.
k) We have functions search_expression_insert()/extract() and a SearchExpression class with these methods, and then the NodeSearch plugin goes ahead and does its own parsing for the search facets... Wow, do we really need to have all of that? I think we should get rid of the search_expression* functions (they are just wrappers on the methods), and I think that the NodeSearch plugin should use the SearchExpression class to add/extract facets. Either that or we should get rid of the SearchExpression's insert/extract methods entirely.
l) I noticed that on other plugin annotations, items that are translatables have
instead of @var string. Should our Search annotation class be doing that too? I really don't know...
Comment #146
pwolanin CreditAttribution: pwolanin commentedI don't see much value in using the SearchExpression class - just as well to remove it in the name of simplicity
Only expected option strings are matched from the f parameter, so there isn't any leakage.
I was assuming we wanted all module function calls to go via the module handler - maybe Tim can give feedback on that?
Comment #147
tim.plunkettComment #148
jhodgdonRE #147 - well if you're going to do that, you should do it all over all kinds of methods in NodeSearch. Because many of them are assuming that certain database tables exist, so you'd get PDO exceptions if you tried to run those methods without search.module being enabled.
So we would either need to put that all over NodeSearch plugin, or I think it's just safe to call that function in the NodeIndex method.
Comment #149
tim.plunkettI misunderstood. I was responding to the question about that particular usage of invokeHook, and missed the forest for the trees.
Since search plugins are provided by the search module, you theoretically should be able to rely on the module being enabled.
But the best course of action would be to turn that function into a method on a service.
That way if anyone wanted to reuse the search plugins without search.module in contrib, they'd just have to provide their own service with a null implementation or something.
Moving search_index() to a service should be opened as follow-up and referenced by issue number in an @todo
Comment #150
jhodgdonYeah, a service for indexing would be the holy grail that would allow Solr etc. to just take over on searching. Not happening on this issue. :) A @todo comment in the code sounds like a good idea.
So I think the answer to #145 (f) is that all that invokehook nonsense should just be replaced by a simple function call to search_index(). Status to needs work accordingly.
The other comments I had on #145 (G-L)... I am not sure about all of them and am certainly willing to be convinced (which is why I did not set that patch to "needs work"). But if the answer is "convince Jennifer" then maybe some comments ought to be added to the code to clarify those questions?
Comment #151
pwolanin CreditAttribution: pwolanin commentedtim points out that the annotation type is provided by the search module, so basically these plugins would never be discovered or used in its absence.
Comment #152
pwolanin CreditAttribution: pwolanin commentedComment #153
pwolanin CreditAttribution: pwolanin commentedoops, mis-made the interdiff
Comment #154
tim.plunkettHate to say it, but the render controller should be injected here. See what entity_view is wrapping.
Comment #156
pwolanin CreditAttribution: pwolanin commentedOops, when I rebased I missed that the search block form moved to class SearchBlockForm
This also removes the now-unused (and broken) setOption() method from the query class
Comment #157
pwolanin CreditAttribution: pwolanin commented@tim.plunkett - sorry I didn't see you comment about node_view() when I re-rolled.
In fact we are already using the render controller elsewhere in that class, so this was just an oversight.
Also switches to just language_load() instead of calling invoke on it, since that function is defined in bootstrap.inc
Comment #158
pwolanin CreditAttribution: pwolanin commentedre: #145::h searchFormSubmit() is simply processing the values and putting them into the redirect URL. So, there's no reason to try to sanitize, since the user can just as well type anything they like into the URL.
Adding a DISTINCT per #145::j
re: #145::l, the title is not actually translated - it's indeed a string since it's passed through the menu system which later translates it. This may change later depending on local task plugin conversion.
Comment #159
jhodgdonI think that the only comment from #145 that still hasn't been addressed is:
g) In NodeSearch::execute(), there is an implicit assumption that everything that comes through in the facets in the URL is a known type of advanced search. I added a comment there to that effect, but it seems like that assumption should be checked in the code?
Any thoughts? Otherwise I think this is ready to go.
Comment #160
pwolanin CreditAttribution: pwolanin commented@johgdon - re: node facets, I don't think there is any assumption (I think I addressed this above)
The patterns is:
So only one of the expected option strings will be matched. However, I see you could mess with this a bit. Let's fix that by adding "^" to the pattern.
Otherwise, yes - I think this is pretty well vetted now.
Comment #161
jhodgdonAh yes, I see. OK, I think this is ready to go in, after only 160 comments, reviews, and reworkings of patches.
Comment #162
tim.plunkettOne last thing.
This needs to be
@Translation("Content")
(same with other plugins)This should be
@var \Drupal\Core\Annotation\Translation
We can't use t() like this.
Any reason not to reuse this?
Comment #163
jhodgdonRe #162 - I was wondering about #1/2 (commented on #145 but pwolanin said it was OK). I missed #3/#4, good catch!
Comment #164
pwolanin CreditAttribution: pwolanin commentedok, so using @Translation() and putting in a todo that this may be broken until the route conversion follow-up.
Comment #165
jhodgdonLooks good to me. +1 for RTBC again but I'll let @tim.plunkett decide.
Comment #166
tim.plunkettAlmost!
No colon after @todo, the second line needs to be indented two spaces, and you have trailing whitespace.
This should technically be the interface, not the class itself
No docblock (don't ask why)
public function setUp()
Comment #167
pwolanin CreditAttribution: pwolanin commentedComment #168
tim.plunkettGreat, thanks!
Comment #169
tim.plunkettGreat, thanks!
Comment #170
pwolanin CreditAttribution: pwolanin commentedrebased and re-rolled to account for HEAD changes to Drupal::config() from config()
Comment #171
effulgentsia CreditAttribution: effulgentsia commented#1987806: Convert search_view() to a new style controller is postponed on this.
Comment #172
pwolanin CreditAttribution: pwolanin commented#170: 2003482-170.patch queued for re-testing.
Comment #174
pwolanin CreditAttribution: pwolanin commentedre-roll for conflict in the use statements in user.module
Comment #175
pwolanin CreditAttribution: pwolanin commented#174: 2003482-174.patch queued for re-testing.
Comment #176
jhodgdonI think it is time to stop having to re-roll this issue after 175 comments.
Comment #177
pwolanin CreditAttribution: pwolanin commented#174: 2003482-174.patch queued for re-testing.
Comment #178
alexpottUnfortunately a critical got committed that broke this.
Comment #179
BerdirThe only conflict was this
Actually wondering if we didn't lose test coverage there. Restored the previous functionality using the new API there, see interdiff.
Also removed a few now unecessary getBCEntity() and calls that relied on the BC decorator, let's see if there's more.
Comment #180
pwolanin CreditAttribution: pwolanin commentedLooks good - glad to know we can remove the BC wrapper now.
Comment #181
pwolanin CreditAttribution: pwolanin commentedremoving tag
Comment #182
pwolanin CreditAttribution: pwolanin commented#179: 2003482-174.patch queued for re-testing.
Comment #183
effulgentsia CreditAttribution: effulgentsia commentedDries, xjm, and I quickly glanced over this patch, and at a high level, it makes a lot of sense to modernize the Search API to OO. However, it looks like this patch makes more changes than just moving code from a pseudohooks to interface methods. For example, dropping the {search_node_links} table, and changing search_touch_node() to node_reindex_node_search(). Can the issue summary be updated with the complete set of API changes being made here? We'll need that for a change notice anyway.
Comment #183.0
effulgentsia CreditAttribution: effulgentsia commentedadded related issues
Comment #183.1
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #183.2
jhodgdonAdd more details and use the issue summary template
Comment #183.3
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #184
pwolanin CreditAttribution: pwolanin commentedjohogdon and I updated the summary
verified that patch applies cleanly to HEAD commit fb7f2c7c85a8112af8bbd14df3e3198cf4c54b51
Comment #185
pwolanin CreditAttribution: pwolanin commented#179: 2003482-174.patch queued for re-testing.
Comment #186
pwolanin CreditAttribution: pwolanin commentedstill applies cleanly to HEAD:
commit 970fdb9e479f15291a9b54ea429c68e1b621607a
Author: Nathaniel Catchpole
Date: Wed Sep 4 12:09:19 2013 +0100
Comment #187
Dries CreditAttribution: Dries commentedIt looks like upgrade path tests are missing. Because
_search_update_8000()
is changed to something non-trivial it could really use an upgrade path test. Other than the missing upgrade path tests, this patch looks RTBC. I understand it's no fun to keep re-rolling this patch -- I will add it to the top of my TODO list once the upgrade path tests have been added. I think we're really close -- thank you for your efforts to date.Comment #188
xjmReference: http://drupal.org/node/1429136
Comment #189
pwolanin CreditAttribution: pwolanin commentedOk, well I guess that was a useful exercise, since we missed the need to call $config->save() in the update function helper and some wonkiness around config merging.
Includes a little tweak also to avoid an array_flip() warning, though I'm 99.9% sure that's only a HEAD-to-HEAD problem. Also tweaks the config yml file so the default looks the same as what's written out when you submit the form.
Comment #190
pwolanin CreditAttribution: pwolanin commentedoops - browser funkiness with the tags. adding back the relevant one.
Comment #191
jhodgdonI am not an expert on upgrade tests, but to me, all of the changes appear to be correct and the test code is very clear.
There's a typo that appears twice in the upgrade test comments (databse -> database):
I (gasp) edited the patch file directly to change those two words to the correct spelling (no other changes, verified with diff but I'm not making an interdiff file) and re-uploaded. Assuming it turns green, I think this is good to go.
Comment #192
effulgentsia CreditAttribution: effulgentsia commentedUpdated search.schema.yml to match the config changes made by this issue. Trivial, so leaving at RTBC.
Comment #193
jhodgdonWhoops, that got overlooked - thanks effulgentsia!
Comment #194
alexpottThis patch needed a reroll but git did it automatically... :)
Tried to commit this but I noticed some spelling mistakes (would have fixed during commit) but I also noticed that the removal of the
search_node_links
means that the views.view.backlinks.yml will be impossible to do in core since this linked to the said table in Drupal 7. Patch attached removes it. Talked it over with @xjm and she agreed that if this issue removes this table then it needs to do something with the backlinks view.Setting patch to needs review to confirm approach.
Comment #195
jhodgdonI don't get it.
If the search_node_links table was needed, how come there wasn't a test that failed when we removed it?
The code that was finding those links was deeply flawed and not accurate in Drupal 7 anyway... and the links table, even if it was working, was not really doing anything for searches, as discussed previously, except making the whole process less efficient.
That said, if someone wants to make a table of link A goes to link B (which is what that table was purporting to do), you could take the code we took out of the search indexing code, fix it to make it actually work correctly, and make a hook_cron to generate the table of links.
What spelling mistakes did you notice? We've all been through the code multiple times and didn't notice them... Happy to fix them but editing your own writing or writing you've looked at multiple times, you don't notice spelling problems.
Comment #196
dawehnerThis view does not have tests at the moment is because the search integration into views wasn't part of the initial commit of views, and it haven't made it in yet. The approach is fine,
The approach done by alex is fine!
Comment #197
tim.plunkettWhat @dawehner said, +1.
Comment #198
jibranoh tags please comeback to the issue node.
Comment #199
alexpottConsidering the scope of my patch was to remove a view and fix some spelling mistakes I feel I can still commit this - it has been rerolled enough :)
Committed db2d92e and pushed to 8.x. Thanks!
Comment #200
jhodgdonOMG!
So... I woke up at 5 AM this morning worried there was perhaps one more thing we needed to do in this patch -- but it was already committed.... We may need a quick follow-up on the update function.
Here's what I was worried about.
I think in the search_index and search_dataset tables in D7 and previous, we were indexing nodes with the type column set to "node" [type was the module name], and in D8 we are using the plugin name, which is now "node_search".
So I think we need to in the update_8000 function also do a database query that translates the type column in those tables from the module name to the plugin ID?
It seems like _search_update_8000_modules_mapto_plugins() needs a few lines inside the
that do this? And then the update test would need to be updated to test that a search query on the D7 node database would also work for the D8 node database.
Tell me I'm wrong... :) If not, I'm sorry for not realizing it sooner!
Comment #201
jhodgdonApparently my brain is not working quite right at 5 AM. :) We truncate all search tables in the update_8001 function (that was written before this patch). So we're OK.
Comment #202
catch#2083415: [META] Write up all outstanding change notices before release
Comment #203
effulgentsia CreditAttribution: effulgentsia commentedremoving some unneeded tags
Comment #204
pwolanin CreditAttribution: pwolanin commentedinitial notice posted: https://drupal.org/node/2083471
Comment #205
BerdirRestoring title and priority.
Comment #206
jhodgdonI have reviewed the change notice and made some minor copy edits. I think it is good to go.
Comment #207
jhodgdonI found this issue in the Search component... Is this where the backlinks stuff is being worked on or is it a duplicate of another issue? Hasn't been updated since July...
#1853536: Reintroduce Views integration for search.module (not supporting backlinks view)
Comment #208
mradcliffeI opened up a follow-up #2087169: NodeSearch::updateIndex does not include order by column in select to address a lingering postgresql issue carried over from Drupal 7.
Edit: s/linger/lingering
Comment #209
tstoecklerQuick follow-up: #2089835: SearchSettingsForm "use"s ModuleHandler but actually doesn't
Comment #210
BerdirPerformance follow-up: #2090947: SearchPluginManager plugin definitions aren't cached.
Comment #211.0
(not verified) CreditAttribution: commentedUpdated issue summary.