Regression: Form submit redirects from 403/404 pages are no longer possible

People talked a lot in IRC, I could not follow.

Nice, this issue is making alot of sense.

1. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/RedirectTest.php
   @@ -19,12 +19,12 @@ class RedirectTest extends WebTestBase {
   +      'description' => 'Tests functionality of form redirection.',
   

   Sounds a bit weird, switch them round? i.e. "Tests form redirection functionality."

2. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/RedirectTest.php
   @@ -85,4 +85,32 @@ function testRedirect() {
   +  function testRedirectFromErrorPages() {
   

   Shouldn't we also have a test case using the destination parameter? As that is one of the things that has changed most in the patch.

3. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/RedirectTest.php
   @@ -85,4 +85,32 @@ function testRedirect() {
   +    $this->assertEqual($this->getUrl(), $expected, 'Redirected to correct url.');
   

   Does this also check the query string? I can't remember.

4. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Form/RedirectBlockForm.php
   @@ -0,0 +1,45 @@
   +    $form['actions']['submit'] = array('#type' => 'submit', '#value' => $this->t('Submit'));
   

   Nitpick alert: separate onto different lines.

5. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Plugin/Block/RedirectFormBlock.php
   @@ -0,0 +1,39 @@
   +    return drupal_get_form('Drupal\form_test\Form\RedirectBlockForm');
   

   Can we use form builder instead.

I can't find anything, but yeah obviously also cannot RTBC it.

I do not understand your question.... If you look up a few lines, you will see...

I just meant, does the return value of getUrl() definitely contain the query string too. Which it does, so that's fine.

What form builder? I don't see one on BlockBase, and on a 10-line class for testing purposes,

Yes, I meant use formBuilder :) just not the old deprecated function. Also, there is no harm in injecting this properly even if it is a test class. This sets an example that people look at for how things are done.

So yes, I think someone should still add some coverage for the destination parameter whilst we are changing this code that touches it. Otherwise this is looking good.

did you check? What's missing?

Yes, of course.

+++ b/core/modules/user/lib/Drupal/user/Form/UserLoginForm.php
@@ -104,10 +104,17 @@ public function buildForm(array $form, array &$form_state) {
+    if (!$this->request->request->has('destination')) {
+      $form_state['redirect_route'] = array(
+        'route_name' => 'user.view',
+        'route_parameters' => array('user' => $account->id()),
+      );
+    }
+    else {
+      $this->request->query->set('destination', $this->request->request->get('destination'));
+    }

This. destination parameter. I thought that was pretty self explanatory.. For the UserLoginForm.

Yes, exactly that! :)

This is looking pretty good. Let's see what the bot thinks - I think this is ready to fly though.

1. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/RedirectTest.php
   @@ -85,4 +85,32 @@ function testRedirect() {
   +  function testRedirectFromErrorPages() {
   
   +++ b/core/modules/user/lib/Drupal/user/Tests/UserLoginTest.php
   @@ -23,6 +23,18 @@ public static function getInfo() {
   +  function testLoginDestination() {
   

   public function

2. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Form/RedirectBlockForm.php
   @@ -0,0 +1,45 @@
   +    $form_state['redirect_route'] = array(
   +      'route_name' => 'form_test.route1',
   +      'options' => array('query' => array('test1' => 'test2')),
   +    );
   +  }
   +}
   

   Can this use a \Drupal\Core\Url object directly? One less thing to convert later.

   Also missing blank line before end of class

3. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Plugin/Block/RedirectFormBlock.php
   @@ -0,0 +1,39 @@
   +  public function build() {
   +    return \Drupal::formBuilder()->getForm('Drupal\form_test\Form\RedirectBlockForm');
   +  }
   +}
   

   I guess it doesn't matter since its a test block, but this is injectable if we want. Also, missing blank line before end of class

4. +++ b/core/modules/user/lib/Drupal/user/Form/UserLoginForm.php
   @@ -104,10 +104,17 @@ public function buildForm(array $form, array &$form_state) {
   +    // A destination was set, probably on an exception controller,
   +    if (!$this->request->request->has('destination')) {
   +      $form_state['redirect_route'] = array(
   +        'route_name' => 'user.view',
   +        'route_parameters' => array('user' => $account->id()),
   +      );
   +    }
   +    else {
   +      $this->request->query->set('destination', $this->request->request->get('destination'));
   +    }
   

   Is this something we should bake into redirectForm? I remember doing a similar dance in EntityFormController::delete().

Is this something we should bake into redirectForm? I remember doing a similar dance in EntityFormController::delete().

This is actually the full idea of the issue, we do remove the support for destination inside exception controllers and move that behavior to move back to the previous page into the login block.

Ahh, the specific 404 in the issue title confused me, I now see why we're only changing the user login block (so you can log in from a 403 and get redirected).

Committed 455840f and pushed to 8.x. Thanks!