summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorNathaniel Catchpole2018-07-04 11:48:07 (GMT)
committerNathaniel Catchpole2018-07-04 11:48:07 (GMT)
commitcb67958df88092f704405a9b0039b80ba6e52be7 (patch)
treeb8f11982a6827d158f922d95fe33b29caf738996
parent5e07c02053aa73d84a6cb67b8e7632a6f37dfe32 (diff)
Issue #2557299 by alexpott, Manuel Garcia, borisson_, phenaproxima, swentel, Berdir, larowlan, mbovan, martin107, LKS90, segi, tim.plunkett, giancarlosotelo: Any AJAX call disregards machine name verification when AJAX is used and leads to a fatal error
-rw-r--r--core/lib/Drupal/Core/Render/Element/MachineName.php21
-rw-r--r--core/modules/ckeditor/tests/src/Functional/CKEditorAdminTest.php13
-rw-r--r--core/modules/media/src/MediaTypeForm.php19
-rw-r--r--core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php39
-rw-r--r--core/modules/system/tests/modules/form_test/form_test.routing.yml8
-rw-r--r--core/modules/system/tests/modules/form_test/src/Form/FormTestMachineNameValidationForm.php141
-rw-r--r--core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php31
-rw-r--r--core/tests/Drupal/Tests/Core/Render/Element/MachineNameTest.php4
8 files changed, 236 insertions, 40 deletions
diff --git a/core/lib/Drupal/Core/Render/Element/MachineName.php b/core/lib/Drupal/Core/Render/Element/MachineName.php
index ace452b..9a193ba 100644
--- a/core/lib/Drupal/Core/Render/Element/MachineName.php
+++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
@@ -28,7 +28,8 @@ use Drupal\Core\Language\LanguageInterface;
* - 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 @@ class MachineName extends Textfield {
'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 @@ class MachineName extends Textfield {
}
}
- // 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 1ec2227..aeba60f 100644
--- a/core/modules/ckeditor/tests/src/Functional/CKEditorAdminTest.php
+++ b/core/modules/ckeditor/tests/src/Functional/CKEditorAdminTest.php
@@ -216,6 +216,19 @@ class CKEditorAdminTest extends BrowserTestBase {
$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 4994480..d696556 100644
--- a/core/modules/media/src/MediaTypeForm.php
+++ b/core/modules/media/src/MediaTypeForm.php
@@ -131,13 +131,6 @@ class MediaTypeForm extends EntityForm {
'#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.
@@ -237,18 +230,6 @@ class MediaTypeForm extends EntityForm {
}
/**
- * 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.
*
* @return array
diff --git a/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
index 61fe2a9..207b542 100644
--- a/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
+++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
@@ -24,19 +24,21 @@ class MediaTypeCreationTest extends MediaJavascriptTestBase {
$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 @@ class MediaTypeCreationTest extends MediaJavascriptTestBase {
// 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 @@ class MediaTypeCreationTest extends MediaJavascriptTestBase {
$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 @@ class MediaTypeCreationTest extends MediaJavascriptTestBase {
$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 9e178cd..f2de126 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 0000000..ff2dcaa
--- /dev/null
+++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestMachineNameValidationForm.php
@@ -0,0 +1,141 @@
+<?php
+
+namespace Drupal\form_test\Form;
+
+use Drupal\Core\Form\FormBase;
+use Drupal\Core\Form\FormStateInterface;
+
+/**
+ * Form to test whether machine name validation works with ajax requests.
+ */
+class FormTestMachineNameValidationForm extends FormBase {
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getFormId() {
+ return 'form_test_machine_name_validation_form';
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function buildForm(array $form, FormStateInterface $form_state) {
+ // Disable client-side validation so that we can test AJAX requests with
+ // invalid input.
+ $form['#attributes']['novalidate'] = 'novalidate';
+
+ $form['name'] = [
+ '#type' => '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 bf2643d..acbaadb 100644
--- a/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php
+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php
@@ -59,7 +59,6 @@ class MachineNameTest extends JavascriptTestBase {
// 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 @@ class MachineNameTest extends JavascriptTestBase {
// 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 06f11d3..51f8461 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 @@ class MachineNameTest extends UnitTestCase {
'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 = [