Files: 
CommentFileSizeAuthor
#48 1933518-48.patch18.2 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 56,363 pass(es).
[ View ]
#48 1933518-incremental-44-48.txt3.84 KBpwolanin
#47 1933518-46.patch18.03 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#47 1933518-incremental-44-46.txt3.46 KBpwolanin
#45 1933518-incremental-33-44.txt9.97 KBpwolanin
#44 1933518-44.patch17.73 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 55,036 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#43 1933518-43.patch17.72 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#42 1933518-41.patch17.86 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#33 drupal-convert-search-system_config_form-1933518-33.patch17.81 KBmparker17
PASSED: [[SimpleTest]]: [MySQL] 55,802 pass(es).
[ View ]
#33 interdiff.txt577 bytesmparker17
#30 drupal-convert-search-system_config_form-1933518-30.patch17.81 KBmparker17
FAILED: [[SimpleTest]]: [MySQL] 55,639 pass(es), 39 fail(s), and 14 exception(s).
[ View ]
#30 interdiff.txt689 bytesmparker17
#25 1933518-forminterface-search-drupal-25.patch17.63 KBACF
PASSED: [[SimpleTest]]: [MySQL] 56,219 pass(es).
[ View ]
#16 1933518-forminterface-search-drupal-16.patch17.08 KBACF
PASSED: [[SimpleTest]]: [MySQL] 53,384 pass(es).
[ View ]
#14 1933518-forminterface-search-drupal-14.patch17.07 KBACF
FAILED: [[SimpleTest]]: [MySQL] 52,591 pass(es), 1 fail(s), and 4 exception(s).
[ View ]
#14 interdiff.txt732 bytesACF
#12 1933518-forminterface-search-drupal-12.patch17.07 KBACF
FAILED: [[SimpleTest]]: [MySQL] 52,582 pass(es), 1 fail(s), and 4 exception(s).
[ View ]
#11 1933518-forminterface-search-drupal-11.patch17.07 KBACF
FAILED: [[SimpleTest]]: [MySQL] 52,658 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#11 interdiff.txt2.08 KBACF
#7 1933518-forminterface-search-drupal-7.patch16.84 KBACF
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#5 1933518-forminterface-search-drupal-5.patch17.72 KBACF
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#4 1933518-forminterface-search-drupal-4.patch17.36 KBACF
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#3 1933518-forminterface-search-drupal-3.patch17.49 KBACF
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#3 interdiff.txt547 bytesACF
#1 1933518-forminterface-search-drupal-1.patch18.77 KBACF
FAILED: [[SimpleTest]]: [MySQL] 52,217 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new18.77 KB
FAILED: [[SimpleTest]]: [MySQL] 52,217 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1933518-forminterface-search-drupal-1.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new547 bytes
new17.49 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

This should fix the issue. Forgot to explicitly add the submit function and it is needed as other submit functions are added to this form and it was in the old version of the form.

StatusFileSize
new17.36 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Fix indentation problems.

StatusFileSize
new17.72 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Changed patch to use the moduleHandler.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new16.84 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Updated the patch.

Updated the patch.

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

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new2.08 KB
new17.07 KB
FAILED: [[SimpleTest]]: [MySQL] 52,658 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Updated with change to the doc block.

StatusFileSize
new17.07 KB
FAILED: [[SimpleTest]]: [MySQL] 52,582 pass(es), 1 fail(s), and 4 exception(s).
[ View ]

Changed 'route' => to 'route_name' =>

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new732 bytes
new17.07 KB
FAILED: [[SimpleTest]]: [MySQL] 52,591 pass(es), 1 fail(s), and 4 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1933518-forminterface-search-drupal-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new17.08 KB
PASSED: [[SimpleTest]]: [MySQL] 53,384 pass(es).
[ View ]

Think this should fix the error.

Status:Needs review» Needs work
Issue tags:-FormInterface

The last submitted patch, 1933518-forminterface-search-drupal-16.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+FormInterface

The last submitted patch, 1933518-forminterface-search-drupal-16.patch, failed testing.

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.

Status:Needs work» Postponed

Hey thanks for looking at this. I had been looking myself and this patch #1934738: Exception: Serialization of 'Closure' is not allowed in serialize() (line 154 of dblog.module) seems to fix things for me locally as well, so putting this issue on hold until one of these drops.

Status:Postponed» Needs review

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}

StatusFileSize
new17.63 KB
PASSED: [[SimpleTest]]: [MySQL] 56,219 pass(es).
[ View ]

Re rolled. moved the re-index submit into the searchsettingsform class so I could then remove the system.admin.inc. Think this should be okay.

Status:Needs review» Needs work
Issue tags:-FormInterface

The last submitted patch, 1933518-forminterface-search-drupal-25.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+FormInterface

#25: 1933518-forminterface-search-drupal-25.patch queued for re-testing.

It seemed like a random fail, it was 1 fail dateintltest fail.

Status:Needs review» Needs work

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

Assigned:Unassigned» mparker17

I'll help :)

Assigned:mparker17» Unassigned
Status:Needs work» Needs review
StatusFileSize
new689 bytes
new17.81 KB
FAILED: [[SimpleTest]]: [MySQL] 55,639 pass(es), 39 fail(s), and 14 exception(s).
[ View ]

Try this...

Status:Needs review» Needs work

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

Assigned:Unassigned» mparker17

I will fix.

Assigned:mparker17» Unassigned
Status:Needs work» Needs review
StatusFileSize
new577 bytes
new17.81 KB
PASSED: [[SimpleTest]]: [MySQL] 55,802 pass(es).
[ View ]

Try this.

Status:Needs review» Reviewed & tested by the community
Issue tags:+WSCCI-conversion

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

Status:Reviewed & tested by the community» Needs work

+++ 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

Status:Needs work» Reviewed & tested by the community

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.

Status:Reviewed & tested by the community» Needs work

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.

Status:Needs work» Needs review
StatusFileSize
new17.86 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

re-roll and cleanup per tim's comments.

StatusFileSize
new17.72 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

ok, taking that line out and reformatting a bit

StatusFileSize
new17.73 KB
FAILED: [[SimpleTest]]: [MySQL] 55,036 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

plus doxygen fix.

StatusFileSize
new9.97 KB

here's incremental diff from 33 to 44

Status:Needs review» Needs work

The last submitted patch, 1933518-44.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.46 KB
new18.03 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

StatusFileSize
new3.84 KB
new18.2 KB
PASSED: [[SimpleTest]]: [MySQL] 56,363 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Fixed

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

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

Issue summary:View changes

Updated issue summary.