Convert search's system_config_form() to SystemConfigFormBase

Please see the review from #1937968-4: Convert statistics's system_config_form() to SystemConfigFormBase which applies here as well.

+++ b/core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.php
@@ -0,0 +1,210 @@
+  /**
+   * Helper function to get real module names.

This docblock needs to be clearer. The "Verbs X" format says what the method does. It also needs a @return.

+++ b/core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.php
@@ -0,0 +1,210 @@
+  private function SearchGetModuleNames() {

Drupal convention is to use protected only, no private methods.

+++ b/core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.php
@@ -0,0 +1,210 @@
+    module_load_include('inc', 'search', 'search.admin');

This can be replaced with $this->moduleHandler->loadAllIncludes().

+++ b/core/modules/search/search.module
@@ -157,8 +157,7 @@ function search_menu() {
-    'page callback' => 'drupal_get_form',
-    'page arguments' => array('search_admin_settings'),
+    'page callback' => 'NOT_USED',
     'access arguments' => array('administer search'),

This should be "route" => "search_settings", with no page callback specified.

+++ b/core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.php
@@ -0,0 +1,214 @@
+  protected function SearchGetModuleNames() {

Methods should start with a lowercase character. Sorry for not catching that before.

Otherwise I can't find anything else to object to.

Applying just this patch, it fails the BreadcrumbTest (Drupal\system\Tests\Menu\BreadcrumbTest). However, when I apply both this patch and #1872690: Exception: Serialization of 'Closure' is not allowed in serialize, this patch passes the BreadcrumbTest. So it looks as though this patch should be re-tested after 1872690 gets committed.

looks good:) just one thing

+++ b/core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.phpundefined
@@ -0,0 +1,214 @@
+   * Implements \Drupal\Core\ControllerInterface::create().
...
+   * Implements \Drupal\Core\Form\FormInterface::getFormID().
...
+   * Implements \Drupal\Core\Form\FormInterface::buildForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::validateForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::submitForm().

should be {@inheritdoc}

+++ b/core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.php
@@ -0,0 +1,224 @@
+    $this->configFactory = $config_factory;

$this->configFactory isn't defined above It needs to be. Or, actually, you can just grab the one config object we care about here in the constructor and store that as a property.

+++ b/core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.php
@@ -0,0 +1,224 @@
+    $config = $this->configFactory->get('search.settings');

This can be simplified if we move the ->get() up to the constructor, as above.

I'll help :)

Try this...

So, I made a boo-boo. Member variables should be protected, not private.

I will fix.

Try this.

I think this covers it for now. Anything else here would be part of a larger search refactor, which is being done elsewhere.

+++ b/core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.phpundefined
@@ -0,0 +1,231 @@
+    $form['status']['wipe'] = array('#type' => 'submit', '#value' => t('Re-index site'), '#submit' => array(array($form_state['build_info']['callback_object'], 'searchAdminReindexSubmit')));
+    $form['#submit'][] = array($form_state['build_info']['callback_object'], 'submitForm');
...
+    $form['#submit'][] = array($form_state['build_info']['callback_object'], 'submitForm');

All of these $form_state['build_info']['controller'] should just be $this

Turns out the existing tests don't actually test the functionality of this form (apparently it's currently a no-op), so it would be good to see a new test that e.g. fails against current HEAD.

Just because node's implementation of hook_search_admin has been broken for months isn't a reason to hold up this conversion.
But that point from #35 does need to be fixed.

@tim.plunkett - didn't mean to change the status.

Also, just trying to get a handle on this new system - the form object instance is serialized into the form state?

Also, patch doesn't apply is seemd, maybe due to changes in #1947720: Use Drupal::state() to replace state()

@tim - re: #35 - do you really have to declare the submit, etc by hand? I'd have assumed the Form API handles this if the class has an interface requiring those methods?

Oh yeah, I wasn't even paying attention. It does add submitForm for you.

re-roll and cleanup per tim's comments.

ok, taking that line out and reformatting a bit

plus doxygen fix.

here's incremental diff from 33 to 44

That looks great, thanks @ACF and @mparker17 for the initial patch, and @pwolanin for finishing it up!

Committed 7b9e229 and pushed to 8.x. Thanks!