Media Type entities don't validate machine name properly

Thanks for reporting, this is a pretty nasty bug :)

Bumping to major because a "normal" user action on the UI shouldn't trigger a WSOD.

Added steps to reproduce to the IS.

After digging into this a bit, this is what I could find:

- MachineName::validateMachineName() is being called during the first AJAX callback (when the source select is triggered), and correctly calls $form_state->setError() due to the machine name not being unique.
- This call to $form_state->setError() does NOT set the error because at this point $form_state->getLimitValidationErrors() returns an empty array.
- The empty array comes from FormValidator::determineLimitValidationErrors(), which will set it to [] whenever the triggering element does not execute submit callbacks and there is no explicit '#limit_validation_errors' key on the element.

Setting the element to have '#executes_submit_callback' => TRUE, appears to solve the problem. Also added a specific submit for this element, to prevent the whole form from being submitted on the AJAX call.

I'm adding a test-only patch to demonstrate the bug.

Also, solving this has surfaced to the UI the notices reported in #2932222: Undefined index: source_configuration breaks AJAX:

Notice: Undefined index: source_configuration in Drupal\media\MediaTypeForm->validateForm() (line 289 of /var/www/html/core/modules/media/src/MediaTypeForm.php)

Because now it's on the UI, I'm including here the fix from that patch as well. If this is OK, please credit @Darvanen for the patch as well.

Fine if you found a workaround for that specific form but this is a generic problem for all forms with ajax elements, we've had #2557299: Any AJAX call disregards machine name verification when AJAX is used and leads to a fatal error open for a long time about that to fix it in general but it's complicated IIRC.

Thank you @Berdir and @Darvanen

This one has a workaround for a specific form that affects site builders in a much more evident way. Solving it generically might take some time, so IMHO I believe it is justified to have a solution for the Media type form in the short term as its own issue.

I think I'm OK with this workaround, as long as we open a follow-up issue to remove it once #2557299: Any AJAX call disregards machine name verification when AJAX is used and leads to a fatal error lands, and a comment in the patch which points to that follow-up.

Created follow-up: #2934154: [PP-1] Remove workaround to enforce machine name validation on MediaType forms, and included a @TODO in the patch.

+++ b/core/modules/media/src/MediaTypeForm.php
@@ -131,6 +131,13 @@ public function form(array $form, FormStateInterface $form_state) {
+      // @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.

Minor formatting nitpick: @todo should be lowercase, and the subsequent comment lines should be indented by two spaces between the // and the text.

FWIW I am also okay with an @todo postponed on the followup, at least in principle.

Looks great! Just nits.

1. +++ b/core/modules/media/src/MediaTypeForm.php
   @@ -131,6 +131,13 @@ public function form(array $form, FormStateInterface $form_state) {
   +      '#submit' => [[get_called_class(), 'rebuildSubmit']],
   

   We can use static::class instead of get_called_class().

2. +++ b/core/modules/media/src/MediaTypeForm.php
   @@ -234,6 +241,13 @@ public function form(array $form, FormStateInterface $form_state) {
   +  public static function rebuildSubmit(array $form, FormStateInterface $form_state) {
   

   $form is passed by reference.

3. +++ b/core/modules/media/src/MediaTypeForm.php
   @@ -234,6 +241,13 @@ public function form(array $form, FormStateInterface $form_state) {
   +    $form_state->setRebuild(TRUE);
   

   No need for TRUE here.

4. +++ b/core/modules/media/src/MediaTypeForm.php
   @@ -273,7 +287,7 @@ protected function getSourceSubFormState(array $form, FormStateInterface $form_s
   +    if (array_key_exists('source_configuration', $form['source_dependent'])) {
   

   Let's use isset($form['source_dependent']['source_configuration']) instead.

5. +++ b/core/modules/media/src/MediaTypeForm.php
   @@ -296,7 +310,7 @@ function ($item) {
   +    if (array_key_exists('source_configuration', $form['source_dependent'])) {
   

   Same here.

+++ b/core/modules/media/src/MediaTypeForm.php
@@ -234,6 +241,13 @@ public function form(array $form, FormStateInterface $form_state) {
+   * Rebuilds the form on select submit.
+   */
+  public static function rebuildSubmit(array $form, FormStateInterface $form_state) {

This should be a full code block.

Attached patch includes #13, #15 and the above code block.

+++ b/core/modules/media/src/MediaTypeForm.php
@@ -241,10 +241,13 @@
+   * Form Submission to rebuild the form on select submit.
+   *
+   * @param array $form
+   * @param \Drupal\Core\Form\FormStateInterface $form_state

SUPERNIT! The description should begin with "Form submission handler...", and the two parameters need a description line ("The structured form array" and "The current form state" oughta do the trick).

updates done

+++ b/core/modules/media/src/MediaTypeForm.php
@@ -241,10 +241,12 @@
    * @param \Drupal\Core\Form\FormStateInterface $form_state
+   *  Parent form state.

Aw man, sorry to do this, but...

1) The description needs to be indented by one more space.

2) The description should say "Current form state", not "parent".

update done

Ah yesh, this is good to go once Drupal CI passes it. Thanks, @robpowell!

I checked with @larowlan and he also mentioned being comfortable with this workaround/hotfix for this specific issue.

Committed and pushed to 8.5.x. Thanks!