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:
#1933518: Convert search's system_config_form() to SystemConfigFormBase
#1831632: Convert node_rank_ $rank variables to use configuration system

CommentFileSizeAuthor
#194 192-194-interdiff.txt6.86 KBalexpott
#194 2003482-194.patch184.08 KBalexpott
#192 2003482-192.patch178.21 KBeffulgentsia
#192 interdiff.txt756 byteseffulgentsia
#191 2003482-190.patch177.35 KBjhodgdon
#189 2003482-189.patch177.35 KBpwolanin
#189 2003482-174-189.increment.txt6.74 KBpwolanin
#179 2003482-174.patch173.64 KBBerdir
#179 2003482-174-interdiff.txt3.02 KBBerdir
#174 2003482-174.patch174.02 KBpwolanin
#170 2003482-170.patch174.22 KBpwolanin
#167 2003482-167.patch174.04 KBpwolanin
#167 2003482-164-167.increment.txt2.17 KBpwolanin
#164 2003482-164.patch173.75 KBpwolanin
#164 2003482-160-164.increment.txt8.29 KBpwolanin
#160 2003482-160.patch173.12 KBpwolanin
#160 2003482-58-160.increment.txt838 bytespwolanin
#158 2003482-158.patch173.11 KBpwolanin
#158 2003482-157-158.increment.txt1.12 KBpwolanin
#157 2003482-157.patch173.11 KBpwolanin
#157 2003482-156-157.increment.txt1.22 KBpwolanin
#156 2003482-156.patch173.07 KBpwolanin
#156 2003482-151-156.increment.txt1.93 KBpwolanin
#153 2003482-145-151.increment.txt9.38 KBpwolanin
#151 2003482-151.patch171.72 KBpwolanin
#151 2003482-145-151.increment.txt0 bytespwolanin
#145 2003482-145.patch164.04 KBjhodgdon
#145 2003482-143-145-interdiff.txt17.78 KBjhodgdon
#143 2003482-143.patch161.38 KBpwolanin
#143 2003482-141-143.increment.txt4.25 KBpwolanin
#141 2003482-141.patch161.67 KBpwolanin
#141 2003482-137-141.increment.txt5.96 KBpwolanin
#137 2003482-137.patch160.64 KBpwolanin
#137 2003482-136-137.increment.txt2.38 KBpwolanin
#136 2003482-136.patch160.9 KBpwolanin
#136 2003482-127-136.increment.txt22.63 KBpwolanin
#129 2003482-125-127.increment.txt1.72 KBpwolanin
#128 2003482-127.patch146.96 KBpwolanin
#128 2003482-127.patch146.96 KBpwolanin
#125 2003482-125.patch145.94 KBpwolanin
#125 2003482-119-125.increment.txt42.94 KBpwolanin
#120 search-2003482-119.patch121.06 KBpwolanin
#120 2003482-116-119.increment.txt1.96 KBpwolanin
#116 search-2003482-116.patch121.04 KBpwolanin
#116 2003482-113-116.increment.txt4.05 KBpwolanin
#113 2003482-search-plugin-113.patch125.57 KBkim.pepper
#113 interdiff.txt8.53 KBkim.pepper
#108 search-2003482-108.patch125.03 KBpwolanin
#108 2003482-105-108.increment.txt743 bytespwolanin
#105 search-2003482-105.patch125.08 KBpwolanin
#105 search-2003482-102-105.increment.txt49.83 KBpwolanin
#102 2003482-102.patch92.91 KBjibran
#102 interdiff.txt619 bytesjibran
#100 2003482-99.patch92.96 KBpwolanin
#100 2003482-97-99.increment.txt1.47 KBpwolanin
#97 2003482-97.patch92.96 KBjibran
#97 diff.txt683 bytesjibran
#94 2003482-search-plugin-94.patch92.89 KBkim.pepper
#94 interdiff.txt3.17 KBkim.pepper
#92 2003482-search-plugin-92.patch92.54 KBkim.pepper
#92 interdiff.txt1.21 KBkim.pepper
#90 2003482-search-plugin-90.patch92.45 KBkim.pepper
#90 interdiff.txt861 byteskim.pepper
#89 2003482-search-plugin-89.patch92.44 KBkim.pepper
#89 interdiff.txt5.99 KBkim.pepper
#86 2003482-search-plugin-86.patch88.09 KBkim.pepper
#81 2003482-search-plugin-81.patch87.98 KBkim.pepper
#81 interdiff.txt1.23 KBkim.pepper
#80 2003482-search-plugin-80.patch87.98 KBkim.pepper
#80 interdiff.txt794 byteskim.pepper
#76 2003482-search-plugin-76.patch87.94 KBkim.pepper
#76 interdiff.txt7.16 KBkim.pepper
#71 search-2003482-71.patch86.13 KBpwolanin
#71 search-2003482-67-71.increment.txt6.45 KBpwolanin
#67 search-2003482-67.patch86.01 KBtim.plunkett
#67 interdiff.txt1.63 KBtim.plunkett
#65 search-2003482-65.patch86.24 KBtim.plunkett
#65 interdiff.txt847 bytestim.plunkett
#63 search-2003482-63.patch86.24 KBtim.plunkett
#63 interdiff.txt24.64 KBtim.plunkett
#56 2003482-56.patch80.31 KBpwolanin
#56 2003482-51-56.increment.txt10.31 KBpwolanin
#51 2003482-51.patch78.89 KBpwolanin
#51 2003482-43-51.increment.txt14.95 KBpwolanin
#44 2003482-incremental-37-43.txt28.63 KBpwolanin
#43 2003482-43.patch76.3 KBpwolanin
#43 2003482-37-43-interdiff.txt31.2 KBpwolanin
#41 2003482-41.patch73.29 KBpwolanin
#41 2003482-37-41-interdiff.txt26.73 KBpwolanin
#37 2003482-37.patch73.89 KBpwolanin
#36 2003482-36.patch73.63 KBpwolanin
#32 2003482-32.patch36.01 KBpwolanin
#31 2003482-31.patch36 KBpwolanin
#29 2003482-29.patch34.44 KBpwolanin
#27 interdiff-22-26.txt20.69 KBpwolanin
#26 2003482-26.patch34.44 KBpwolanin
#23 interdiff-16-22.txt6.98 KBjibran
#22 2003482-22.patch29.99 KBpwolanin
#16 2003482-16.patch28.95 KBpwolanin
#14 2003482-14.patch24.49 KBNick_vh
#13 2003482-13.patch24.49 KBNick_vh
#8 2003482-8.patch23.1 KBpwolanin
#5 2003482-5.patch22.88 KBpwolanin
#4 2003482-4.patch23.17 KBNick_vh
#3 2003482-3.patch25.41 KBNick_vh
#2 2003482-1.patch25.41 KBNick_vh
#1 2003482-1.patch927 bytesNick_vh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Nick_vh’s picture

