Updated: Comment 0

Problem/Motivation

Proposed resolution

Convert exposed forms to use FormInterface

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
15.6 KB

Let's see what the testbot tells us.

dawehner’s picture

Issue tags: +PHPUnit
FileSize
21.32 KB
5.72 KB

A lot of really great points of tim later.

The last submitted patch, 2: vdc-2147669.patch, failed testing.

tim.plunkett’s picture

FileSize
24.3 KB
5.12 KB

mergeDeep *always* confuses me. I do this wrong every time.

When you add a check for form errors to the end of testGetCache you see that the whole method was flawed, and was not actually testing the proper behavior. This was exposed by these changes.

The last submitted patch, 4: views-form-2147669-4.patch, failed testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

perfect!

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
25.15 KB
870 bytes

The installer is drunk. I manually tested and it failed, and pointed me to what I fixed in the interdiff. I don't know that you could ever have those args be by reference, but it doesn't seem to hurt to remove it now.

tim.plunkett’s picture

FileSize
22.88 KB
3.26 KB

I take it back, not the installer's fault. The mergeDeep is stripping references. Here's an alternate patch.

tim.plunkett’s picture

I don't want to trigger another retest until those two come back, so just leaving this here for now:

diff --git a/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
index 82f245e..2771d22 100644
--- a/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
+++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
@@ -356,7 +356,7 @@ public function testGetFormWithObject() {
   /**
    * Tests the getForm() method with a class name based form ID.
    */
-  public function testgetFormWithClassString() {
+  public function testGetFormWithClassString() {
     $form_id = '\Drupal\Tests\Core\Form\TestForm';
     $object = new TestForm();
     $form = array();
@@ -402,11 +402,10 @@ public function testBuildFormWithObject() {
     $form_id = 'test_form_id';
     $expected_form = $form_id();
 
-    $form_arg = $this->getMockForm(NULL, $expected_form);
+    $form_arg = $this->getMockForm($form_id, $expected_form);
 
-    $form_state['build_info']['callback_object'] = $form_arg;
-
-    $form = $this->formBuilder->buildForm($form_id, $form_state);
+    $form_state = array();
+    $form = $this->formBuilder->buildForm($form_arg, $form_state);
     $this->assertFormElement($expected_form, $form, 'test');
     $this->assertSame($form_id, $form_state['build_info']['form_id']);
     $this->assertSame($form_id, $form['#id']);
@@ -446,12 +445,13 @@ public function testRebuildForm() {
 
     // The form will be built four times.
     $form_arg = $this->getMockForm(NULL, $expected_form, 4);
+    $form_arg->expects($this->exactly(2))
+      ->method('getFormId')
+      ->will($this->returnValue($form_id));
 
     // Do an initial build of the form and track the build ID.
     $form_state = array();
-    $form_state['build_info']['callback_object'] = $form_arg;
-    $form_state['build_info']['args'] = array();
-    $form = $this->formBuilder->buildForm($form_id, $form_state);
+    $form = $this->formBuilder->buildForm($form_arg, $form_state);
     $original_build_id = $form['#build_id'];
 
     // Rebuild the form, and assert that the build ID has not changed.
@@ -463,7 +463,7 @@ public function testRebuildForm() {
 
     // Rebuild the form again, and assert that there is a new build ID.
     $form_state['rebuild_info'] = array();
-    $form = $this->formBuilder->buildForm($form_id, $form_state);
+    $form = $this->formBuilder->buildForm($form_arg, $form_state);
     $this->assertNotSame($original_build_id, $form['#build_id']);
   }
 
@@ -659,7 +659,7 @@ public function testGetCache() {
 
     // FormBuilder::buildForm() will be called twice, but the form object will
     // only be called once due to caching.
-    $form_arg = $this->getMockForm(NULL, $expected_form, 1);
+    $form_arg = $this->getMockForm($form_id, $expected_form, 1);
 
     // The CSRF token is checked each time.
     $this->csrfToken->expects($this->exactly(2))
@@ -678,11 +678,10 @@ public function testGetCache() {
 
     // Do an initial build of the form and track the build ID.
     $form_state = array();
-    $form_state['build_info']['callback_object'] = $form_arg;
     $form_state['build_info']['args'] = array();
     $form_state['build_info']['files'] = array(array('module' => 'node', 'type' => 'pages.inc'));
     $form_state['cache'] = TRUE;
-    $form = $this->formBuilder->buildForm($form_id, $form_state);
+    $form = $this->formBuilder->buildForm($form_arg, $form_state);
 
     $cached_form = $form;
     $cached_form['#cache_token'] = 'csrf_token';
@@ -720,13 +719,11 @@ public function testSendResponse() {
       ->method('prepare')
       ->will($this->returnValue($expected_form));
 
-    $form_arg = $this->getMockForm(NULL, $expected_form);
+    $form_arg = $this->getMockForm($form_id, $expected_form);
 
     // Do an initial build of the form and track the build ID.
     $form_state = array();
-    $form_state['build_info']['callback_object'] = $form_arg;
-    $form_state['build_info']['args'] = array();
-    $this->formBuilder->buildForm($form_id, $form_state);
+    $this->formBuilder->buildForm($form_arg, $form_state);
   }
 
   /**
@@ -734,7 +731,7 @@ public function testSendResponse() {
    *
    * @param string $form_id
    *   (optional) The form ID to be used. If none is provided, the form will be
-   *   set to expect that getFormId() will never be called.
+   *   set with no expectation about getFormId().
    * @param mixed $expected_form
    *   (optional) If provided, the expected form response for buildForm() to
    *   return. Defaults to NULL.
@@ -745,17 +742,13 @@ public function testSendResponse() {
    * @return \PHPUnit_Framework_MockObject_MockObject|\Drupal\Core\Form\FormInterface
    *   The mocked form object.
    */
-  protected function getMockForm($form_id = NULL, $expected_form = NULL, $count = 1) {
+  protected function getMockForm($form_id, $expected_form = NULL, $count = 1) {
     $form = $this->getMock('Drupal\Core\Form\FormInterface');
     if ($form_id) {
       $form->expects($this->once())
         ->method('getFormId')
         ->will($this->returnValue($form_id));
     }
-    else {
-      $form->expects($this->never())
-        ->method('getFormId');
-    }
 
     if ($expected_form) {
       $form->expects($this->exactly($count))
tim.plunkett’s picture

FileSize
27.05 KB

Finally remembered this issue, whoops.

dawehner’s picture

Assigned: Unassigned » damiankloip

Let's ask damian for a review.

tim.plunkett’s picture

10: form-2147669-10.patch queued for re-testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I am fine with that now.

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.

cilefen’s picture