Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
3.87 KB
dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
@@ -476,6 +480,32 @@ public function testGetCache() {
+    $expected_form = $this->getMockBuilder('Symfony\Component\HttpFoundation\Response')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $expected_form->expects($this->once())
+      ->method('prepare')
+      ->will($this->returnValue($expected_form));
+
+    $form_arg = $this->getMock('Drupal\Core\Form\FormInterface');
+    $form_arg->expects($this->any())
+      ->method('buildForm')
+      ->will($this->returnValue($expected_form));

I try to understand the usecase of a form being able to return a response object.

tim.plunkett’s picture

In \Drupal\system\Form\ModulesListConfirmForm:

  public function buildForm(array $form, array &$form_state) {
    $account = $this->currentUser()->id();
    $this->modules = $this->keyValueExpirable->get($account);

    // Redirect to the modules list page if the key value store is empty.
    if (!$this->modules) {
      return new RedirectResponse($this->urlGenerator()->generate('system.modules_list', array(), TRUE));
    }

It can't wait for $form_state['redirect'] or anything.

Either way, this just adds test coverage

tim.plunkett’s picture

Talked with Paris, this was originally added in #1668866: Replace drupal_goto() with RedirectResponse.

We cannot assume that a form is being built as a controller response directly, and this replaced arbitrary drupal_goto() calls within forms.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

We should try to investigate whether we can get rid of using the http kernel but this is work for another issue.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

For those following along at home, removing the exit; from sendResponse() and adding it manually where it's called allows that function to be unit tested, which is then added by this patch.

There's a slight DX loss here in that you always want to call exit; when calling sendResponse() but OTOH it's clear that this isn't used very often (Tim said it should only ever be used twice, and hopefully we can get that down to once by the time D8 ships), so the win of unit testability wins out in this case.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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