Module uninstall form does not validate correctly and breaks the confirmation form after 60 seconds

To me this sounds major, since there is a workaround (reload the form). Icky bug though, and good find!

Given that the uninstall form is not like a super high frequent accessed page, I think it would be totally legit to increase the number,
but yeah I guess we should have some common pattern to show some form of message what was going on.

I guess the timeout could be set to 6 hours here as well, or even a week which is what we use in tempstore.

Just some ideas for the message (which we could maybe use on other forms as well):

a) ping the server when we're about to expire the data, setting the key/value expiry date into the future on every ping (though that won't work when there's no internet connection).
b) inform users on the confirmation form itself that they have X amount of time to press the confirm button (which may be confusing and raise more questions than it answers).
c) if users press the confirm button when the data is already expired, display a big red error screen that links back to the uninstall page.
d) prevent users from having a failed confirmation attempt in the first place, i.e. when they're about to time out we auto redirect them away from the confirmation page to a big red error page explaining what just happened

I would lean toward d) with a fallback to c) in case javascript breaks or the internet connection goes away, and just set the timeout to an as large as possible number so we don't have to show any unnecessary error screens anyway.

Note that the known "workaround" is submitting the form in less than 60 second

Yes either that, or resubmitting the form.

It's definitely an annoying bug, but it doesn't prevent uninstalling modules altogether and it's not like an entity form where there's actual data loss. Per https://www.drupal.org/core/issue-priority this is definitely major for me.

It seems to be that we should set the time to a reasonable high value and be done with it ...

@dawehner or even unreasonably high, as in a week? We use a week in tempstore as well...

Still, this needs an error message at least. Also the redirect didn't work when using a URL prefix.

Looks like Simpletest wants the redirect to be an absolute URL

Maybe someone else can confirm but I don't think this particular form is cached...

Actually there is an additional problem with the uninstall form, it doesn't validate correctly.

If we submit an empty uninstall form, it will trigger the submit hooks despite an error happening in the validation. This is because the validation error is a simple drupal_set_message() as opposed to being filed against the form, so the $form_state->setRedirect('system.modules_uninstall'); gets overwritten by $form_state->setRedirect('system.modules_uninstall_confirm'); in the submit hooks.

The current issue is about bad things happening in the transition from the uninstall form to the uninstall confirm form as well, so we can probably add a fix for this to the patch?

So just to summarize, the issues here were:

1. The uninstall form didn't validate correctly and was redirecting to the confirm form even if there was an error, as the error wasn't filed against the form.
2. The confirm form didn't validate correctly as it was redirecting to an unprefixed URL and didn't display any error message.
3. We only gave people 60 seconds to confirm before losing the module list.

1. +++ b/core/modules/system/src/Form/ModulesUninstallConfirmForm.php
   @@ -135,7 +135,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -      return new RedirectResponse('/admin/modules/uninstall');
   +      drupal_set_message($this->t('The selected modules could not be uninstalled, either due to a website problem or due to the uninstall confirmation form timing out. Please try again.'), 'error');
   +      return new RedirectResponse($this->getCancelUrl()->setAbsolute()->toString());
   

   Good idea

2. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   @@ -171,7 +171,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -      drupal_set_message($this->t('No modules selected.'), 'error');
   +      $form_state->setErrorByName('uninstall', $this->t('No modules selected.'));
   

   +1

3. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   @@ -184,7 +184,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   -    $this->keyValueExpirable->setWithExpire($account, $uninstall, 60);
   +    $this->keyValueExpirable->setWithExpire($account, $uninstall, 604800);
    
   

   Do we have that stored somewhere accessible in form cache? Should we also document why we hav chosen that particular value?

Added a comment.

re-roll. Also set the expiration date to 6 hours, though not sure this form even uses the form cache?

I guess no, but its still a fair number

Re-queued the patch for testing because of test fail unrelated to this patch (which also occurred on other issues):

"FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]."

Back to RTBC

Committed 99e0777 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.