diff --git a/core/includes/form.inc b/core/includes/form.inc index 694fb41..a3c6572 100644 --- a/core/includes/form.inc +++ b/core/includes/form.inc @@ -12,6 +12,7 @@ use Drupal\Component\Utility\Xss; use Drupal\Core\Database\Database; use Drupal\Core\Form\FormStateInterface; +use Drupal\Core\Form\FormElementHelper; use Drupal\Core\Form\OptGroup; use Drupal\Core\Render\Element; use Drupal\Core\Template\Attribute; @@ -216,6 +217,12 @@ function template_preprocess_fieldset(&$variables) { // Add the description's id to the fieldset aria attributes. $variables['attributes']['aria-describedby'] = $description_id; } + + // Display any error messages. + $variables['errors'] = NULL; + if (!empty($element['#errors']) && empty($element['#error_use_parent'])) { + $variables['errors'] = $element['#errors']; + } } /** @@ -344,6 +351,30 @@ function template_preprocess_form(&$variables) { } $variables['attributes'] = $element['#attributes']; $variables['children'] = $element['#children']; + + if (!empty($element['#errors'])) { + $error_links = []; + // Loop through all form errors, and display a link for each error that + // is associated with a visible form element. + foreach ($element['#errors'] as $key => $error) { + if (($form_element = FormElementHelper::getElementByName($key, $element)) && Element::isVisibleElement($form_element)) { + $title = FormElementHelper::getElementTitle($form_element); + $error_links[] = \Drupal::l($title, Url::fromRoute('', [], ['fragment' => 'edit-' . str_replace('_', '-', $key), 'external' => TRUE])); + } + else { + drupal_set_message($error, 'error'); + } + } + + if (!empty($error_links)) { + // We need to pass this through String::format() so drupal_set_message() + // doesn't encode the links. + $message = \Drupal::translation()->formatPlural(count($error_links), '1 error has been found: !errors', '@count errors have been found: !errors', [ + '!errors' => implode(', ', $error_links), + ]); + drupal_set_message($message, 'error'); + } + } } /** @@ -442,6 +473,12 @@ function template_preprocess_form_element(&$variables) { // Pass elements disabled status to template. $variables['disabled'] = !empty($element['#attributes']['disabled']) ? $element['#attributes']['disabled'] : NULL; + // Display any error messages. + $variables['errors'] = NULL; + if (!empty($element['#errors']) && empty($element['#error_use_parent'])) { + $variables['errors'] = $element['#errors']; + } + // If #title is not set, we don't display any label. if (!isset($element['#title'])) { $element['#title_display'] = 'none'; diff --git a/core/lib/Drupal/Core/Form/FormElementHelper.php b/core/lib/Drupal/Core/Form/FormElementHelper.php new file mode 100644 index 0000000..9d3e193 --- /dev/null +++ b/core/lib/Drupal/Core/Form/FormElementHelper.php @@ -0,0 +1,68 @@ +errors = $errors; static::setAnyErrors(); - if ($message) { - $this->drupalSetMessage($message, 'error'); - } } } @@ -1205,15 +1202,6 @@ public function cleanValues() { } /** - * Wraps drupal_set_message(). - * - * @return array|null - */ - protected function drupalSetMessage($message = NULL, $type = 'status', $repeat = FALSE) { - return drupal_set_message($message, $type, $repeat); - } - - /** * Wraps ModuleHandler::loadInclude(). */ protected function moduleLoadInclude($module, $type, $name = NULL) { diff --git a/core/lib/Drupal/Core/Form/FormValidator.php b/core/lib/Drupal/Core/Form/FormValidator.php index ee2da65..0f2c23f 100644 --- a/core/lib/Drupal/Core/Form/FormValidator.php +++ b/core/lib/Drupal/Core/Form/FormValidator.php @@ -186,6 +186,8 @@ protected function handleErrorsWithLimitedValidation(&$form, FormStateInterface protected function finalizeValidation(&$form, FormStateInterface &$form_state, $form_id) { // After validation, loop through and assign each element its errors. $this->setElementErrorsFromFormState($form, $form_state); + // Store all of the errors for this form at the top level. + $form['#errors'] = $form_state->getErrors(); // Mark this form as validated. $form_state->setValidationComplete(); } diff --git a/core/lib/Drupal/Core/Render/Element.php b/core/lib/Drupal/Core/Render/Element.php index eed5c30..d25fb14 100644 --- a/core/lib/Drupal/Core/Render/Element.php +++ b/core/lib/Drupal/Core/Render/Element.php @@ -131,7 +131,7 @@ public static function getVisibleChildren(array $elements) { } // Skip value and hidden elements, since they are not rendered. - if (isset($child['#type']) && in_array($child['#type'], array('value', 'hidden'))) { + if (!static::isVisibleElement($child)) { continue; } @@ -142,6 +142,19 @@ public static function getVisibleChildren(array $elements) { } /** + * Determines if an element is visible. + * + * @param array $element + * The element to check for visibility. + * + * @return bool + * TRUE if the element is visible, otherwise FALSE. + */ + public static function isVisibleElement($element) { + return !isset($element['#type']) || !in_array($element['#type'], ['value', 'hidden', 'token']); + } + + /** * Sets HTML attributes based on element properties. * * @param array $element diff --git a/core/lib/Drupal/Core/Render/Element/Checkboxes.php b/core/lib/Drupal/Core/Render/Element/Checkboxes.php index 1891ceb..f3a2872 100644 --- a/core/lib/Drupal/Core/Render/Element/Checkboxes.php +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php @@ -74,6 +74,8 @@ public static function processCheckboxes(&$element, FormStateInterface $form_sta '#default_value' => isset($value[$key]) ? $key : NULL, '#attributes' => $element['#attributes'], '#ajax' => isset($element['#ajax']) ? $element['#ajax'] : NULL, + // Errors should only be shown on the parent checkboxes element. + '#error_use_parent' => TRUE, '#weight' => $weight, ); } diff --git a/core/lib/Drupal/Core/Render/Element/PasswordConfirm.php b/core/lib/Drupal/Core/Render/Element/PasswordConfirm.php index 9bc32b7..eee0cec 100644 --- a/core/lib/Drupal/Core/Render/Element/PasswordConfirm.php +++ b/core/lib/Drupal/Core/Render/Element/PasswordConfirm.php @@ -53,6 +53,7 @@ public static function processPasswordConfirm(&$element, FormStateInterface $for '#value' => empty($element['#value']) ? NULL : $element['#value']['pass1'], '#required' => $element['#required'], '#attributes' => array('class' => array('password-field')), + '#error_use_parent' => TRUE, ); $element['pass2'] = array( '#type' => 'password', @@ -60,6 +61,7 @@ public static function processPasswordConfirm(&$element, FormStateInterface $for '#value' => empty($element['#value']) ? NULL : $element['#value']['pass2'], '#required' => $element['#required'], '#attributes' => array('class' => array('password-confirm')), + '#error_use_parent' => TRUE, ); $element['#element_validate'] = array(array(get_called_class(), 'validatePasswordConfirm')); $element['#tree'] = TRUE; diff --git a/core/lib/Drupal/Core/Render/Element/Radios.php b/core/lib/Drupal/Core/Render/Element/Radios.php index b010628..ba5b5ce 100644 --- a/core/lib/Drupal/Core/Render/Element/Radios.php +++ b/core/lib/Drupal/Core/Render/Element/Radios.php @@ -67,6 +67,8 @@ public static function processRadios(&$element, FormStateInterface $form_state, '#parents' => $element['#parents'], '#id' => drupal_html_id('edit-' . implode('-', $parents_for_id)), '#ajax' => isset($element['#ajax']) ? $element['#ajax'] : NULL, + // Errors should only be shown on the parent radios element. + '#error_use_parent' => TRUE, '#weight' => $weight, ); } diff --git a/core/modules/shortcut/src/Tests/ShortcutSetsTest.php b/core/modules/shortcut/src/Tests/ShortcutSetsTest.php index c321bc4..806a486 100644 --- a/core/modules/shortcut/src/Tests/ShortcutSetsTest.php +++ b/core/modules/shortcut/src/Tests/ShortcutSetsTest.php @@ -131,7 +131,7 @@ function testShortcutSetSwitchCreate() { function testShortcutSetSwitchNoSetName() { $edit = array('set' => 'new'); $this->drupalPostForm('user/' . $this->adminUser->id() . '/shortcuts', $edit, t('Change set')); - $this->assertText(t('The new set label is required.')); + $this->assertRaw(t('1 error has been found: New set')); $current_set = shortcut_current_displayed_set($this->adminUser); $this->assertEqual($current_set->id(), $this->set->id(), 'Attempting to switch to a new shortcut set without providing a set name does not succeed.'); } diff --git a/core/modules/system/css/system.theme.css b/core/modules/system/css/system.theme.css index e520523..d5d9a68 100644 --- a/core/modules/system/css/system.theme.css +++ b/core/modules/system/css/system.theme.css @@ -113,6 +113,23 @@ abbr.ajax-changed { margin-right: 0; } +.form-error { + background-color: #fef5f1; + border: 1px solid #ed541d; + color: #8c2e0b; + padding: 5px; +} +.form-error-message { + margin-bottom: 10px; + min-height: 25px; +} +.form-error legend { + position: absolute; +} +.form-error .fieldset-wrapper { + margin-top: 35px; +} + /** * Inline items. */ @@ -551,6 +568,7 @@ ul.tabs { background-image: url(../../../misc/icons/ea2800/error.svg); border-color: #f9c9bf #f9c9bf #f9c9bf transparent; /* LTR */ box-shadow: -8px 0 0 #e62600; /* LTR */ + margin-left: 8px; } [dir="rtl"] .messages--error { border-color: #f9c9bf transparent #f9c9bf #f9c9bf; diff --git a/core/modules/system/src/Tests/Form/FormTest.php b/core/modules/system/src/Tests/Form/FormTest.php index 22b3b84..274221f 100644 --- a/core/modules/system/src/Tests/Form/FormTest.php +++ b/core/modules/system/src/Tests/Form/FormTest.php @@ -187,7 +187,7 @@ function testRequiredCheckboxesRadio() { } // Check the page for error messages. - $errors = $this->xpath('//div[contains(@class, "error")]//li'); + $errors = $this->xpath('//div[contains(@class, "form-error-message")]//strong'); foreach ($errors as $error) { $expected_key = array_search($error[0], $expected); // If the error message is not one of the expected messages, fail. diff --git a/core/modules/system/src/Tests/Form/ValidationTest.php b/core/modules/system/src/Tests/Form/ValidationTest.php index 7b6c9e8..fb0ac7e 100644 --- a/core/modules/system/src/Tests/Form/ValidationTest.php +++ b/core/modules/system/src/Tests/Form/ValidationTest.php @@ -8,6 +8,7 @@ namespace Drupal\system\Tests\Form; use Drupal\Core\Render\Element; +use Drupal\Core\Url; use Drupal\simpletest\WebTestBase; /** @@ -206,17 +207,34 @@ function testCustomRequiredError() { $edit = array(); $this->drupalPostForm('form-test/validate-required', $edit, 'Submit'); + $messages = []; foreach (Element::children($form) as $key) { if (isset($form[$key]['#required_error'])) { $this->assertNoText(t('!name field is required.', array('!name' => $form[$key]['#title']))); - $this->assertText($form[$key]['#required_error']); + $messages[] = [ + 'title' => $form[$key]['#title'], + 'message' => $form[$key]['#required_error'], + 'key' => $key, + ]; } elseif (isset($form[$key]['#form_test_required_error'])) { $this->assertNoText(t('!name field is required.', array('!name' => $form[$key]['#title']))); - $this->assertText($form[$key]['#form_test_required_error']); + $messages[] = [ + 'title' => $form[$key]['#title'], + 'message' => $form[$key]['#form_test_required_error'], + 'key' => $key, + ]; + } + elseif (!empty($form[$key]['#required'])) { + $messages[] = [ + 'title' => $form[$key]['#title'], + 'message' => t('!name field is required.', ['!name' => $form[$key]['#title']]), + 'key' => $key, + ]; } } - $this->assertNoText(t('An illegal choice has been detected. Please contact the site administrator.')); + $this->assertErrorMessages($messages); + // Verify that no custom validation error appears with valid values. $edit = array( @@ -226,6 +244,7 @@ function testCustomRequiredError() { ); $this->drupalPostForm('form-test/validate-required', $edit, 'Submit'); + $messages = []; foreach (Element::children($form) as $key) { if (isset($form[$key]['#required_error'])) { $this->assertNoText(t('!name field is required.', array('!name' => $form[$key]['#title']))); @@ -235,7 +254,47 @@ function testCustomRequiredError() { $this->assertNoText(t('!name field is required.', array('!name' => $form[$key]['#title']))); $this->assertNoText($form[$key]['#form_test_required_error']); } + elseif (!empty($form[$key]['#required'])) { + $messages[] = [ + 'title' => $form[$key]['#title'], + 'message' => t('!name field is required.', ['!name' => $form[$key]['#title']]), + 'key' => $key, + ]; + } } + $this->assertErrorMessages($messages); + } + + /** + * Asserts that the given error messages are displayed. + * + * @param array $messages + * An associative array of error messages keyed by the order they appear on + * the page, with the following key-value pairs: + * - title: The human readable form element title. + * - message: The error message for this form element. + * - key: The key used for the form element. + */ + protected function assertErrorMessages($messages) { + $element = $this->xpath('//div[@class = "form-error-message"]/strong'); + $this->assertIdentical(count($messages), count($element)); + + $error_links = []; + foreach ($messages as $delta => $message) { + // Ensure the message appears in the correct place. + if (!isset($element[$delta])) { + $this->fail(format_string('The error message for the "@title" element with key "@key" was not found.', ['@title' => $message['title'], '@key' => $message['key']])); + } + else { + $this->assertIdentical($message['message'], (string) $element[$delta]); + } + + // Gather the element for checking the jump link section. + $error_links[] = \Drupal::l($message['title'], Url::fromRoute('', [], ['fragment' => 'edit-' . str_replace('_', '-', $message['key']), 'external' => TRUE])); + } + $top_message = \Drupal::translation()->formatPlural(count($error_links), '1 error has been found', '@count errors have been found') . ': ' . implode(', ', $error_links); + $this->assertRaw($top_message); $this->assertNoText(t('An illegal choice has been detected. Please contact the site administrator.')); } + } diff --git a/core/modules/system/templates/fieldset.html.twig b/core/modules/system/templates/fieldset.html.twig index ab6796c..0823204 100644 --- a/core/modules/system/templates/fieldset.html.twig +++ b/core/modules/system/templates/fieldset.html.twig @@ -5,6 +5,7 @@ * * Available variables: * - attributes: HTML attributes for the fieldset element. + * - errors: (optional) Any errors for this fieldset element, may not be set. * - required: Boolean indicating whether the fieldeset element is required. * - legend: The legend element containing the following properties: * - title: Title of the fieldset, intended for use as the text of the legend. @@ -21,7 +22,19 @@ * @ingroup themeable */ #} - +{% + set classes = [ + 'form-item', + 'form-wrapper', + errors ? 'form-error', + ] +%} + + {% if errors %} +
+ {{ errors }} +
+ {% endif %} {% set legend_span_classes = [ 'fieldset-legend', diff --git a/core/modules/system/templates/form-element.html.twig b/core/modules/system/templates/form-element.html.twig index a961801..f9148c8 100644 --- a/core/modules/system/templates/form-element.html.twig +++ b/core/modules/system/templates/form-element.html.twig @@ -5,6 +5,7 @@ * * Available variables: * - attributes: HTML attributes for the containing element. + * - errors: (optional) Any errors for this form element, may not be set. * - prefix: (optional) The form element prefix, may not be set. * - suffix: (optional) The form element suffix, may not be set. * - required: The required marker, or empty if the associated form element is @@ -52,6 +53,7 @@ 'form-item-' ~ name|clean_class, title_display not in ['after', 'before'] ? 'form-no-label', disabled == 'disabled' ? 'form-disabled', + errors ? 'form-error', ] %} {% @@ -61,6 +63,11 @@ ] %} + {% if errors %} +
+ {{ errors }} +
+ {% endif %} {% if label_display in ['before', 'invisible'] %} {{ label }} {% endif %} diff --git a/core/tests/Drupal/Tests/Core/Form/FormElementHelperTest.php b/core/tests/Drupal/Tests/Core/Form/FormElementHelperTest.php new file mode 100644 index 0000000..d4c0fb4 --- /dev/null +++ b/core/tests/Drupal/Tests/Core/Form/FormElementHelperTest.php @@ -0,0 +1,70 @@ +assertSame($expected, FormElementHelper::getElementByName($name, $form)); + } + + /** + * Provides test data. + */ + public function getElementByNameProvider() { + return [ + ['id', [], []], + ['id', ['id' => ['#title' => 'ID']], ['#title' => 'ID']], + ['id', ['fieldset' => ['id' => ['#title' => 'ID']]], ['#title' => 'ID']], + ['fieldset', ['fieldset' => ['id' => ['#title' => 'ID']]], ['id' => ['#title' => 'ID']]], + ]; + } + + /** + * Tests the getElementTitle() method. + * + * @covers ::getElementTitle() + * + * @dataProvider getElementTitleProvider + */ + public function testGetElementTitle($name, $form, $expected) { + $element = FormElementHelper::getElementByName($name, $form); + $this->assertSame($expected, FormElementHelper::getElementTitle($element)); + } + + /** + * Provides test data. + */ + public function getElementTitleProvider() { + return [ + ['id', [], ''], + ['id', ['id' => ['#title' => 'ID']], 'ID'], + ['id', ['fieldset' => ['id' => ['#title' => 'ID']]], 'ID'], + ['fieldset', ['fieldset' => ['id' => ['#title' => 'ID']]], 'ID'], + ]; + } + +} diff --git a/core/tests/Drupal/Tests/Core/Form/FormStateTest.php b/core/tests/Drupal/Tests/Core/Form/FormStateTest.php index 2b2a193..3c49a78 100644 --- a/core/tests/Drupal/Tests/Core/Form/FormStateTest.php +++ b/core/tests/Drupal/Tests/Core/Form/FormStateTest.php @@ -62,15 +62,10 @@ public function providerTestGetRedirect() { * @covers ::setError */ public function testSetError() { - $form_state = $this->getMockBuilder('Drupal\Core\Form\FormState') - ->setMethods(array('drupalSetMessage')) - ->getMock(); - $form_state->expects($this->once()) - ->method('drupalSetMessage') - ->willReturn('Fail'); - + $form_state = new FormState(); $element['#parents'] = array('foo', 'bar'); $form_state->setError($element, 'Fail'); + $this->assertSame(['foo][bar' => 'Fail'], $form_state->getErrors()); } /** @@ -108,14 +103,10 @@ public function providerTestGetError() { * * @dataProvider providerTestSetErrorByName */ - public function testSetErrorByName($limit_validation_errors, $expected_errors, $set_message = FALSE) { - $form_state = $this->getMockBuilder('Drupal\Core\Form\FormState') - ->setMethods(array('drupalSetMessage')) - ->getMock(); + public function testSetErrorByName($limit_validation_errors, $expected_errors) { + $form_state = new FormState(); $form_state->setLimitValidationErrors($limit_validation_errors); $form_state->clearErrors(); - $form_state->expects($set_message ? $this->once() : $this->never()) - ->method('drupalSetMessage'); $form_state->setErrorByName('test', 'Fail 1'); $form_state->setErrorByName('test', 'Fail 2'); @@ -131,7 +122,7 @@ public function providerTestSetErrorByName() { array(array(array('options')), array('options' => '')), // Do not limit an validation, and, ensuring the first error is returned // for the 'test' element. - array(NULL, array('test' => 'Fail 1', 'options' => ''), TRUE), + array(NULL, array('test' => 'Fail 1', 'options' => '')), // Limit all validation. array(array(), array()), ); @@ -146,9 +137,7 @@ public function providerTestSetErrorByName() { * @expectedExceptionMessage Form errors cannot be set after form validation has finished. */ public function testFormErrorsDuringSubmission() { - $form_state = $this->getMockBuilder('Drupal\Core\Form\FormState') - ->setMethods(array('drupalSetMessage')) - ->getMock(); + $form_state = new FormState(); $form_state->setValidationComplete(); $form_state->setErrorByName('test', 'message'); }