diff --git a/core/lib/Drupal/Core/Render/Element/MachineName.php b/core/lib/Drupal/Core/Render/Element/MachineName.php index ace452b072578c49ebb341d1c7cbe5714908ac9e..9a193ba9b9eb623d28b9dfb3f2b68d5ca58ddb8d 100644 --- a/core/lib/Drupal/Core/Render/Element/MachineName.php +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php @@ -28,7 +28,8 @@ * - The form state object. * In most cases, an existing API or menu argument loader function can be * re-used. The callback is only invoked if the submitted value differs from - * the element's #default_value. + * the element's initial #default_value. The initial #default_value is + * stored in form state so AJAX forms can be reliably validated. * - source: (optional) The #array_parents of the form element containing the * human-readable name (i.e., as contained in the $form structure) to use as * source for the machine name. Defaults to array('label'). @@ -150,6 +151,17 @@ public static function processMachineName(&$element, FormStateInterface $form_st 'field_suffix' => $element['#field_suffix'], ]; + // Store the initial value in form state. The machine name needs this to + // ensure that the exists function is not called for existing values when + // editing them. + $initial_values = $form_state->get('machine_name.initial_values') ?: []; + // Store the initial values in an array so we can differentiate between a + // NULL default value and a new machine name element. + if (!array_key_exists($element['#name'], $initial_values)) { + $initial_values[$element['#name']] = $element['#default_value']; + $form_state->set('machine_name.initial_values', $initial_values); + } + // By default, machine names are restricted to Latin alphanumeric characters. // So, default to LTR directionality. if (!isset($element['#attributes'])) { @@ -246,8 +258,11 @@ public static function validateMachineName(&$element, FormStateInterface $form_s } } - // Verify that the machine name is unique. - if ($element['#default_value'] !== $element['#value']) { + // Verify that the machine name is unique. If the value matches the initial + // default value then it does not need to be validated as the machine name + // element assumes the form is editing the existing value. + $initial_values = $form_state->get('machine_name.initial_values') ?: []; + if (!array_key_exists($element['#name'], $initial_values) || $initial_values[$element['#name']] !== $element['#value']) { $function = $element['#machine_name']['exists']; if (call_user_func($function, $element['#value'], $element, $form_state)) { $form_state->setError($element, t('The machine-readable name is already in use. It must be unique.')); diff --git a/core/modules/ckeditor/tests/src/Functional/CKEditorAdminTest.php b/core/modules/ckeditor/tests/src/Functional/CKEditorAdminTest.php index 1ec222746e3fa9d5133f66323b8663bd65f07d4a..aeba60fd7545bc0cd125153ebedca0060c5e6ef9 100644 --- a/core/modules/ckeditor/tests/src/Functional/CKEditorAdminTest.php +++ b/core/modules/ckeditor/tests/src/Functional/CKEditorAdminTest.php @@ -216,6 +216,19 @@ public function testExistingFormat() { $editor = Editor::load('filtered_html'); $this->assertTrue($editor instanceof Editor, 'An Editor config entity exists.'); $this->assertEqual($expected_settings, $editor->getSettings()); + + $this->drupalGet('admin/config/content/formats/add'); + // Now attempt to add another filter format with the same editor and same + // machine name. + $edit = [ + 'format' => 'filtered_html', + 'name' => 'Filtered HTML', + 'editor[editor]' => 'ckeditor', + ]; + $this->submitForm($edit, 'editor_configure'); + $this->submitForm($edit, 'Save configuration'); + $this->assertResponse(200); + $this->assertText('The machine-readable name is already in use. It must be unique.'); } /** diff --git a/core/modules/media/src/MediaTypeForm.php b/core/modules/media/src/MediaTypeForm.php index 49944800efe95b7e393ca227e848c6e355e20c15..d6965560160e94d88ae2f80fec8fc1d33861e65c 100644 --- a/core/modules/media/src/MediaTypeForm.php +++ b/core/modules/media/src/MediaTypeForm.php @@ -131,13 +131,6 @@ public function form(array $form, FormStateInterface $form_state) { '#options' => $options, '#description' => $source_description, '#ajax' => ['callback' => '::ajaxHandlerData'], - // Rebuilding the form as part of the AJAX request is a workaround to - // enforce machine_name validation. - // @todo This was added as part of #2932226 and it should be removed once - // https://www.drupal.org/project/drupal/issues/2557299 solves it in a - // more generic way. - '#executes_submit_callback' => TRUE, - '#submit' => [[static::class, 'rebuildSubmit']], '#required' => TRUE, // Once the media type is created, its source plugin cannot be changed // anymore. @@ -236,18 +229,6 @@ public function form(array $form, FormStateInterface $form_state) { return $form; } - /** - * Form submission handler to rebuild the form on select submit. - * - * @param array $form - * Full form array. - * @param \Drupal\Core\Form\FormStateInterface $form_state - * Current form state. - */ - public static function rebuildSubmit(array &$form, FormStateInterface $form_state) { - $form_state->setRebuild(); - } - /** * Prepares workflow options to be used in the 'checkboxes' form element. * diff --git a/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php index 61fe2a9c55e169cb838095be09485d49ddd833e5..207b542f95f6e000f72a47019ddc743dc12e6da5 100644 --- a/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php @@ -24,19 +24,21 @@ public function testMediaTypeCreationFormWithDefaultField() { $this->drupalGet('admin/structure/media/add'); - // Fill in a label to the media type. - $page->fillField('label', $label); - // Wait for machine name generation. Default: waitUntilVisible(), does not - // work properly. - $session->wait(5000, "jQuery('.machine-name-value').text() === '{$mediaTypeMachineName}'"); - - // Select the media source used by our media type. + // Select the media source used by our media type. Do this before setting + // the label or machine name in order to guard against the regression in + // https://www.drupal.org/project/drupal/issues/2557299. $assert_session->fieldExists('Media source'); $assert_session->optionExists('Media source', 'test'); $page->selectFieldOption('Media source', 'test'); $result = $assert_session->waitForElementVisible('css', 'fieldset[data-drupal-selector="edit-source-configuration"]'); $this->assertNotEmpty($result); + // Fill in a label to the media type. + $page->fillField('label', $label); + // Wait for machine name generation. Default: waitUntilVisible(), does not + // work properly. + $session->wait(5000, "jQuery('.machine-name-value').text() === '{$mediaTypeMachineName}'"); + $page->pressButton('Save'); // Check whether the source field was correctly created. @@ -97,11 +99,12 @@ public function testMediaTypeCreationReuseSourceField() { // Create a new media type, which should create a new field we can reuse. $this->drupalGet('/admin/structure/media/add'); - $page->fillField('label', 'Pastafazoul'); - $session->wait(5000, "jQuery('.machine-name-value').text() === 'pastafazoul'"); + // Choose the source plugin before setting the label and machine name. $page->selectFieldOption('Media source', 'test'); $result = $assert_session->waitForElementVisible('css', 'fieldset[data-drupal-selector="edit-source-configuration"]'); $this->assertNotEmpty($result); + $page->fillField('label', 'Pastafazoul'); + $session->wait(5000, "jQuery('.machine-name-value').text() === 'pastafazoul'"); $page->pressButton('Save'); $label = 'Type reusing Default Field'; @@ -109,14 +112,8 @@ public function testMediaTypeCreationReuseSourceField() { $this->drupalGet('admin/structure/media/add'); - // Fill in a label to the media type. - $page->fillField('label', $label); - - // Wait for machine name generation. Default: waitUntilVisible(), does not - // work properly. - $session->wait(5000, "jQuery('.machine-name-value').text() === '{$mediaTypeMachineName}'"); - - // Select the media source used by our media type. + // Select the media source used by our media type. Do this before setting + // the label and machine name. $assert_session->fieldExists('Media source'); $assert_session->optionExists('Media source', 'test'); $page->selectFieldOption('Media source', 'test'); @@ -124,6 +121,14 @@ public function testMediaTypeCreationReuseSourceField() { $this->assertNotEmpty($result); // Select the existing field for re-use. $page->selectFieldOption('source_configuration[source_field]', 'field_media_test'); + + // Fill in a label to the media type. + $page->fillField('label', $label); + + // Wait for machine name generation. Default: waitUntilVisible(), does not + // work properly. + $session->wait(5000, "jQuery('.machine-name-value').text() === '{$mediaTypeMachineName}'"); + $page->pressButton('Save'); // Check that no new fields were created. diff --git a/core/modules/system/tests/modules/form_test/form_test.routing.yml b/core/modules/system/tests/modules/form_test/form_test.routing.yml index 9e178cd51a6a57a6849e7d3bb1e31375fde4437d..f2de1264743415bfcefb0ed06aa5cd16fb2b8d04 100644 --- a/core/modules/system/tests/modules/form_test/form_test.routing.yml +++ b/core/modules/system/tests/modules/form_test/form_test.routing.yml @@ -498,6 +498,14 @@ form_test.get_form: requirements: _access: 'TRUE' +form_test.machine_name_validation: + path: '/form-test/form-test-machine-name-validation' + defaults: + _form: '\Drupal\form_test\Form\FormTestMachineNameValidationForm' + _title: 'Form machine name validation test' + requirements: + _access: 'TRUE' + form_test.optional_container: path: '/form-test/optional-container' defaults: diff --git a/core/modules/system/tests/modules/form_test/src/Form/FormTestMachineNameValidationForm.php b/core/modules/system/tests/modules/form_test/src/Form/FormTestMachineNameValidationForm.php new file mode 100644 index 0000000000000000000000000000000000000000..ff2dcaac582386cc1fbbd0682dec43d01e214689 --- /dev/null +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestMachineNameValidationForm.php @@ -0,0 +1,141 @@ + 'textfield', + '#default_value' => $form_state->getValue('name'), + '#maxlength' => 50, + '#required' => TRUE, + '#title' => 'Name', + ]; + + // The default value simulates how an entity form works, which has default + // values based on an entity, which is updated in an afterBuild callback. + // During validation and after build, limit_validation_errors is not + // in effect, which means that getValue('id') does return a value, while it + // does not during the submit callback. Therefore, this test sets the value + // in ::buildAjaxSnackConfigureFormValidate() and then uses that as the + // default value, so that the default value and the value are identical. + $form['id'] = [ + '#type' => 'machine_name', + '#default_value' => $form_state->get('id'), + '#maxlength' => 50, + '#required' => TRUE, + '#machine_name' => [ + 'exists' => [$this, 'load'], + 'source' => ['name'], + ], + ]; + + // Test support for multiple machine names on the form. Although this has + // the default value duplicate it should not generate an error because it + // is the default value. + $form['id2'] = [ + '#type' => 'machine_name', + '#default_value' => 'duplicate', + '#maxlength' => 50, + '#required' => TRUE, + '#machine_name' => [ + 'exists' => [$this, 'load'], + 'source' => ['name'], + ], + ]; + + $form['snack'] = [ + '#type' => 'select', + '#title' => $this->t('Select a snack'), + '#options' => [ + 'apple' => 'apple', + 'pear' => 'pear', + 'potato' => 'potato', + ], + '#required' => TRUE, + '#ajax' => [ + 'callback' => '::buildAjaxSnackConfigureForm', + 'wrapper' => 'snack-config-form', + 'method' => 'replace', + 'effect' => 'fade', + ], + ]; + $form['snack_configs'] = [ + '#type' => 'container', + '#attributes' => [ + 'id' => 'snack-config-form', + ], + '#tree' => TRUE, + ]; + + $form['submit'] = [ + '#type' => 'submit', + '#value' => 'Save', + ]; + return $form; + } + + /** + * Validate callback that forces a form rebuild. + */ + public function buildAjaxSnackConfigureFormValidate(array $form, FormStateInterface $form_state) { + $form_state->set('id', $form_state->getValue('id')); + } + + /** + * Submit callback that forces a form rebuild. + */ + public function buildAjaxSnackConfigureFormSubmit(array $form, FormStateInterface $form_state) { + $form_state->setRebuild(); + } + + /** + * Handles changes to the selected snack configuration. + */ + public function buildAjaxSnackConfigureForm(array $form, FormStateInterface $form_state) { + return $form['snack_configs']; + } + + /** + * Loading stub for machine name + * + * @param $machine_name + * @return bool + */ + public function load($machine_name) { + if (strpos($machine_name, 'duplicate') !== FALSE) { + return TRUE; + } + + return FALSE; + } + + /** + * {@inheritdoc} + */ + public function submitForm(array &$form, FormStateInterface $form_state) { + $this->messenger()->addStatus('The form_test_machine_name_validation_form form has been submitted successfully.'); + } + +} diff --git a/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php index bf2643da6a536a3855d90d89635326489e8840a2..acbaadb69886dd1c71573152f2f52759fd7839ac 100644 --- a/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php @@ -59,7 +59,6 @@ public function testMachineName() { // Get page and session. $page = $this->getSession()->getPage(); - $assert_session = $this->assertSession(); // Get elements from the page. $title_1 = $page->findField('machine_name_1_label'); @@ -111,6 +110,36 @@ public function testMachineName() { // Validate if the element contains the correct value. $this->assertEquals($test_values[1]['expected'], $machine_name_1_field->getValue(), 'The ID field value must be equal to the php generated machine name'); + + $assert = $this->assertSession(); + $this->drupalGet('/form-test/form-test-machine-name-validation'); + + // Test errors after with no AJAX. + $assert->buttonExists('Save')->press(); + $assert->pageTextContains('Machine-readable name field is required.'); + // Ensure only the first machine name field has an error. + $this->assertTrue($assert->fieldExists('id')->hasClass('error')); + $this->assertFalse($assert->fieldExists('id2')->hasClass('error')); + + // Test a successful submit after using AJAX. + $assert->fieldExists('Name')->setValue('test 1'); + $assert->fieldExists('id')->setValue('test_1'); + $assert->selectExists('snack')->selectOption('apple'); + $assert->assertWaitOnAjaxRequest(); + $assert->buttonExists('Save')->press(); + $assert->pageTextContains('The form_test_machine_name_validation_form form has been submitted successfully.'); + + // Test errors after using AJAX. + $assert->fieldExists('Name')->setValue('duplicate'); + $this->assertJsCondition('document.forms[0].id.value === "duplicate"'); + $assert->fieldExists('id2')->setValue('duplicate2'); + $assert->selectExists('snack')->selectOption('potato'); + $assert->assertWaitOnAjaxRequest(); + $assert->buttonExists('Save')->press(); + $assert->pageTextContains('The machine-readable name is already in use. It must be unique.'); + // Ensure both machine name fields both have errors. + $this->assertTrue($assert->fieldExists('id')->hasClass('error')); + $this->assertTrue($assert->fieldExists('id2')->hasClass('error')); } } diff --git a/core/tests/Drupal/Tests/Core/Render/Element/MachineNameTest.php b/core/tests/Drupal/Tests/Core/Render/Element/MachineNameTest.php index 06f11d3b9468fe30fcd7f1c0b3a51454d91a2726..51f84615f4fa5e563ff2da4d85889b07d1c7b76c 100644 --- a/core/tests/Drupal/Tests/Core/Render/Element/MachineNameTest.php +++ b/core/tests/Drupal/Tests/Core/Render/Element/MachineNameTest.php @@ -60,6 +60,10 @@ public function testProcessMachineName() { 'additional_property' => TRUE, '#additional_property_with_hash' => TRUE, ], + // The process function requires these to be set. During regular form + // building they are always set. + '#name' => 'test_machine_name', + '#default_value' => NULL, ]; $complete_form = [