Move index.cron_limit setting to NodeSearch

Actually, I think a better approach would be
#1560820: Use queue for indexing content

And if we don't change to using queues, the limit should be made into a per-plugin setting rather than a global Search module setting. Each search plugin has the opportunity to add settings to the search settings page (node is already doing this), so it would be easy to move this setting over to the node module.

Yeah, anyone is free to read in settings from any module/configuration. So I would go by moving the limit to the node module instead of directly from search. However, that would mean we'd have todo that for user as well.

That is incorrect. User search does not do anything during cron -- it just queries the {users} table directly when you search. There is no indexing involved.

Oh, right, I was completely mixing that variable up with the limit per page - which even doesn't exist in core as a setting, but I was working on a contrib module that exposes that. Sorry for that mix up.

So yeah, just moving that node would be fine enough here.

We've been managing search-related issues in the search.module component rather than node, even though this would be under node.module. So, changing component.

And regarding number of results per page, can I just mention
#702940: Make the number of results per page configurable?
I don't see a strong reason why we couldn't get this fixed in core. It's been requested for a loooooong time... :)

What are you saying in #8? Each search page will have its own settings. The ranking stuff is done at search time and does not go into the index. And the search index is separated out by the page, not by the page type, anyway.

The indexing throttle setting should go in the individual search.page configuration for indexable searches, and should be removed from the main search settings page. We should wait until #2042807: Convert search plugins to use a ConfigEntity and a PluginBag goes in however (or combine it with that issue). Different indexable plugins of different types would have different sensible defaults, and would want different settings. (I ran into that in D7 with the Search by Page contrib module, for instance -- I just had a separate setting for the throttle for that module since indexing took longer.)

And yes I am certain that the index is by page, at least once 2042807 goes in. The indexing code in NodeSearch calls:

      search_index($node->id(), $this->getPluginId(), $text, $language->id);

and $this->getPluginID() returns the individual page config's ID (machine name). Then when searching, the execute method does this:

      ->searchExpression($keys, $this->getPluginId());

which sets up the query to match only records that were indexed by this plugin.

Before 2042807 goes in, the index is by plugin ID as well, but the plugin ID is one per plugin type, since each plugin only has one page.

I was partly wrong in #11 and really in the NodeSearch plugin specifically, we are sharing an index across all individual page plugin instances.

However, the NodeSearch::updateIndex() method will be called once per defined NodeSearch page.

We do want to have the search cron setting be per-page though so you can say "each cron run I want this page to index this many nodes" -- and really what will happen is that the total for all pages will be indexed since each NodeSearch page will just grab the next few nodes to do.

What we should really do in this issue is make the cron limit a configuration option in NodeSearch instead of in search.module.

I think Proposal #1 is the best idea.

Given how things are right now in Core, I don't think anyone would want to configure multiple NodeSearch pages, because the only difference between them would be the rankings.

However, for some contrib plugins (such as the one I need to create for my contrib Search by Page module), you may have each search page have its own index space, in which case you definitely want each search page to have its own cron throttle limit.

So given that it's going to need to be per-page for that type of plugin, I think we should also just make it per-page for the Node search plugin. We should just make it clear (with a description text?) on the configuration page that you are saying how many nodes to index for that particular page per cron run. Which will probably be clear anyway since you are configuring a particular page.

I think proposal 2 (having one setting for all of NodeSearch but displaying it on each NodeSearch page config screen) would be too confusing for users, and too difficult to implement (we'd have to change the way cron is invoked in search.module but only for NodeSearch, because my other search plugin is going to need it invoked for each page as it is now).

Proposal 3 (having the setting be on the main search page but have different settings for each type of index) also seems difficult to implement, and then also NodeSearch will need to get that value, which defeats the original intent of this issue (decoupling search.settings from NodeSearch).

This looks good! It seems like all we need to do now is fix that one test that is setting the cron limit in the old way, and also fix the config update test.

Excellent.

Yes, the migration is correct there is #2171087: Migrate search page settings for the new migration.

Right, I see no problems at all: we will need to add the search_cron_limit to the source (which is d6_variable anyways), 'configuration:cron_limit': search_cron_limit to process and we are done. To test, add the variable and a value of it to the dump, assert to the test and done.

The patch looks OK to me... but it bothers me that there was apparently not a test that checked the ability to set the cron limit from the UI that had to be changed? As it stands right now, it looks like we'll have to test that it works manually, and some later patch could break it without triggering a test failure?

or the test can form alter so that you can index 2...

We could have kind of a hybrid test:
a) Verify that changing settings in the UI updates the configuration value.
b) The test that we have now that verifies that changing the configuration value affects what is indexed in each index run.

