Any AJAX call disregards machine name verification when AJAX is used and leads to a fatal error

I could reproduce the problem. I think I found a bug in the validation function, when the function try to check that there is similar format name or not.
The machine name was checked with not equal. I think we have to check the test as well.
This bug independents from ajax or it is a new bug?

Failing test

So the issue is the limit validation errors added by editor module means the machine name validation is discarded, but the default value is set, which means the next time around the machine name doesn't see the value as changed and doesn't run validation.

The fix is to also include the format in the validation.

I fixed the test fails which suddenly appeared with the patch in EditorAdminTest.
The problem were missing required fields in the $edit array when using the selectUnicornEditor() function, I fixed it by adding a parameter to this helper function, the $edit array passed by reference. So an existing $edit array is modified by the function and used in it's drupalPostForm() call.

I think we have to fix it somewhere deeper maybe in MachineName class.

Yep, this is a general problem - I'd say this almost is critical worthy.

Moving to forms component - I agree with #13 and #14 to look into forms system / MachineName class somewhere as this affects a lot of contrib and custom modules.

Changing title as well

Wow, machine names and ajax requests are really fun. The validation is actually happening when the ajax request is triggered, but the error is not shown. Then usually during an ajax request, a form rebuild is triggered and '#value' in form_state is now populated and then the actual submission doesn't validate the name anymore since default_value and value are equal.

Another example how broken this behavior is: edit some existing entity, edit the machine name, trigger an ajax request, then change the machine name to the original, then hit submit. You'll get the "The machine-readable name is already in use. It must be unique." message. Granted, this is a situation that won't happen that much, but it's possible.

Been looking into a patch, but have been running against form api stuff. Will look further tomorrow.

Thanks for tagging this for rc target triage! To have committers consider it for inclusion in RC, we should add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. We can add a section <h3>Why this should be an RC target</h3> to the summary. Sounds like it has a significant impact on contrib?

Update the issue summary.

Patch attached with proof of concept - it at least now works for facetapi & search api - hopefully core is fine and no other things break ..
Attaching fail only from larowlan + fix.

+++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
@@ -10,6 +10,7 @@
+use Drupal\Core\Logger\RfcLogLevel;

will fix when the test is back

Sigh

Haven't looked at it manually yet, just the patch itself.

