Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Issue tags: +FormInterface

Updating tag.

RoSk0’s picture

Assigned: Unassigned » RoSk0

Starting...

RoSk0’s picture

Component: shortcut.module » search.module
Status: Active » Needs review
FileSize
4.78 KB

Initial patch version.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

This looks fine to me, and besides which, the tests pass. Thanks!

It's going to conflict with
#1926344: Consolidate search-result.html.twig and search-results.html.twig

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/search/tests/modules/search_embedded_form/lib/Drupal/search_embedded_form/Form/SearchEmbeddedForm.php
@@ -7,16 +7,53 @@
+    $count = $this->container()->get('config.factory')
+      ->get('search_embedded_form.settings')->get('submitted');
...
+    $config = $this->container()->get('config.factory')->get('search_embedded_form.settings');

Should be using FormBase's config method

jhodgdon’s picture

RE #5 - we just need to convert

 $this->container()->get('config.factory')->get()

to

 $this->config()

I believe.

RoSk0: If you'd like to make a new patch, with an interdiff, that would be great!
https://drupal.org/documentation/git/interdiff
If you do not have time, unassign the issue and we'll find someone else. Thanks!

RoSk0’s picture

Status: Needs work » Needs review
FileSize
2.13 KB
4.72 KB

Update.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks!

The interdiff looks right to me.

I also went back and reviewed the whole patch... and I think we should be using the State system instead of the Config system to save the count of how many times the form has been submitted -- it seems like state information and not configuration information. However, I think that is a separate issue. This issue was about converting the existing form to be a form class, and the patch is fine for that. Follow-up: #2122321: search_embedded_form test module is improperly using config instead of state

But there is one small error in this patch:

+ * Form controller for search_embedded_form form.
  */
-class SearchEmbeddedForm {
+class SearchEmbeddedForm extends FormBase {
...
+  public function getFormID() {
+    return 'search_embedded_form_form';
+  }

The docs here say it is for search_embedded_form but the form ID is actually search_embedded_form_form. One or the other should be changed -- they need to match. Thanks!

RoSk0’s picture

Status: Needs work » Needs review
FileSize
730 bytes
4.72 KB

Fixed.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Excellent! I think this is ready to go in now.

jhodgdon’s picture

We have two issues that are postponed on this getting committed -- adding them to related issues. Also I'll click retest just in case since it's been a while since the last test.

jhodgdon’s picture

Xano’s picture

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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