Problem/Motivation

#2880011: Add a #disable_inline_form_errors_summary property to disable the Inline Form Errors summary needs refactoring of test FormErrorHandlerTest.

Add one form in the Setup with fields for various test cases (e.g. missing or invisible fields) and use it in every tests.

protected function setUp() {
  parent::setUp();

  $form = [
    '#parents' => [],
    '#form_id' => 'test_form',
    '#array_parents' => [],
  ];
  $form['test1'] = [
    '#type' => 'textfield',
    '#title' => 'Test 1',
    '#parents' => ['test1'],
    '#array_parents' => ['test1'],
    '#id' => 'edit-test1',
  ];


  (...)


  $form['test6'] = [
    '#type' => 'value',
    '#title' => 'Test 6',
    '#parents' => ['test6'],
    '#array_parents' => ['test6'],
    '#id' => 'edit-test6',
  ];

  $this->form = $form;

Proposed resolution

Use the existing form from the testDisplayErrorMessagesInline().

Remove testSetElementErrorsFromFormState(). It does not seem to be necessary. It's covered in testDisplayErrorMessagesInline()!

CommentFileSizeAuthor
#5 core_3027318-5.patch9.33 KBKrzysztof Domański
#5 interdiff-2-5.txt4.79 KBKrzysztof Domański
#2 core_3027318-2.patch7.14 KBKrzysztof Domański
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Krzysztof Domański created an issue. See original summary.

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Assigned: Krzysztof Domański » Unassigned
Issue summary: View changes
Status: Active » Needs review
FileSize
core_3027318-2.patch7.14 KB
Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Parent issue: » #2880011: Add a #disable_inline_form_errors_summary property to disable the Inline Form Errors summary
wengerk’s picture

wengerk
he/him
Primary language French
Location Lausanne
CreditAttribution: wengerk at Antistatique commented
Status: Needs review » Needs work

Thanks to @Krzysztof Domański for addressing this blocker issue from #2880011: Add a #disable_inline_form_errors_summary property to disable the Inline Form Errors summary !

I still think there is room for improvement, but these are minor points.

  1. I agree with your statement.

    Remove testSetElementErrorsFromFormState(). It does not seem to be necessary. It's covered in testDisplayErrorMessagesInline()!

  2. The $form property could be more "explicite" as it's only used for tests purpose. 
        /**
      * The form.
      *
      * @var array
      */
      protected $form;

    I would suggest sommething such:

          /**
      * Form for testing.
      *
      * @var array
      */
      protected $testForm;
  3. Use inheritdoc for $modules property. 
          /**
      * {@inheritdoc}
      */
      public static $modules = ['system', 'user', 'filter', 'filter_test', 'editor'];
  4. Why not directly assign the form to $this->form instead of doing $this->form = $form; ?
  5. The assert comment on testErrorMessagesNotInline seems wrong.

    This method disable the inline_form_error but we still have:

          // Assert the inline error still exists.
      $this->assertSame('invalid', $this->form['test1']['#errors']);

    This don't assert the inline message when #disable_inline_form_errors is used. It only assert the error belongs to the correct input.

    I think we can keep this $this->assertSame('invalid', $this->form['test1']['#errors']); on both Test, but we have to change the comment to reflect it only assert #error is populated for proper input.

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Status: Needs work » Needs review
FileSize
interdiff-2-5.txt4.79 KB
core_3027318-5.patch9.33 KB

I add the changes suggested by #4.

I'm not sure about 4.3. Is $modules property required here? Does not exist in FormErrorHandlerTest.

wengerk’s picture

wengerk
he/him
Primary language French
Location Lausanne
CreditAttribution: wengerk at Antistatique commented

I'm not sure about 4.3. Is $modules property required here? Does not exist in FormErrorHandlerTest.

Right, I think I miss review by seeing it in another file cause there is not $modules in your code haha sorry.

Seems good to me, let's see what people think about it !

wengerk’s picture

wengerk
he/him
Primary language French
Location Lausanne
CreditAttribution: wengerk at Antistatique commented
Issue tags: +drupalmountaincamp

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mpp’s picture

mpp CreditAttribution: mpp at District09 for District09 commented
Status: Needs review » Reviewed & tested by the community

Looks good.

Let's move this forward so we can fix #2880011 as well.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 1debb36c1a to 8.8.x and 7460ad9745 to 8.7.x. Thanks!

As a test-only change backported to 8.7.x to keep the branches aligned.

  • alexpott committed 1debb36 on 8.8.x 
    Issue #3027318 by Krzysztof Domański, wengerk: Improve test coverage for...

  • alexpott committed 7460ad9 on 8.7.x 
    Issue #3027318 by Krzysztof Domański, wengerk: Improve test coverage for...

Status: Fixed » Closed (fixed)

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