1. +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
   @@ -232,12 +232,38 @@ public static function validateMachineName(&$element, FormStateInterface $form_s
   +    $needs_revalidation = $form_state->get('needs_revalidation', $element['#name']);
   ...
   +        $form_state->set('needs_revalidation', $element['#name']);
   

   Can we use get/setTemporaryValue here?

2. +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
   @@ -232,12 +232,38 @@ public static function validateMachineName(&$element, FormStateInterface $form_s
   +      self::isUnique($element, $form_state);
   ...
   +      self::isUnique($element, $form_state);
   

   Might as well use static::

I can't get it to work in the temporary values.

I changed this to static:: over self:: though.

This can't work with the temporary value store, because that is not accessible between requests.
Because this is currently being set in an ajax request, this will not work.

So I think this is ready.

I think @tim.plunkett needs to the final review, or another Form API maintainer.

1. +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
   @@ -232,12 +232,38 @@ public static function validateMachineName(&$element, FormStateInterface $form_s
   +    }
   +
   +  }
   

   Nit: extraneous newline.

2. +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
   @@ -232,12 +232,38 @@ public static function validateMachineName(&$element, FormStateInterface $form_s
   +   * Verify that the machine name is unique.
   

   Nit: s/Verify/Verifies/

3. +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
   @@ -232,12 +232,38 @@ public static function validateMachineName(&$element, FormStateInterface $form_s
   +   * @param $element
   

   Missing typehint: array.

4. +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
   @@ -232,12 +232,38 @@ public static function validateMachineName(&$element, FormStateInterface $form_s
   +   * @param FormStateInterface $form_state
   

   Needs FQCN.

5. +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
   @@ -232,12 +232,38 @@ public static function validateMachineName(&$element, FormStateInterface $form_s
   +   *   The form state interface.
   

   Eh, not an interface methinks :)

6. +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
   @@ -232,12 +232,38 @@ public static function validateMachineName(&$element, FormStateInterface $form_s
   +   * @param (optional) $set_needs_revalidation
   

   Missing typehint: bool.

7. +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
   --- a/core/modules/ckeditor/src/Tests/CKEditorAdminTest.php
   +++ b/core/modules/ckeditor/src/Tests/CKEditorAdminTest.php
   

   If this is actually a Form API bug, then I think this should also have explicit test coverage in Form API tests. Otherwise it could easily regress in the future.

This fixes #33 1 - 6

We should still add a test form api test as well.

Added a new test. test-only also serves as an interdiff.

1. +++ b/core/modules/system/src/Tests/Form/FormTest.php
   @@ -750,4 +750,37 @@ function testRequiredAttribute() {
   +   * Test machine name still required after an ajax submit.
   

   s/name still/name is still

2. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestMachineNameValidationForm.php
   @@ -0,0 +1,130 @@
   +   * Handles changes to the selected facet sources.
   

   facet :)

Fixes remarks made in #36.

The test-only patch in #35 didn't fail, so the test isn't good enough. Back to NW.

I looked at this again and I can't figure out how to write a failing test-only patch.

Rerolled and small changes.

Tried to debug test-only case and it seems that $form_state->getValue('id') doesn't have a value when the form is rebuilt on an ajax request.
Core usually uses #machine_name with an entity, so maybe we can think about extending the form test and replacing the form state value with a test entity ID, since the state can be preserved in that case.

Tested with inmail and collect modules, works with both. +1 from me.

Ok, this is a good one. @mbovan was on the right track.

The ajax callback uses limit validation errors, but that is interestingly not active during validation and in #after_build callbacks, which is where config entity forms update $this->entity. So I changed the test a bit to simulate behavior, with a validate callback that sets the value and then submit that rebuilds the form. I'm actually wondering if that isn't a flaw/bug in the form system but I have no idea how to prevent that. And after build actually even happens *before* validation, so we're technically building and keeping the entity which is based on unvalidated input. Weird but I think not actually a real issue since we do then run validation and don't do anything if there are validation errors

That makes it fail/pass as expected. It's not exactly like an entity form but I think that's OK since we also have test that are actually using entity forms and this is more understandable/reproducible than those. Real use case tests and a simplified, documented test that doesn't require an unhealthy amount of knowledge about the form *and* entity systems to know what's going on ;)

Since #45 addresses #33.7 (Test fail in \Drupal\system\Tests\Form\FormTest), this look good to me.

1. +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
   @@ -240,12 +240,37 @@ public static function validateMachineName(&$element, FormStateInterface $form_s
   +    if ($needs_revalidation) {
   +      static::isUnique($element, $form_state);
   +    }
   +    elseif ($element['#default_value'] !== $element['#value']) {
   +      static::isUnique($element, $form_state);
   +    }
   ...
   +  public static function isUnique($element, FormStateInterface $form_state, $set_needs_revalidation = TRUE) {
   

   Right now this is the same as

   if ($needs_revalidation || $element['#default_value'] !== $element['#value']) {

   Where are the calls to isUnique where $set_needs_revalidation would be FALSE?

2. +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
   @@ -240,12 +240,37 @@ public static function validateMachineName(&$element, FormStateInterface $form_s
   +   *   The form element
   

   Missing .

3. +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
   @@ -240,12 +240,37 @@ public static function validateMachineName(&$element, FormStateInterface $form_s
   +   * @param (optional) bool $set_needs_revalidation
   +   *   Whether to set a property on form state in case of an error to revalidate
   

   The (optional) part goes in the description

#45 was not applying any more, rerolled

I've just tested this patch to see if it solves the issue with the ckeditor configuration page described in #2701393: Switching between editors on the format configuration causes errors upon save, but the bug persists.

FYI this can be worked around for now by forcing the #default_value to be NULL for any new entities:

    $form['id'] = [
      '#type' => 'machine_name',
      '#default_value' => $entity->isNew() ? NULL : $entity->id(), // <--- THIS
      '#maxlength' => EntityTypeInterface::BUNDLE_MAX_LENGTH,
      '#machine_name' => [
        'exists' => ['\Drupal\my_entity\Entity\MyEntity', 'load'],
      ],
      '#disabled' => !$entity->isNew(),
    ];

Rerolling again. Also switched to short array syntax the code we're adding to CKEditorAdminTest.php.

Oops!

Attempting to fix the failing tests...

I think #48 still needs to be addressed.

#2934154: [PP-1] Remove workaround to enforce machine name validation on MediaType forms is a followup for media once the general fix lands.

The patch from #60 fixes #2934154: [PP-1] Remove workaround to enforce machine name validation on MediaType forms when the workaround from #2932226: Media Type entities don't validate machine name properly is removed.

I added a test in #2934154: [PP-1] Remove workaround to enforce machine name validation on MediaType forms which fails the current workaround from #2932226: Media Type entities don't validate machine name properly. In the second patch I added the patch from #60 and removed the old workaround, to show a passing test.
And in the third patch I only included the test and removed the old workaround, so when this issue is committed, #2934154: [PP-1] Remove workaround to enforce machine name validation on MediaType forms can be committed too.

Addressed the feedback in #48 and re-rolled for 8.6.x (hence, no interdiff).

Let's get this in, so we can finally land #2934154: [PP-1] Remove workaround to enforce machine name validation on MediaType forms too.

+++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
@@ -213,7 +213,8 @@ public static function processMachineName(&$element, FormStateInterface $form_st
    *

@@ -246,12 +247,34 @@ public static function validateMachineName(&$element, FormStateInterface $form_s
+  public static function isUnique($element, FormStateInterface $form_state, $set_needs_revalidation = TRUE) {

Why add more public API? I don't think we should add this method because it's not necessary. It should just be part of the validateMachineName() as before.

Fixed! Plus a little more minor cleanup. Let's see if this flies.

I have some questions myself, directed at those who understand this patch more than I do:

• We're testing AJAX, but none of the tests are functional JS tests. Wouldn't it be better (or at least a good idea) to have one?
• Should we be unsetting needs_revalidation if the machine name is, in fact, unique?

+++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
@@ -246,10 +247,14 @@ public static function validateMachineName(&$element, FormStateInterface $form_s
+    if ($form_state->get('needs_revalidation') === $element['#name'] || $element['#default_value'] !== $element['#value']) {
...
+        $form_state->set('needs_revalidation', $element['#name']);

What if there are multiple machine name elements per form? Wouldn't needs_revalidation be overwritten incorrectly?

I think the answer to both questions in #69 is "yes"

This problem manifests very visibly in the Media module (specifically in MediaTypeForm), so I have adapted Media's existing functional JS tests to prove the problem, and its fix. I've also reverted the temporary workaround Media had implemented to hack past this problem in #2932226: Media Type entities don't validate machine name properly.

Given that, I'm not sure we need additional dedicated JS tests.

Tagging as part of the Media Initiative.

Switching to 8.6.x, since both patches in #71 were rolled against that.

This should fix the failing test (drupal_set_message was deprecated https://www.drupal.org/node/2774931).

Huh?
Not sure why it fails, FormTestMachineNameValidationForm extends Drupal\Core\Form\FormBase which uses the MessengerTrait.. so it should work no?

You need to use $this->messenger() — the method — not $this->messenger, the property, since it is not explicitly set until $this->messenger() is called.

crap, of course - thanks @phenaproxima!

This issue is critical for Media and I would argue should be targeted for 8.6.x as now the oEmbed patch has landed there's going to be more media type creation.

1. +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
   @@ -213,7 +213,8 @@ public static function processMachineName(&$element, FormStateInterface $form_st
   +   * Note that #maxlength is validated by the form validator already, in
   +   * \Drupal\Core\Form\FormValidatorInterface::validateForm().
   

   This change is odd. The interface does not do the validation. We should file a follow up to fix this. It is unrelated to the problem.

2. +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
   @@ -246,10 +247,13 @@ public static function validateMachineName(&$element, FormStateInterface $form_s
   +    // Verify that the machine name is unique. Check form state if we need to
   +    // explicitly validate the element. This happens every time when an error
   +    // has been set, since this breaks on AJAX requests
   +    if ($form_state->get('needs_revalidation') === $element['#name'] || $element['#default_value'] !== $element['#value']) {
   

   I wonder what happens if a form has multiple machine name fields. It looks like this won't work.

3. +++ b/core/modules/system/tests/src/Functional/Form/FormTest.php
   @@ -770,4 +770,36 @@ public function testRequiredAttribute() {
   +  /**
   +   * Tests that a machine name is still required after an AJAX submit.
   +   *
   +   * This protects against the regression in https://www.drupal.org/node/2557299.
   +   */
   +  public function testMachineNameRequiredFormAjaxSubmit() {
   

   I think this test needs to be Javascript test to properly test AJAX.

1. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
   @@ -97,11 +99,14 @@ public function testMediaTypeCreationReuseSourceField() {
   +    // Choose the source plugin _before_ setting the label and machine name in
   

   Nice markdown :)

2. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
   @@ -97,11 +99,14 @@ public function testMediaTypeCreationReuseSourceField() {
   +    // https://www.drupal.org/project/drupal/issues/2557299.
   
   @@ -109,14 +114,9 @@ public function testMediaTypeCreationReuseSourceField() {
   +    // https://www.drupal.org/project/drupal/issues/2557299.
   

   Do we usually link to the bug we're fixing when we fix it?

1. +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
   @@ -246,10 +247,13 @@ public static function validateMachineName(&$element, FormStateInterface $form_s
   -    if ($element['#default_value'] !== $element['#value']) {
   ...
   +    if ($form_state->get('needs_revalidation') === $element['#name'] || $element['#default_value'] !== $element['#value']) {
   

   Stepped through this with @phenaproxima. This code goes back to the origin of machine name validation as a standalone concept.

   However, I don't understand why we're checking #default_value at all.

   My suggestion would be to switch the condition to if (isset($element['#value'])) { and always run the validation.

2. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestMachineNameValidationForm.php
   @@ -0,0 +1,147 @@
   +    $form['snack_configure_button'] = [
   +      '#type' => 'submit',
   +      '#name' => 'snack_configure',
   +      '#value' => 'Configure snack',
   +      '#limit_validation_errors' => [['snack']],
   +      '#validate' => ['::buildAjaxSnackConfigureFormValidate'],
   +      '#submit' => ['::buildAjaxSnackConfigureFormSubmit'],
   +      '#executes_submit_callback' => TRUE,
   +      '#ajax' => [
   +        'callback' => '::buildAjaxSnackConfigureForm',
   +        'wrapper' => 'snack-config-form',
   +      ],
   +      '#attributes' => ['class' => ['js-hide']],
   +    ];
   
   +++ b/core/modules/system/tests/src/Functional/Form/FormTest.php
   @@ -770,4 +770,36 @@ public function testRequiredAttribute() {
   +  public function testMachineNameRequiredFormAjaxSubmit() {
   

   When this test is rewritten as a Functional JS test, this extra button shouldn't be necessary

3. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestMachineNameValidationForm.php
   @@ -0,0 +1,147 @@
   +    $form['snack_configs']['apple']['#type'] = 'details';
   +    $form['snack_configs']['apple']['#title'] = 'Configure Apple';
   +    $form['snack_configs']['apple']['#open'] = TRUE;
   +    $form['snack_configs']['pear']['#type'] = 'details';
   +    $form['snack_configs']['pear']['#title'] = 'Configure pear';
   +    $form['snack_configs']['pear']['#open'] = TRUE;
   +    $form['snack_configs']['potato']['#type'] = 'details';
   +    $form['snack_configs']['potato']['#title'] = 'Configure potato';
   +    $form['snack_configs']['potato']['#open'] = TRUE;
   

   🤔 Can't see why we need this for the test.

Okay, this should address all of the feedback. Let's see if this flies.

+++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
@@ -247,7 +245,7 @@ public static function validateMachineName(&$element, FormStateInterface $form_s
     // Verify that the machine name is unique.
-    if ($element['#default_value'] !== $element['#value']) {
+    if (isset($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.'));

This won't work. Forms that are editing something with a machine name will always fail the existing check with this change. That's why the default value check is there.

@tim.plunkett asked for an interdiff back to #80

I wish this were solvable in a more holistic way (aka rewriting how exists callbacks work) but that is out of scope.
Thanks for the updated approach and test, @alexpott

@mahtab_alam what's the purpose of #90? It's missing the new files (all the tests).

I can't "officially" RTBC this since I worked on it recently, but +1 for RTBC of #86.

So #90 is just #86 without the new files so I'm removing @mahtab_alam from the issue credit as #90 doesn't seem to change anything and there's no comment explaining the purpose.

I wonder if there's a better approach available. The problem is being caused by the fact that the default value is assumed to the initial value the form element was created with. This is not true for Ajax requests where the value before the submission becomes the new default value. So we can store the initial values and check that instead. That makes the whole thing a bit less opaque at least to me.

If @tim.plunkett as Forms API maintainer feels this is a bad path to pursue then we can revert to #86.

Fixing the tests. Can't use $element['#id'] because that changes on AJAX submission doh.

Whoops.

#2981332: Media type form and Filter format form set machine name #default_value incorrectly for new entities shows yet another solution that doesn't rely on state at all but fixes the default value setting.

I've re-written the issue summary to better explain what the problem is and its scale. I think due to the scale in core and probably contrib this issue is critical. People shouldn't be able WSOD their Drupal install via regular operation.

The steps to reproduce in the issue summary can be adapted for the responsive image case:

1. Install responsive_image
2. Go to admin/config/media/responsive-image-style/add
3. Set the label/machine name to narrow
4. Choose the Breakpoint group
5. Press save
6. See WSOD with The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">Drupal\Core\Entity\EntityStorageException</em>: &#039;responsive_image_style&#039; entity with ID &#039;narrow&#039; already exists.

+++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
@@ -150,6 +150,17 @@ public static function processMachineName(&$element, FormStateInterface $form_st
+      $initial_values[$element['#name']] = $element['#default_value'];

I don't think $element['#name'] is a good key. It's possible that if multiple forms for the same type of entity are on the same page this might cause a problem. Therefore changing this to use $element['#parents'].

Improved the documentation.

Spent a little bit more time thinking about this. For me the real issue is that \Drupal\Core\Entity\EntityForm::afterBuild() results in an entity being set with invalid values because it occurs before form validation. I'm not sure this is the best behaviour but changing it feels very very difficult because the whole purpose is so that AJAX submissions get a updated $this->entity to work with. Anyhow, I still think #105 is the best solution.

I still think #105 is the best solution.

Yep, I think that is correct.... at least #105 is a well through out solution.

So while looking over the code I saw just one a minor issue.

For the new test code I have removed a few calls to t().

From https://www.drupal.org/docs/8/phpunit/phpunit-browser-test-tutorial

When to use t() in browser tests
Never! Nope, not in assertion messages, not for button labels, not for text you assert on the page. You always want to test the literal string on the page, you don't want to test the Drupal translation system.

+++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
@@ -150,6 +151,18 @@ public static function processMachineName(&$element, FormStateInterface $form_st
+    $element_path = implode('][', $element['#parents']);

One thing that worries me about using $element['#parents'] as a key is that additional process callbacks later on might change it, which if I understand things correctly could trigger this bug again. I'm not sure how much of an edge case that is, but it's worth mentioning here. Perhaps this should be set in an after_build callback, or maybe #array_parents should be used instead of #parents? Or maybe we should generate an arbitrary hash to identify the element in form state? Or should we not even worry about this?

Yeah I had a similar fear with using #name too. At least #parents allows fields with the same #name in the same form with different parents. But yeah maybe #array_parents would be better because that should not change.

Moved to using #array_parents and also merged the new JS test with an existing one that is fit for the same purpose.

Manually tried to break it but it seemed to work as expected for several forms (node types/media types/text formats).
The tests look good, obsolete media code is removed and all feedback is addressed. I think it's done!

What's wrong with #name? I see that discussed in #104, but I was fairly certain we now have separate form_states per form, so it wouldn't matter how many forms are on a page...

@tim.plunkett I think using #array_parents would allow someone to have two machine names with the same #name on the same form where they are using #tree to so they don't overwrite each others form values.

I've tried testing #113 but there are problems with forms like

    $form['name'] = [
      '#type' => 'textfield',
    ];
    $form['tree'] = [
      '#type' => 'container',
      '#tree' => TRUE,
    ];<code>

$form['tree']['name'] = [
'#type' => 'textfield',
'#default_value' => 'blah',
];

That seem to be well beyond the scope of this issue. The default value from $form['tree']['name'] is copied to $form['name'] ?!?!

So #name is probably okay as forms don't seem to work very well when there are duplicates but I'm not sure that we should rely on the fact that FAPI struggles with multiple elements with the same #name as that feels like a bug to me.

The element #name, as I understand is, is the value that will appear in the HTML as <input name="...">. I'm pretty sure those can't be duplicates anyway, because otherwise the values submitted to the server would be fubar'ed. So #name should, in theory, work fine (I'd like a Form API expert to validate that statement, though). I also feel like #array_parents is also pretty safe Drupal-specific unique identifier, though, I don't feel terribly strongly about changing it to #name.

@tim.plunkett nothing is wrong with #name! I've overcomplicated it. I've just learnt when you #tree the parents get added to the name automatically so it's perfect for our use.

Forgot about the unit test.

Glad we got that nailed down. Interdiffs look good to me, so back to RTBC once all backends are green.

Looks great, thanks for that change. Signing off as a FAPI maintainer ✅

I think it might be worth a follow-up for EntityForm::afterBuild() - although whether it's actually fixable is a different matter. A couple of the comments in the test coverage are hard to read, but I could not think of a way to make them more readable, just a difficult thing to describe.

Yay, this not only helps Media, but also the CKEditor module :)