[Edit: added] Along with a comment in the test code explaining why it's a hybrid (because the min value in the UI is 10 or whatever it is).

This patch also needs to remove all the existing update code that attempts to update the search cron limit config, or else we'll end up with another issue like #2178205: Delete obsolete config key active_plugins from search module

Your patch is removing the settings save from the SearchPageListController::submitForm() method:

-    $search_settings
-      ->set('index.cron_limit', $form_state['values']['cron_limit'])
-      ->save();

OK, down to nitpicks...

a) Do we need a fieldset/details element for this setting on the config page? It doesn't seem necessary to me to have a fieldset with only one item in it.

b) I know you just copied this from the search settings page, but in the description for that field:

t(...<a href="@cron">cron maintenance task</a> ..., array('@cron' => url('admin/reports/status'))

The url() function is deprecated -- this should use a route. Also, we really should not be sending people to admin/reports/status when we have a Cron config page in the system.module, so can we send them there instead?

c) It looks like the code that saved the search settings in SearchPageListController::submitForm() is still being removed:

-    $search_settings
-      ->set('index.cron_limit', $form_state['values']['cron_limit'])
-      ->save(); 

I don't see the $search_settings->save() being put back in anywhere? Maybe we need a test added to make sure you can save the main search page settings (possibly out of scope for this issue, but then again maybe not)?

d) Oh wait, that patch is very very odd. There are two sections for SearchPageListController... Can you make a new patch? I can't read the existing patch if it has multiple sections for the same file. Weird.

I do see something added to

Um... So do you want a review of this patch, or should we postpone on the other issue?

I do not think we are really going to do #2180389: Switch from item-based to time-based index throttling and #2178643: Indexing status can show a negative percentage of content as having been indexed is fixed, so let's move this back to Active.

If we are going to do this issue, the current patch will not quite work:
- More tests have been added to the throttling test in SearchMultilingualEntityTest.php, so that will need to be updated.
- I don't think we need the additional tests in SearchConfigSettingsFormTest -- one place for testing throttling should be fine, and I think the SearchMultilingualEntityTest covers all the necessary tests.
- It looks like the migration changes are removing the setting from being migrated. Don't they also need to add it back in in the new location for this setting?

However, now that #2178651: Consider UX implications of indexed vs non-indexed search pages has been taken care of, the UI confusion problems are greatly reduced, since the UI now shows us which search pages use the index and which don't. So, I'm not sure we need to do this at all?

Thoughts?

According to my read of the Beta data model policy, I think this would be a beta deadline issue, because it would change the config schema.

So if we want to do this, we'd better do it now.

I'm not sure about the status of this issue overall, but I'd allow minor changes to configuration files between beta and when we provide a beta-beta upgrade path if they're needed to resolve issues (i.e. not for the sake of just changing the file structure).

Once the beta-to-beta upgrade path is supported, I'm not sure the change warrants the risk of an upgrade path between betas.

However if there's a usability improvement in the end, we could possibly do it as an 8.1.x change if there's no API change and there's a well tested update path.

That makes this 'beta target', 'D8 upgrade path', 'minor version target'.

I'm thinking that with #2211241: Refactor search_reindex() into separate functions, which adds notes to the API docs and UI help/descriptions explaining that this setting is a default for pages using the index, and since we've made other changes recently in the UI to explain indexed vs. non-indexed pages, we probably do not really need to do this.

The Node plugin (and any others that want to) can go ahead and use the default setting, and any other plugin that wants to can override with its own setting. So... I'm going to go ahead and close this as "won't fix". We can reopen if we change our minds.