diff --git a/core/lib/Drupal/Core/Render/Element/MachineName.php b/core/lib/Drupal/Core/Render/Element/MachineName.php index ace452b072..22bdd45142 100644 --- a/core/lib/Drupal/Core/Render/Element/MachineName.php +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php @@ -150,6 +150,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 +257,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 + // elements 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 1ec222746e..4de07be215 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, t('Save configuration')); + $this->assertResponse(200); + $this->assertText(t('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 49944800ef..d696556016 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 61fe2a9c55..207b542f95 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 9e178cd51a..f2de126474 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 0000000000..ff2dcaac58 --- /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/modules/system/tests/src/FunctionalJavascript/MachineNameValidationTest.php b/core/modules/system/tests/src/FunctionalJavascript/MachineNameValidationTest.php new file mode 100644 index 0000000000..e09ca6cf4c --- /dev/null +++ b/core/modules/system/tests/src/FunctionalJavascript/MachineNameValidationTest.php @@ -0,0 +1,57 @@ +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 06f11d3b94..036a5240ed 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 this to be set. During regular form + // building it always be because it is set in + // \Drupal\Core\Render\Element\MachineName::getInfo(). + '#default_value' => NULL, ]; $complete_form = [