diff --git a/core/modules/help_topics/help_topics.install b/core/modules/help_topics/help_topics.install index 02cfadc033..b0b604f0a9 100644 --- a/core/modules/help_topics/help_topics.install +++ b/core/modules/help_topics/help_topics.install @@ -14,10 +14,9 @@ function help_topics_schema() { 'fields' => [ 'sid' => [ 'description' => 'Numeric index of this item in the search index', - 'type' => 'int', + 'type' => 'serial', 'unsigned' => TRUE, 'not null' => TRUE, - 'default' => 0, ], 'plugin_type' => [ 'description' => 'The help plugin type the item comes from', diff --git a/core/modules/help_topics/src/Plugin/HelpSection/HelpTopicSection.php b/core/modules/help_topics/src/Plugin/HelpSection/HelpTopicSection.php index 9d29830327..5c0c160801 100644 --- a/core/modules/help_topics/src/Plugin/HelpSection/HelpTopicSection.php +++ b/core/modules/help_topics/src/Plugin/HelpSection/HelpTopicSection.php @@ -181,11 +181,7 @@ protected function getPlugins() { */ public function listSearchableTopics() { $definitions = $this->pluginManager->getDefinitions(); - $ids = []; - foreach ($definitions as $definition) { - $ids[] = $definition['id']; - } - return $ids; + return array_column($definitions, 'id'); } /** @@ -201,38 +197,47 @@ public function renderTopicForSearch($id, LanguageInterface $language) { // possibly in a different language than the current language. The topic // title and body come from translatable things in the Twig template, so we // need to set the default language to the desired language, render them, - // then reset the default language so we do not screw up other cron things. + // then reset the default language so we do not affect other cron + // processes. Also, just in case there is an exception, wrap the whole + // thing in a try/finally block, and reset the language in the finally part. $old_language = $this->defaultLanguage->get(); - $this->defaultLanguage->set($language); - $topic = []; - - // Render the title in this language. - $title_build = [ - 'title' => [ - '#type' => '#markup', - '#markup' => $plugin->getLabel(), - ], - ]; - $topic['title'] = $this->renderer->renderPlain($title_build); - $cache_meta = CacheableMetadata::createFromRenderArray($title_build); - - // Render the body in this language. For this, we need to set up a render - // context, because the Twig plugins that provide the body assume one - // is present. - $context = new RenderContext(); - $build = [ - 'body' => $this->renderer->executeInRenderContext($context, [$plugin, 'getBody']), - ]; - $topic['text'] = $this->renderer->renderPlain($build); - $cache_meta->addCacheableDependency(CacheableMetadata::createFromRenderArray($build)); - $cache_meta->addCacheableDependency($plugin); - - // Add the other information. - $topic['url'] = $plugin->toUrl(); - $topic['cache_dependency'] = $cache_meta; - - // Reset the language. - $this->defaultLanguage->set($old_language); + try { + $this->defaultLanguage->set($language); + $topic = []; + + // Render the title in this language. + $title_build = [ + 'title' => [ + '#type' => '#markup', + '#markup' => $plugin->getLabel(), + ], + ]; + $topic['title'] = $this->renderer->renderPlain($title_build); + $cacheable_metadata = CacheableMetadata::createFromRenderArray($title_build); + + // Render the body in this language. For this, we need to set up a render + // context, because the Twig plugins that provide the body assume one + // is present. + $context = new RenderContext(); + $build = [ + 'body' => $this->renderer->executeInRenderContext($context, [$plugin, 'getBody']), + ]; + $topic['text'] = $this->renderer->renderPlain($build); + $cacheable_metadata->addCacheableDependency(CacheableMetadata::createFromRenderArray($build)); + $cacheable_metadata->addCacheableDependency($plugin); + if (!$context->isEmpty()) { + $cacheable_metadata->addCacheableDependency($context->pop()); + } + + // Add the other information. + $topic['url'] = $plugin->toUrl(); + $topic['cacheable_metadata'] = $cacheable_metadata; + } + finally { + // Reset the language. + $this->defaultLanguage->set($old_language); + } + return $topic; } diff --git a/core/modules/help_topics/src/Plugin/Search/HelpSearch.php b/core/modules/help_topics/src/Plugin/Search/HelpSearch.php index cd9a4d13e8..3a848d1223 100644 --- a/core/modules/help_topics/src/Plugin/Search/HelpSearch.php +++ b/core/modules/help_topics/src/Plugin/Search/HelpSearch.php @@ -183,7 +183,7 @@ public function execute() { */ protected function findResults() { // We need to check access for the current user to see the topics that - // could get returned by search. Each entry in the help_search_items + // could be returned by search. Each entry in the help_search_items // database has an optional permission that comes from the HelpSection // plugin, in addition to the generic 'access administration pages' // permission. In order to enforce these permissions so only topics that @@ -193,18 +193,16 @@ protected function findResults() { if (!$this->account->hasPermission('access administration pages')) { return NULL; } - $permissions = $this->database + $permissions = array_filter($this->database ->select('help_search_items', 'hsi') ->fields('hsi', ['permission']) ->groupBy('hsi.permission') ->execute() - ->fetchCol(); + ->fetchCol()); $denied_permissions = []; - if ($permissions) { - foreach ($permissions as $permission) { - if ($permission && !$this->account->hasPermission($permission)) { - $denied_permissions[] = $permission; - } + foreach ($permissions as $permission) { + if (!$this->account->hasPermission($permission)) { + $denied_permissions[] = $permission; } } @@ -260,31 +258,33 @@ protected function findResults() { protected function prepareResults(StatementInterface $found) { $results = []; $plugins = []; + $languages = []; $keys = $this->getKeywords(); foreach ($found as $item) { - $result = $this->database->select('help_search_items', 'hsi') + $record = $this->database->select('help_search_items', 'hsi') ->condition('sid', $item->sid) ->fields('hsi', ['plugin_type', 'id']) - ->execute(); - foreach ($result as $record) { - $type = $record->plugin_type; - if (!isset($plugins[$type])) { - $plugins[$type] = $this->getSectionPlugin($type); + ->execute()->fetchObject(); + $type = $record->plugin_type; + if (!isset($plugins[$type])) { + $plugins[$type] = $this->getSectionPlugin($type); + } + if ($plugins[$type]) { + $langcode = $item->langcode; + if (!isset($languages[$langcode])) { + $languages[$langcode] = $this->languageManager->getLanguage($item->langcode); } - if ($plugins[$type]) { - $language = $this->languageManager->getLanguage($item->langcode); - $topic = $plugins[$type]->renderTopicForSearch($record->id, $language); - if ($topic) { - if (isset($topic['cache_dependency'])) { - $this->addCacheableDependency($topic['cache_dependency']); - } - $results[] = [ - 'title' => $topic['title'], - 'link' => $topic['url']->toString(), - 'snippet' => search_excerpt($keys, $topic['title'] . ' ' . $topic['text'], $item->langcode), - 'langcode' => $item->langcode, - ]; + $topic = $plugins[$type]->renderTopicForSearch($record->id, $languages[$langcode]); + if ($topic) { + if (isset($topic['cacheable_metadata'])) { + $this->addCacheableDependency($topic['cacheable_metadata']); } + $results[] = [ + 'title' => $topic['title'], + 'link' => $topic['url']->toString(), + 'snippet' => search_excerpt($keys, $topic['title'] . ' ' . $topic['text'], $item->langcode), + 'langcode' => $item->langcode, + ]; } } } @@ -302,8 +302,7 @@ public function updateIndex() { } // Find some items that need to be updated. Start with ones that have - // never been indexed, and if there is still space in the limit, add in - // ones that are marked to reindex. + // never been indexed. $limit = (int) $this->searchSettings->get('index.cron_limit'); $query = $this->database->select('help_search_items', 'hsi'); @@ -316,6 +315,8 @@ public function updateIndex() { ->range(0, $limit); $items = $query->execute()->fetchAll(); + // If there is still space in the indexing limit, index items that have + // been indexed before, but are currently marked as needing a re-index. if (count($items) < $limit) { $query = $this->database->select('help_search_items', 'hsi'); $query->fields('hsi', ['sid', 'plugin_type', 'id']); @@ -328,7 +329,7 @@ public function updateIndex() { $items = $items + $query->execute()->fetchAll(); } - // Index items, in all available languages. + // Index the items we have chosen, in all available languages. $language_list = $this->languageManager->getLanguages(LanguageInterface::STATE_CONFIGURABLE); $plugins = []; @@ -374,13 +375,9 @@ public function markForReindex() { ->execute() ->fetchAll(); $old_list_ordered = []; - $max_sid = 0; $sids_to_remove = []; foreach ($old_list as $item) { $old_list_ordered[$item->plugin_type][$item->id] = $item; - if ($item->sid > $max_sid) { - $max_sid = $item->sid; - } $sids_to_remove[$item->sid] = $item->sid; } @@ -390,7 +387,7 @@ public function markForReindex() { if (!$plugin) { continue; } - $permission = (isset($plugin_definition['permission']) ? $plugin_definition['permission'] : ''); + $permission = $plugin_definition['permission'] ?? ''; $items = $plugin->listSearchableTopics(); if (!count($items)) { continue; @@ -401,27 +398,26 @@ public function markForReindex() { if ($old_item->permission == $permission) { // Record has not changed. unset($sids_to_remove[$old_item->sid]); + continue; } - else { - // Permission has changed, update record. - $this->database->update('help_search_items') - ->condition('sid', $old_item->sid) - ->fields(['permission' => $permission]) - ->execute(); - } - } - else { - // New record, create it. - $this->database->insert('help_search_items') - ->fields([ - 'sid' => $max_sid + 1, - 'plugin_type' => $plugin_id, - 'permission' => $permission, - 'id' => $id, - ]) + + // Permission has changed, update record. + $this->database->update('help_search_items') + ->condition('sid', $old_item->sid) + ->fields(['permission' => $permission]) ->execute(); - $max_sid++; + unset($sids_to_remove[$old_item->sid]); + continue; } + + // New record, create it. + $this->database->insert('help_search_items') + ->fields([ + 'plugin_type' => $plugin_id, + 'permission' => $permission, + 'id' => $id, + ]) + ->execute(); } } @@ -469,18 +465,17 @@ public function indexStatus() { * Search ID (sid) of item or items to remove. */ protected function removeItemsFromIndex($sids) { - if (!is_array($sids)) { - $sids = [$sids]; - } + $sids = (array) $sids; - // Remove items from our table in batches of 100. - for ($start = 0; $start < count($sids); $start += 100) { - $this_list = array_slice($sids, $start, 100); + // Remove items from our table in batches of 100, to avoid problems + // with having too many placeholders in database queries. + foreach (array_chunk($sids, 100) as $this_list) { $this->database->delete('help_search_items') ->condition('sid', $this_list, 'IN') ->execute(); } - // Remove items from the search tables individually. + // Remove items from the search tables individually, as there is no bulk + // function to delete items from the search index. foreach ($sids as $sid) { search_index_clear($this->getPluginId(), $sid); } diff --git a/core/modules/help_topics/src/SearchableHelpInterface.php b/core/modules/help_topics/src/SearchableHelpInterface.php index 3803259c82..1cac8dea49 100644 --- a/core/modules/help_topics/src/SearchableHelpInterface.php +++ b/core/modules/help_topics/src/SearchableHelpInterface.php @@ -38,7 +38,7 @@ public function listSearchableTopics(); * - title: The title of the topic in this language. * - text: The text of the topic in this language. * - url: The URL of the topic as a \Drupal\Core\Url object. - * - cache_dependency: (optional) An object to add as a cache dependency + * - cacheable_metadata: (optional) An object to add as a cache dependency * if this topic is shown in search results. */ public function renderTopicForSearch($id, LanguageInterface $language); diff --git a/core/modules/help_topics/tests/modules/help_topics_test/src/Plugin/HelpSection/TestHelpSection.php b/core/modules/help_topics/tests/modules/help_topics_test/src/Plugin/HelpSection/TestHelpSection.php index 6b8cdb096e..46e53219ed 100644 --- a/core/modules/help_topics/tests/modules/help_topics_test/src/Plugin/HelpSection/TestHelpSection.php +++ b/core/modules/help_topics/tests/modules/help_topics_test/src/Plugin/HelpSection/TestHelpSection.php @@ -51,13 +51,11 @@ public function renderTopicForSearch($id, LanguageInterface $language) { 'url' => Url::fromUri('https://foo.com'), ]; } - else { - return [ - 'title' => 'Foomm Foreign heading', - 'text' => 'Fake foreign foomm text', - 'url' => Url::fromUri('https://mm.foo.com'), - ]; - } + return [ + 'title' => 'Foomm Foreign heading', + 'text' => 'Fake foreign foomm text', + 'url' => Url::fromUri('https://mm.foo.com'), + ]; break; case 'bar': @@ -68,17 +66,15 @@ public function renderTopicForSearch($id, LanguageInterface $language) { 'url' => Url::fromUri('https://bar.com'), ]; } - else { - return [ - 'title' => 'Barmm Foreign', - 'text' => 'Fake foreign barmm', - 'url' => Url::fromUri('https://mm.bar.com'), - ]; - } + return [ + 'title' => 'Barmm Foreign', + 'text' => 'Fake foreign barmm', + 'url' => Url::fromUri('https://mm.bar.com'), + ]; break; default: - trigger_error('Unexpected ID encountered', E_USER_ERROR); + throw new \InvalidArgumentException('Unexpected ID encountered'); break; } } diff --git a/core/modules/help_topics/tests/src/Functional/HelpTopicSearchTest.php b/core/modules/help_topics/tests/src/Functional/HelpTopicSearchTest.php index 62c39d9d43..ed63d198c6 100644 --- a/core/modules/help_topics/tests/src/Functional/HelpTopicSearchTest.php +++ b/core/modules/help_topics/tests/src/Functional/HelpTopicSearchTest.php @@ -2,6 +2,9 @@ namespace Drupal\Tests\help_topics\Functional; +use Drupal\Tests\Traits\Core\CronRunTrait; +use Drupal\help_topics\Plugin\Search\HelpSearch; + /** * Verifies help topic search. * @@ -9,6 +12,8 @@ */ class HelpTopicSearchTest extends HelpTopicTranslatedTestBase { + use CronRunTrait; + /** * {@inheritdoc} */ @@ -44,12 +49,21 @@ protected function setUp() { 'site_default_language' => 'en', ], 'Save configuration'); - // Clear cache and run cron a few times to update the search index. + // Clear the cache after changing the default language. Without this, or + // if this line is replaced by $this->resetAll(), visiting URLs like + // 'de/search/help' result in 404 errors. $this->drupalPostForm('admin/config/development/performance', [], 'Clear all caches'); - $this->drupalPostForm('admin/config/system/cron', [], 'Run cron'); - $this->drupalPostForm('admin/config/system/cron', [], 'Run cron'); - $this->drupalPostForm('admin/config/system/cron', [], 'Run cron'); - $this->drupalPostForm('admin/config/system/cron', [], 'Run cron'); + + // Run cron until the topics are fully indexed, with a limit of 100 runs + // to avoid infinite loops. + $num_runs = 0; + $indexed = FALSE; + $plugin = HelpSearch::create($this->container, [], 'foo', []); + while (($num_runs < 100) && !$indexed) { + $this->cronRun(); + $status = $plugin->indexStatus(); + $indexed = ($status['remaining'] == $status['total']); + } // Visit the Search settings page and verify it says 100% indexed. $this->drupalGet('admin/config/search/pages');