Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ACF’s picture

Status: Active » Needs review
FileSize
18.77 KB

Status: Needs review » Needs work

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

ACF’s picture

Status: Needs work » Needs review
FileSize
547 bytes
17.49 KB

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.

ACF’s picture

Fix indentation problems.

ACF’s picture

Changed patch to use the moduleHandler.

amateescu’s picture

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.

ACF’s picture

Status: Needs work » Needs review
FileSize
16.84 KB

Updated the patch.

ACF’s picture

Updated the patch.

Crell’s picture

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

Crell’s picture

Status: Needs review » Needs work
ACF’s picture

Status: Needs work » Needs review
FileSize
2.08 KB
17.07 KB

Updated with change to the doc block.

ACF’s picture

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

Crell’s picture

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.

ACF’s picture

Status: Needs work » Needs review
FileSize
732 bytes
17.07 KB

Status: Needs review » Needs work

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

ACF’s picture

Status: Needs work » Needs review
FileSize
17.08 KB

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.

ACF’s picture

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.

mtift’s picture

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.

ACF’s picture

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.

ACF’s picture

Status: Postponed » Needs review
ACF’s picture

ParisLiakos’s picture

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}

ACF’s picture

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.

ACF’s picture

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.

Crell’s picture

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.

mparker17’s picture

Assigned: Unassigned » mparker17

I'll help :)

mparker17’s picture

Assigned: mparker17 » Unassigned
Status: Needs work » Needs review
FileSize
689 bytes
17.81 KB

Try this...

mparker17’s picture

Status: Needs review » Needs work

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

mparker17’s picture

Assigned: Unassigned » mparker17

I will fix.

mparker17’s picture

Assigned: mparker17 » Unassigned
Status: Needs work » Needs review
FileSize
577 bytes
17.81 KB

Try this.

Crell’s picture

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.

tim.plunkett’s picture

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

pwolanin’s picture

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.

tim.plunkett’s picture

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.

pwolanin’s picture

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

pwolanin’s picture

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

pwolanin’s picture

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

tim.plunkett’s picture

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

pwolanin’s picture

Status: Needs work » Needs review
FileSize
17.86 KB

re-roll and cleanup per tim's comments.

pwolanin’s picture

FileSize
17.72 KB

ok, taking that line out and reformatting a bit

pwolanin’s picture

FileSize
17.73 KB

plus doxygen fix.

pwolanin’s picture

here's incremental diff from 33 to 44

Status: Needs review » Needs work

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

pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.46 KB
18.03 KB
pwolanin’s picture

tim.plunkett’s picture

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!

alexpott’s picture

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.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.