FileSize
927 bytes

Should be a working prototype thanks to pwolanin, tim.plunkett and dawehner!

Nick_vh’s picture

Status: Active » Needs review
FileSize
25.41 KB
Nick_vh’s picture

FileSize
25.41 KB

Third 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.

Nick_vh’s picture

Status: Needs review » Needs work
FileSize
23.17 KB

Last 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".

pwolanin’s picture

Status: Needs work » Needs review
FileSize
22.88 KB

fixes for advanced search, etc.

tim.plunkett’s picture

@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.

pwolanin’s picture

ok, I'm seeing such a typehint error I don't understand why it's happening:

Recoverable fatal error: Argument 1 passed to search_data() must be an instance of SearchExecuteInterface, instance of Drupal\user\Plugin\Search\UserSearchExecute given

The class is:

class UserSearchExecute implements SearchExecuteInterface {

pwolanin’s picture

FileSize
23.1 KB

oh, I see I forgot the "use" declaration.

some other minor cleanup here as well.

Nick_vh’s picture

#8: 2003482-8.patch queued for re-testing.

Berdir’s picture

I'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.

Nick_vh’s picture

Status: Needs review » Needs work
FileSize
24.49 KB

Adding some documentation to make it more complete. Thanks for pointing us to the discovery changes.

Nick_vh’s picture

Status: Needs work » Needs review
FileSize
24.49 KB

Fixes 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.

pwolanin’s picture

FileSize
28.95 KB

reworked the test code to switch to a plugin and ironed out some bugs.

kim.pepper’s picture

This issue touches some of the routing issues found in #1987806: Convert search_view() to a new style controller so added it to the summary.

dawehner’s picture

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearchExecute.phpundefined
@@ -0,0 +1,133 @@
+ * @SearchPagePlugin(

Just wondering whether you have considered to rename it to @SearchPage

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearchExecute.phpundefined
@@ -0,0 +1,133 @@
+ *   title = "Content",

+++ b/core/modules/search/lib/Drupal/search/Annotation/SearchPagePlugin.phpundefined
@@ -0,0 +1,44 @@
+   * The title for the search page tab.
+   *
+   * @var string
+   */
+  public $title;

The title should be translatable, see EntityType as examples

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearchExecute.phpundefined
@@ -0,0 +1,133 @@
+    $query = db_select('search_index', 'i', array('target' => 'slave'))
+      ->extend('Drupal\search\SearchQuery')
+      ->extend('Drupal\Core\Database\Query\PagerSelectExtender');
+    $query->join('node_field_data', 'n', 'n.nid = i.sid');

You could use either ContextFactory or ContainerFactory to inject dependencies into plugins. (Here the database connection).

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearchExecute.phpundefined
@@ -0,0 +1,133 @@
+      $node = node_load($item->sid);
...
+      $node->rendered .= ' ' . module_invoke('comment', 'node_update_index', $node, $item->langcode);
...
+      $extra = module_invoke_all('node_search_result', $node, $item->langcode);

Just a bunch of other dependencies

+++ b/core/modules/search/lib/Drupal/search/Annotation/SearchPagePlugin.phpundefined
@@ -0,0 +1,44 @@
+class SearchPagePlugin extends \Drupal\Component\Annotation\Plugin {

You could just "use" this class.

+++ b/core/modules/search/lib/Drupal/search/SearchExecuteInterface.phpundefined
@@ -0,0 +1,46 @@
+  public function isSearchExecutable();
...
+  public function execute();

If you have an interface you can just use {@inheritdoc} in your implementations and save all this c&p of docs.

pwolanin’s picture

Status: Needs review » Needs work

@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

pwolanin’s picture

@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?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
29.99 KB

trying to resolve the fatal errors (they were still calling the procedural functions).

jibran’s picture

FileSize
6.98 KB

Please add interdiffs it helps a lot.

dawehner’s picture

Status: Needs review » Needs work

@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?

Core 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.

pwolanin’s picture

FileSize
34.44 KB

Here's a significant revision based on discussion with EclipseGC about how to inject dependencies into plugins.

pwolanin’s picture

FileSize
20.69 KB

here's an interdiff too

pwolanin’s picture

Status: Needs work » Needs review
pwolanin’s picture

FileSize
34.44 KB

re-roll against current HEAD - hoping the bot actually tests this one. no actual change from #26

pwolanin’s picture

FileSize
36 KB

fix the fatal error in tests and makes the service naming consistent

pwolanin’s picture

Assigned: Nick_vh » pwolanin
FileSize
36.01 KB

Here's a patch to move away from using context per discussion in IRC.

tim.plunkett’s picture

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearchExecute.phpundefined
@@ -0,0 +1,161 @@
+ *   title = "Content",

+++ b/core/modules/search/tests/modules/search_extra_type/lib/Drupal/search_extra_type/Plugin/Search/SearchExtraSearchExecute.phpundefined
@@ -0,0 +1,97 @@
+ *   title = "Dummy search type",

+++ b/core/modules/user/lib/Drupal/user/Plugin/Search/UserSearchExecute.phpundefined
@@ -0,0 +1,111 @@
+ *   title = "Users",

This should be title = @Translation("Content"),, and remember to use the Translation class.

Otherwise this is looking great.

pwolanin’s picture

Status: Needs review » Needs work

Looking 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.

pwolanin’s picture

@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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
73.63 KB

This patch moves most of the search hooks into plugin methods and makes the plugin class full-featured rather than just for executing a search.

pwolanin’s picture

FileSize
73.89 KB

fixes some notices

tim.plunkett’s picture

NOTE: I reviewed #36. There is no interdiff :( so I don't know what changed.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.phpundefined
@@ -0,0 +1,265 @@
+  protected $database;
+  protected $entity_manager;
+  protected $module_handler;
+  protected $config_factory;
+  protected $state;

These all need docblocks with @var

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.phpundefined
@@ -0,0 +1,265 @@
+  protected $entity_manager;
+  protected $module_handler;
+  protected $config_factory;

Also, lowerCamelCase for these.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.phpundefined
@@ -0,0 +1,265 @@
+    $database = $container->get('database');
+    $entity_manager = $container->get('plugin.manager.entity');
+    $module_handler = $container->get('module_handler');
+    $config_factory = $container->get('config.factory');
+    $state = $container->get('keyvalue')->get('state');
+    return new static($database, $entity_manager, $module_handler, $config_factory, $state, $configuration, $plugin_id, $plugin_definition);

Usually we've been splitting this out like

return new static(
  $container->get('database'),
  $container->get('plugin.manager.entity'),
 ...
);

With no trailing comma on the last one

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.phpundefined
@@ -0,0 +1,265 @@
+  public function __construct(Connection $database, EntityManager $entity_manager, ModuleHandlerInterface $module_handler, ConfigFactory $config_factory, KeyValueStoreInterface $state, array $configuration, $plugin_id, array $plugin_definition) {

Needs a docblock

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.phpundefined
@@ -0,0 +1,265 @@
+    $this->configuration = $configuration;
+    $this->pluginId = $plugin_id;
+    $this->pluginDefinition = $plugin_definition;

This should be done in parent::__construct()

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.phpundefined
@@ -0,0 +1,265 @@
+    $this->entity_manager = $entity_manager;
...
+    $node_storage = $this->entity_manager->getStorageController('node');
+    $node_render = $this->entity_manager->getRenderController('node');

One thing we've done is to get the EntityManager injected, but store the different controllers in properties. Something like

$this->storageController = $entity_manager->getStorageController('node');
+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.phpundefined
@@ -0,0 +1,265 @@
+    $module_handler = $this->module_handler;

No reason to have this variable, $this->module_handler (should be $this->moduleHandler) is fast enough

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.phpundefined
@@ -0,0 +1,265 @@
+   * @param $query

needs a typehint

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.phpundefined
@@ -0,0 +1,265 @@
+    $limit = (int) $this->config_factory->get('search.settings')->get('index.cron_limit');

If 'search.settings' is the only object used, I would also consider storing that directly in __construct instead of the factory

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.phpundefined
@@ -0,0 +1,265 @@
+   * @param \Drupal\Core\Entity\EntityInterface $node
+   *   The node to index.
+   */
+  protected function indexNode(EntityInterface $node) {

This can be \Drupal\node\NodeInterface

+++ b/core/modules/search/lib/Drupal/search/Plugin/SearchInterface.phpundefined
@@ -0,0 +1,144 @@
+  public function setSearch($keywords, array $params, array $attributes);

The params need @param

+++ b/core/modules/search/lib/Drupal/search/Plugin/SearchInterface.phpundefined
@@ -0,0 +1,144 @@
+   * ¶
+   * The core search module only invokes this method on active module plugins.
...
+   * ¶
+   * The core search module only invokes this method on active module plugins.

Trailing whitespace

+++ b/core/modules/search/lib/Drupal/search/Plugin/SearchInterface.phpundefined
@@ -0,0 +1,144 @@
+   * @param $form
...
+   * @param $form_state
...
+   * @param $form
...
+   * @param $form_state

@param array

+++ b/core/modules/search/lib/Drupal/search/Plugin/SearchPluginBase.phpundefined
@@ -0,0 +1,121 @@
+ * Definition of Drupal\search\Plugin\SearchPluginBase

Contains \Drupal\...

+++ b/core/modules/search/lib/Drupal/search/SearchPluginManager.phpundefined
@@ -0,0 +1,41 @@
+   * @param ContainerInterface $container A ContainerInterface instance.

Nope :)

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchAdvancedSearchFormTest.phpundefined
@@ -29,7 +29,7 @@ function setUp() {
+    \Drupal::service('plugin.manager.search')->createInstance('node_search')->updateIndex();

You can actually do $this->container->get('... in tests

+++ b/core/modules/search/search.admin.incundefined
@@ -33,8 +33,10 @@ function search_admin_settings($form, &$form_state) {
+  foreach(search_get_info() as $module => $search_info) {

@@ -107,12 +109,9 @@ function search_admin_settings($form, &$form_state) {
+  foreach($active_plugins as $plugin) {

@@ -146,7 +145,11 @@ function search_admin_settings_submit($form, &$form_state) {
+  foreach(search_get_info() as $module => $search_info) {

Missing a space after foreach. Also, search_get_info still?

+++ b/core/modules/search/search.moduleundefined
@@ -231,30 +233,31 @@ function search_is_active() {
+  $search_info = &drupal_static(__FUNCTION__);

If you're using CacheDecorator it already has a static cache, this is overkill.

+++ b/core/modules/search/search.moduleundefined
@@ -231,30 +233,31 @@ function search_is_active() {
+      $search_info[$module] += array('title' => $module, 'path' => $module, 'module' => $module);

These should be added by the plugin manager or something. We shouldn't wrap the plugin manager getDefinitions()

+++ b/core/modules/search/search.pages.incundefined
@@ -15,7 +15,7 @@
  * @param $keys
  *   Keywords to use for the search.
  */
-function search_view($module = NULL, $keys = '') {
+function search_view($plugin_id = NULL, $module = NULL, $keys = '') {

Probably needs to update the @param

+++ b/core/modules/user/lib/Drupal/user/Plugin/Search/UserSearch.phpundefined
@@ -0,0 +1,105 @@
+class UserSearch extends SearchPluginBase {

Mostly the same comments as for NodeSearch

pwolanin’s picture

Status: Needs review » Needs work

@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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
26.73 KB
73.29 KB
pwolanin’s picture

This fixes most of the tet failures. interdiff is to 37 again, since that's basically what Tim reviewed last.

pwolanin’s picture

ok, posting the incremenetal diff from 37 to 43 per Tim.

tim.plunkett’s picture

tim.plunkett’s picture

Issue summary: View changes

Added a link to controller conversion

pwolanin’s picture

effulgentsia’s picture

Issue tags: +Plugin-conversion

Tagging

kim.pepper’s picture

Issue tags: +Needs reroll
pwolanin’s picture

ok, so let's re-roll and not wait for config conversion

pwolanin’s picture

Status: Needs work » Needs review
FileSize
14.95 KB
78.89 KB

Here's an initial attempt at rebase and re-roll. The incremental diff may lie a bit due to the rebasing.

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.phpundefined
@@ -0,0 +1,271 @@
+  protected $database;
+  protected $entityManager;
+  protected $moduleHandler;
+  protected $searchSettings;
+  protected $state;
...
+  public function __construct(Connection $database, EntityManager $entity_manager, ModuleHandlerInterface $module_handler, Config $search_settings, KeyValueStoreInterface $state, array $configuration, $plugin_id, array $plugin_definition) {

Just a note: let's document these variables.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.phpundefined
@@ -0,0 +1,271 @@
+    $node_storage = $this->entityManager->getStorageController('node');
...
+    $node_storage = $this->entityManager->getStorageController('node');

As the storage controller is needed in some places it could move to the constructor.

+++ b/core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.phpundefined
@@ -92,8 +105,11 @@ public function buildForm(array $form, array &$form_state) {
+    foreach(search_get_info() as $module => $search_info) {
+      $active_plugins[$module] = $this->searchPluginManager->createInstance($search_info['id']);

@@ -172,12 +188,9 @@ public function buildForm(array $form, array &$form_state) {
+    foreach($active_plugins as $plugin) {

@@ -217,7 +230,11 @@ public function submitForm(array &$form, array &$form_state) {
+    foreach(search_get_info() as $module => $search_info) {

lets use spaces after the 'foreach'

+++ b/core/modules/search/lib/Drupal/search/Plugin/SearchInterface.phpundefined
@@ -0,0 +1,140 @@
+   * @return \Drupal\ssearch\Plugin\SearchInterface

"ssearch"

+++ b/core/modules/search/lib/Drupal/search/SearchPluginManager.phpundefined
@@ -0,0 +1,41 @@
+class SearchPluginManager extends PluginManagerBase {
+
+  /**
+   * Overrides \Drupal\Component\Plugin\PluginManagerBase::__construct().
+   *
+   * @param \Traversable $namespaces
+   *   An object that implements \Traversable which contains the root paths
+   *   keyed by the corresponding namespace to look for plugin implementations.
+   * @param ContainerInterface $container A ContainerInterface instance.
+   */
+  public function __construct(\Traversable $namespaces) {
+    $annotation_namespaces = array('Drupal\search\Annotation' => $namespaces['Drupal\search']);
+    $this->discovery = new AnnotatedClassDiscovery('Search', $namespaces, $annotation_namespaces, 'Drupal\search\Annotation\SearchPlugin');
+    $this->discovery = new AlterDecorator($this->discovery, 'search_info');
+    $this->discovery = new CacheDecorator($this->discovery, 'search');
+
+    // By using ContainerFactory, we call a static create() method on each
+    // plugin.
+    $this->factory = new ContainerFactory($this->discovery);

This could be simplified a lot by using the DefaultPluginManager

+++ b/core/modules/search/search.pages.incundefined
@@ -44,7 +44,9 @@ function search_view($module = NULL, $keys = '') {
+  $plugin->setSearch($keys, $request->query->all(), $request->attributes->all());

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.

jhodgdon’s picture

pwolanin 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.

jhodgdon’s picture

pwolanin 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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
10.31 KB
80.31 KB

This 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

$request->attributes->all()

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.

pwolanin’s picture

Issue tags: -Needs reroll

doesn't need reroll

Nick_vh’s picture

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchRankingTest.phpundefined
@@ -91,6 +91,11 @@ function testRankings() {
+    // @todo - comments and views are removed from the array since they are
+    // broken in core. Those modules expected hook_update_index() to be called
+    // even though it was only called on modules that implemented a search type.
+    array_pop($node_ranks);
...
     foreach ($node_ranks as $node_rank) {

This 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.

pwolanin’s picture

ok, 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.

Nick_vh’s picture

Status: Needs review » Reviewed & tested by the community
YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

jhodgdon’s picture

RE #59 - I've bumped that issue up to 8.x.

tim.plunkett’s picture

FileSize
24.64 KB
86.24 KB

This 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.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
847 bytes
86.24 KB

That was a really stupid mistake.

Status: Needs review » Needs work

The last submitted patch, search-2003482-65.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.63 KB
86.01 KB

/facepalms

Nick_vh’s picture

Status: Needs review » Reviewed & tested by the community

Looking good. Marking as RTBC

pwolanin’s picture

Status: Reviewed & tested by the community » Needs work

I'm wondering why the change to array_filter() + anonymous function? I don't think that's a pattern we should be using.

tim.plunkett’s picture

We use it in many places. Why not?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
6.45 KB
86.13 KB

@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.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I like getActivePlugins(). I don't care about foreach vs closure. This is fine.

jibran’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.phpundefined
@@ -106,10 +110,9 @@ public function buildForm(array $form, array &$form_state) {
+    $active_plugins = $this->searchPluginManager->getActivePlugins();
+    foreach ($active_plugins as $plugin) {

Why this extra line?

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.phpundefined
@@ -0,0 +1,335 @@
+        '#account' => user_load($node->uid),

we have entityManger available here.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.phpundefined
@@ -0,0 +1,335 @@
+        'type' => check_plain(node_get_type_label($node)),

node_type is entity now so we can also use entityManger here.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.phpundefined
@@ -0,0 +1,335 @@
+        '#default_value' => variable_get('node_rank_' . $var, 0),
...
+        variable_set('node_rank_' . $var, $form_state['values']['node_rank_' . $var]);

Can we add @todo here with issue link?

+++ b/core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.phpundefined
@@ -41,13 +50,17 @@ class SearchSettingsForm extends SystemConfigFormBase {
+  public function __construct(ConfigFactory $config_factory, SearchPluginManager $manager, ModuleHandlerInterface $module_handler, KeyValueStoreInterface $state) {

@@ -57,7 +70,8 @@ public function __construct(Config $search_settings, ModuleHandler $module_handl
   public static function create(ContainerInterface $container) {

Can we swap location of __construct and create?

+++ b/core/modules/search/tests/modules/search_extra_type/lib/Drupal/search_extra_type/Plugin/Search/SearchExtraTypeSearch.phpundefined
@@ -0,0 +1,142 @@
+  public function __construct(Config $config_settings, array $configuration, $plugin_id, array $plugin_definition) {

Doc missing.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Search/UserSearch.phpundefined
@@ -0,0 +1,114 @@
+  public function __construct(Connection $database, EntityManager $entity_manager, ModuleHandlerInterface $module_handler, array $configuration, $plugin_id, array $plugin_definition) {

Doc missing.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Search/UserSearch.phpundefined
@@ -0,0 +1,114 @@
+      $query->condition(db_or()->

we can replace db_or with $query->orConditionGroup() thanks to @chx see #1921426: [Followup] Move node access storage to DIC.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Search/UserSearch.phpundefined
@@ -0,0 +1,114 @@
+        condition('name', '%' . db_like($keys) . '%', 'LIKE')->

we can replace db_like with $this->database->escapeLike($string); just confirmed this with @dawehner on IRC

jibran’s picture

Thanks @tim.plunkett for the correction no need to swap __construct and create.

tim.plunkett’s picture

Priority: Normal » Critical
Issue tags: +Approved API change
kim.pepper’s picture

Status: Needs work » Needs review
FileSize
7.16 KB
87.94 KB

Applies fixes as per #73. Think I have it all right.

Made a couple of docs cleanups along the way.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Re-RTBC, thanks @kim.pepper!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2003482-search-plugin-76.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.phpundefined
@@ -78,22 +92,26 @@ public function getFormID() {
+    foreach ($this->searchPluginManager->getActivePlugins() as $plugin) {

@@ -172,12 +190,9 @@ public function buildForm(array $form, array &$form_state) {
+    foreach ($active_plugins as $plugin) {

Ah, THAT'S why we had it in a temporary variable. Let's revert that bit.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
794 bytes
87.98 KB

Reverted...

kim.pepper’s picture

Too hasty! I reverted the wrong line :-(

Trying again.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks.

catch’s picture

Priority: Critical » Normal
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll

curl https://drupal.org/files/2003482-search-plugin-81.patch | git apply --check
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 90095  100 90095    0     0  44734      0  0:00:02  0:00:02 --:--:-- 51927
error: patch failed: core/modules/node/node.module:1708
error: core/modules/node/node.module: patch does not apply
error: patch failed: core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.php:6
error: core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.php: patch does not apply
error: patch failed: core/modules/search/lib/Drupal/search/Tests/SearchCommentCountToggleTest.php:66
error: core/modules/search/lib/Drupal/search/Tests/SearchCommentCountToggleTest.php: patch does not apply
error: patch failed: core/modules/search/lib/Drupal/search/Tests/SearchConfigSettingsFormTest.php:49
error: core/modules/search/lib/Drupal/search/Tests/SearchConfigSettingsFormTest.php: patch does not apply
error: patch failed: core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php:114
error: core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php: patch does not apply
error: patch failed: core/modules/search/lib/Drupal/search/Tests/SearchRankingTest.php:90
error: core/modules/search/lib/Drupal/search/Tests/SearchRankingTest.php: patch does not apply
error: patch failed: core/modules/search/search.api.php:316
error: core/modules/search/search.api.php: patch does not apply
Berdir’s picture

That'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.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
88.09 KB

That 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.

jibran’s picture

Issue tags: -Needs reroll

@kim.pepper I know the pain. I have wasted half an hour on re-roll as well and then hit git rebase --abort

+++ b/core/modules/search/search.moduleundefined
@@ -217,52 +217,17 @@ function search_menu() {
+  return user_access('search content') && Drupal::service('plugin.manager.search')->getActiveDefinitions();

+++ b/core/modules/user/lib/Drupal/user/Plugin/Search/UserSearch.phpundefined
@@ -0,0 +1,131 @@
+    if (user_access('administer users')) {
...
+      if (user_access('administer users')) {

user_access to $account->hasPermission()
I think we can replace it easily.

Status: Needs review » Needs work

The last submitted patch, 2003482-search-plugin-86.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
5.99 KB
92.44 KB

Fixed the fatals, but not getting any active search plugins registered when building the form now.

kim.pepper’s picture

FileSize
861 bytes
92.45 KB

Refactored to handle #2043379: Allow plugins to be discovered in any directory

Getting a redirect loop on search/node now.

kim.pepper’s picture

Re-installed, and don't get the redirect loop issues anymore. Spoke with @timplunkett on IRC and he said we have coverage for that already.

kim.pepper’s picture

Fixes for user_access as per #87

jibran’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/lib/Drupal/user/Plugin/Search/UserSearch.phpundefined
@@ -0,0 +1,132 @@
+    $user_account = \Drupal::request()->get('account');

We can inject request using create method. It should be Drupal::request()->attributes->get('account') anyways. See https://drupal.org/node/2049309

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
3.17 KB
92.89 KB

Fixed the issue with missing ->attributes in #93 and injecting the request now.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC after #82.

jibran’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch doesn't apply anymore needs reroll after #2045919: Replace all module_implements() deprecated function calls..

jibran’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
683 bytes
92.96 KB

Here is a reroll and back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2003482-97.patch, failed testing.

tim.plunkett’s picture

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
92.96 KB

fix 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.

Status: Needs review » Needs work

The last submitted patch, 2003482-99.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
619 bytes
92.91 KB

It passes locally after reverting the change of #97.

kim.pepper’s picture

Would RTBC, but had too much input into patches.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

I have only rerolled the patch and reverted the only change I did in #97 so I can RTBC it so back to RTBC.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
49.83 KB
125.08 KB

Per 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

Status: Needs review » Needs work

The last submitted patch, search-2003482-105.patch, failed testing.

pwolanin’s picture

pwolanin’s picture

Status: Needs work » Needs review
FileSize
743 bytes
125.03 KB

Simple fix.

Status: Needs review » Needs work
Issue tags: -Plugin-conversion, -RTBC July 1, -Approved API change

The last submitted patch, search-2003482-108.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
Issue tags: +Plugin-conversion, +RTBC July 1, +Approved API change

#108: search-2003482-108.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Nice. Berdir would be proud.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

I 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:

+      '#description' => t('Choose which search plugins are active from the available pluginss.')

(last word there)

b) in SearchInterface:

+  /**
+   * Set the keywords, params, and attributes to be used by execute().
+   *
+   * @param $keywords
+   * @param array $params
+   * @param array $attributes
+   *
+   * @return \Drupal\search\Plugin\SearchInterface
+   *   The object.
+   */
+  public function setSearch($keywords, array $params, array $attributes);

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:

+   * @return boolean
+   *   A true or false depending on the implementation.
+   */
+  public function isSearchExecutable();

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:

+ *   specified, $plugin_is must also be given. Omit both $sid and $plugin_id to clear

h) search_cron() docs still refer to hook_update_index():

+ * Fires hook_update_index() in all plugins and cleans up dirty words.

i) in search_index():

- * @param $module
- *   The machine-readable name of the module that this item comes from (a
- *   module that implements hook_search_info()).
+ * @param $type
+ *   The entity type or other machine-readable type of this item.

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:

+ * 
+ * To be discovered, the plugins must implement
+ * 

ummm... looks unfinished.

k) Member variables for classes need one-line doc summaries:

+class UserSearch extends SearchPluginBase implements AccessibleInterface {
+
+  /**
+   * @var \Drupal\Core\Database\Connection
+   */
+  protected $database;

and the rest of them in this class.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
8.53 KB
125.57 KB

Fixes docs in #112 except for i) which needs someone more intimate knowledge of the change to comment on.

jhodgdon’s picture

Status: Needs review » Needs work

Looks good!

Still a few things to fix:

a) SearchInterface:

+   * @return bool
+   *   TRUE if the search settings are valid, and FALSE if it isn't.
+   */
+  public function isSearchExecutable();

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)?

pwolanin’s picture

re: 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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
4.05 KB
121.04 KB

This changes params to parameters and tries to improve the doxygen.

jhodgdon’s picture

RE #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:

function search_index($sid, $module, $text) {
...
    db_merge('search_index')
      ->key(array(
        'word' => $word,
        'sid' => $sid,
        'type' => $module,
      ))
      ->fields(array('score' => $score))
      ->expression('score', 'score + :score', array(':score' => $score))
      ->execute();
pwolanin’s picture

@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.

jhodgdon’s picture

OK, 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.

pwolanin’s picture

This fixes some silly fails due to param -> parameters change that I missed in the test plugin.

jhodgdon’s picture

Status: Needs review » Needs work

pwolanin 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:

/**
 * Defines a SearchPlugin type annotation object.
 *
 * SearchPlugin classes define search types for the core Search module. Each
 * active search type is displayed in a tab on the Search page, and each has a
 * URL suffix after "search/".
 *
 * @see SearchPluginBase
 *
 * @Annotation
 */
class SearchPlugin extends Plugin {

  /**
   * A unique identifier for the search plugin.
   *
   * @var string
   */
  public $id;

  /**
   * The path fragment to be added to search/ for the search page.
   *
   * @var string
   */
  public $path;

  /**
   * The title for the search page tab.
   *
   * @ingroup plugin_translatable
   *
   * @var string
   */
  public $title;
}

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).

jhodgdon’s picture

continuation of (a) Actually, SearchPlugin needs to have an @see to SearchInterface too.

b) Let's add some explanation to SearchPluginBase/SearchInterface:

/**
 * Provides a base class for implementing \Drupal\search\Plugin\SearchInterface.
 */
abstract class SearchPluginBase ...
/**
 * Defines a common interface for \Drupal\search\Annotation\SearchPlugin.
 *
 * SearchPlugin classes define search types for the core Search module, and
 * they must implement this interface.
 */
interface SearchInterface extends PluginInspectionInterface {

  /**
   * Sets the keywords, parameters, and attributes to be used by execute().
   *
   * The Search module calls this method to pass in information entered by
   * the user when a search is requested on this search type.
   *
   * @param string $keywords
   *   The keywords to use in a search.
...
  */
  public function setSearch($keywords, array $parameters, array $attributes);

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.

jhodgdon’s picture

e) 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.

pwolanin’s picture

@johgdon - a counter-argument to type being the pluign ID is here:

/**
 * Changes a node's changed timestamp to 'now' to force reindexing.
 *
 * @param $nid
 *   The node ID of the node that needs reindexing.
 */
function search_touch_node($nid) {
  db_update('search_dataset')
    ->fields(array('reindex' => REQUEST_TIME))
    ->condition('type', 'node')
    ->condition('sid', $nid)
    ->execute();
}

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'.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
42.94 KB
145.94 KB

I 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.

Status: Needs review » Needs work

The last submitted patch, 2003482-125.patch, failed testing.

jhodgdon’s picture

RE #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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
146.96 KB
146.96 KB

Fixes a bug in forming the query string, and a test looking for the filter in the wrong place.

pwolanin’s picture

oops, posted the patch 2x, here's the interdiff.

pwolanin’s picture

@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?

jhodgdon’s picture

RE #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.

jhodgdon’s picture

that was an unintentional tag change there ?!?.

jhodgdon’s picture

pwolanin 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:

<a href="http://example.com/node/12345">pizza</a>

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.

jhodgdon’s picture

Oh. 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.

jhodgdon’s picture

Oh, 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?

pwolanin’s picture

I'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.

pwolanin’s picture

noticed some unintended changes to the settings form context param - this restores them.

jhodgdon’s picture

Status: Needs review » Needs work

I 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:

+  protected $advanced = array(
+    'type' => array('column' => 'n.type'),
+    'langcode' => array('column' => 'n.langcode'),
+    'user' => array('column' => 'n.uid'),
+    'term', array('column' => 'ti.tid', 'join' => array('table' => 'taxonomy_index', 'alias' => 'ti', 'condition' => 'n.nid = ti.nid')),
+  );

- 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

-  search_touch_node($comment->nid->target_id);
+  search_mark_for_reindex('node_search', $comment->nid->target_id);

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

+  /**
+   * Executes the search and build a render array.
+   *
+   * @return array
+   *   The search results in a renderable array.
+   */
+  public function buildResults();

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()

+ * @param string $plugin_id
+ *   The name of a search plugin (e.g., 'node_search')

Needs . at end of docs.

h) search_mark_for_reindex docs:

+ *  * @param $sid
+ *   An ID number identifying this particular item (e.g., node ID).

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.

pwolanin’s picture

re: 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

jhodgdon’s picture

OK.

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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
5.96 KB
161.67 KB

Re-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:

?f[]=type:page&f[]=term:27&f[]=term:13&f[]=langcode:en

And this in code:

$f= array('type:page', 'term:27', 'term:13', 'langcode:en');

so imploding that gives a string like we used to have in the advanced search keywords:

'type:page term:27 term:13 langcode:en'

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.

jhodgdon’s picture

Hm. 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:

$facets = array();
foreach ($f as $item) {
  $parts = explode(':', $item);
  $facets[$parts[0]][] = $parts[1];
}

(might need some testing to see if $parts is really 2 elements etc.) That would turn $f into an array something like:

term => array(27, 13)
lang => array(en)

etc.

pwolanin’s picture

@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.

jhodgdon’s picture

Agreed, 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.

jhodgdon’s picture

I 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:

      // Update index.
      $this->moduleHandler->invoke('search', 'index', array($node->id(), $this->getPluginId(), $text, $language->id));

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

   * @var \Drupal\Core\Annotation\Translation

instead of @var string. Should our Search annotation class be doing that too? I really don't know...

pwolanin’s picture

I 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?

tim.plunkett’s picture

if ($this->moduleHandler->moduleExists('search')) {
  search_index($whatever);
}
jhodgdon’s picture

RE #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.

tim.plunkett’s picture

I 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

jhodgdon’s picture

Status: Needs review » Needs work

Yeah, 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?

pwolanin’s picture

tim 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.

pwolanin’s picture

Status: Needs work » Needs review
pwolanin’s picture

oops, mis-made the interdiff

tim.plunkett’s picture

@@ -330,7 +330,7 @@ protected function indexNode(EntityInterface $node) {
+      $build = node_view($node, 'search_index', $language->id);

Hate to say it, but the render controller should be injected here. See what entity_view is wrapping.

Status: Needs review » Needs work

The last submitted patch, 2003482-151.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.93 KB
173.07 KB

Oops, 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

pwolanin’s picture

@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

pwolanin’s picture

re: #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.

jhodgdon’s picture

I 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.

pwolanin’s picture

FileSize
838 bytes
173.12 KB

@johgdon - re: node facets, I don't think there is any assumption (I think I addressed this above)

The patterns is:

$pattern = '/(' . implode('|', array_keys($this->advanced)) . '):([^ ]*)/i';

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.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Ah yes, I see. OK, I think this is ready to go in, after only 160 comments, reviews, and reworkings of patches.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

One last thing.

  1. +++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.php
    @@ -0,0 +1,559 @@
    + *   title = "Content",
    

    This needs to be @Translation("Content") (same with other plugins)

  2. +++ b/core/modules/search/lib/Drupal/search/Annotation/SearchPlugin.php
    @@ -0,0 +1,47 @@
    +   * @var string
    

    This should be @var \Drupal\Core\Annotation\Translation

  3. +++ b/core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.php
    @@ -70,38 +91,39 @@ public function getFormID() {
    +      $options[$plugin_id] = t($search_info['title']) . ' (' . $plugin_id . ')';
    

    We can't use t() like this.

  4. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchRankingTest.php
    @@ -143,7 +148,7 @@ function testHTMLRankings() {
    +    $this->container->get('plugin.manager.search')->createInstance('node_search')->updateIndex();
    
    @@ -154,7 +159,10 @@ function testHTMLRankings() {
    +    $plugin = $this->container->get('plugin.manager.search')->createInstance('node_search');
    
    @@ -173,13 +181,15 @@ function testHTMLRankings() {
    +      $this->container->get('plugin.manager.search')->createInstance('node_search')->updateIndex();
    

    Any reason not to reuse this?

jhodgdon’s picture

Re #162 - I was wondering about #1/2 (commented on #145 but pwolanin said it was OK). I missed #3/#4, good catch!

pwolanin’s picture

Status: Needs work » Needs review
FileSize
8.29 KB
173.75 KB

ok, so using @Translation() and putting in a todo that this may be broken until the route conversion follow-up.

jhodgdon’s picture

Looks good to me. +1 for RTBC again but I'll let @tim.plunkett decide.

tim.plunkett’s picture

Almost!

  1. +++ b/core/modules/search/lib/Drupal/search/Annotation/SearchPlugin.php
    @@ -39,9 +39,12 @@ class SearchPlugin extends Plugin {
    +   * @todo: This will potentially be translated twice or cached with the wrong
    +   * translation until the search tabs are converted to local task plugins.
    +   * ¶
    

    No colon after @todo, the second line needs to be indented two spaces, and you have trailing whitespace.

  2. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchRankingTest.php
    @@ -12,6 +12,13 @@
    +   * @var \Drupal\node\Plugin\Search\NodeSearch
    

    This should technically be the interface, not the class itself

  3. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchRankingTest.php
    @@ -26,6 +33,16 @@ public static function getInfo() {
    +  /**
    +   * {@inheritdoc}
    +   */
    

    No docblock (don't ask why)

  4. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchRankingTest.php
    @@ -26,6 +33,16 @@ public static function getInfo() {
    +  function setUp() {
    

    public function setUp()

pwolanin’s picture

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks!

tim.plunkett’s picture

Great, thanks!

pwolanin’s picture

FileSize
174.22 KB

rebased and re-rolled to account for HEAD changes to Drupal::config() from config()

effulgentsia’s picture

pwolanin’s picture

#170: 2003482-170.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +blocker, +Plugin-conversion, +RTBC July 1, +Approved API change

The last submitted patch, 2003482-170.patch, failed testing.

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
174.02 KB

re-roll for conflict in the use statements in user.module

pwolanin’s picture

#174: 2003482-174.patch queued for re-testing.

jhodgdon’s picture

Issue tags: +Avoid commit conflicts

I think it is time to stop having to re-roll this issue after 175 comments.

pwolanin’s picture

#174: 2003482-174.patch queued for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Unfortunately a critical got committed that broke this.

git ac https://drupal.org/files/2003482-174.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  174k  100  174k    0     0  62860      0  0:00:02  0:00:02 --:--:-- 67936
error: patch failed: core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php:114
error: core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php: patch does not apply
Berdir’s picture

Status: Needs work » Needs review
FileSize
3.02 KB
173.64 KB

The only conflict was this

        // Each searchable node that we created contains values in the body field
++<<<<<<< HEAD
 +      // in one or more languages.
 +      $search_result = node_search_execute($node->body->value);
++=======
+       // in one or more languages. Let's pick the last language variant from the
+       // body array and execute a search using that as a search keyword.
+       $body_language_variant = end($node->body);
+       $plugin->setSearch($body_language_variant[0]['value'], array(), array());
+       // Do the search and assert the results.
+       $search_result = $plugin->execute();
++>>>>>>> applied patch

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.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Looks good - glad to know we can remove the BC wrapper now.

pwolanin’s picture

Issue tags: -Needs reroll

removing tag

pwolanin’s picture

#179: 2003482-174.patch queued for re-testing.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

Dries, 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.

effulgentsia’s picture

Issue summary: View changes

added related issues

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

Issue summary: View changes

Add more details and use the issue summary template

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

johogdon and I updated the summary

verified that patch applies cleanly to HEAD commit fb7f2c7c85a8112af8bbd14df3e3198cf4c54b51

pwolanin’s picture

#179: 2003482-174.patch queued for re-testing.

pwolanin’s picture

still applies cleanly to HEAD:

commit 970fdb9e479f15291a9b54ea429c68e1b621607a
Author: Nathaniel Catchpole
Date: Wed Sep 4 12:09:19 2013 +0100

Dries’s picture

Status: Reviewed & tested by the community » Needs work

It 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.

xjm’s picture

pwolanin’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -D8 upgrade path, -Needs upgrade path tests
FileSize
6.74 KB
177.35 KB

Ok, 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.

pwolanin’s picture

Issue tags: +D8 upgrade path

oops - browser funkiness with the tags. adding back the relevant one.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
177.35 KB

I 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):

// The starting databse has user module as the only active search.

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.

effulgentsia’s picture

FileSize
756 bytes
178.21 KB

Updated search.schema.yml to match the config changes made by this issue. Trivial, so leaving at RTBC.

jhodgdon’s picture

Whoops, that got overlooked - thanks effulgentsia!

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +VDC
FileSize
184.08 KB
6.86 KB

This 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.

jhodgdon’s picture

I 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.

dawehner’s picture

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.

This 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!

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -D8 upgrade path, -VDC, -blocker, -Plugin-conversion, -RTBC July 1

What @dawehner said, +1.

jibran’s picture

oh tags please comeback to the issue node.

alexpott’s picture

Title: Convert hook_search_info to plugin system » Change notice: Convert hook_search_info to plugin system
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Considering 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!

jhodgdon’s picture

OMG!

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

foreach($active_modules as $idx => $module) {
    if (isset($map[$module])) {

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!

jhodgdon’s picture

Apparently 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.

catch’s picture

effulgentsia’s picture

removing some unneeded tags

pwolanin’s picture

Status: Active » Fixed
Issue tags: -Needs change record

initial notice posted: https://drupal.org/node/2083471

Berdir’s picture

Title: Change notice: Convert hook_search_info to plugin system » Convert hook_search_info to plugin system
Priority: Major » Normal

Restoring title and priority.

jhodgdon’s picture

I have reviewed the change notice and made some minor copy edits. I think it is good to go.

jhodgdon’s picture

I 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)

mradcliffe’s picture

I 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

tstoeckler’s picture

Berdir’s picture

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.