Updated: Comment #N

Problem/Motivation

$form_state['redirect'] is used to redirect during form submission.
However, it only supports paths, and we have no way to use routes

Proposed resolution

There are 220 usages of $form_state['redirect'], which is far too many to do at once.
This will add $form_state['redirect_route'], make one real and one test conversion, and open a follow-up to do the rest.

Remaining tasks

Write the patch/tests

User interface changes

N/A

API changes

API addition, and deprecating the old path-based redirect.

Files: 
CommentFileSizeAuthor
#29 2105661_29.patch7.78 KBchx
PASSED: [[SimpleTest]]: [MySQL] 59,908 pass(es).
[ View ]
#29 diffdiff.txt943 byteschx
#25 interdiff.txt2.16 KBtim.plunkett
#25 form-redirect-2105661-25.patch7.74 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,810 pass(es).
[ View ]
#23 redirect-2105661-23.patch7.78 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,258 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#23 interdiff.txt736 bytesdawehner
#21 redirect-2105661-21.patch7.77 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,672 pass(es), 20 fail(s), and 8 exception(s).
[ View ]
#6 interdiff.txt1.1 KBdawehner
#6 form_state-2105661-6.patch4.04 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,026 pass(es).
[ View ]
#3 redirect-2105661-3.patch4.14 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,436 pass(es).
[ View ]
#3 interdiff.txt1.86 KBdawehner
#1 redirect-2105661-1.patch3.14 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,734 pass(es).
[ View ]

Comments

Status:Active» Needs review
Issue tags:+MenuSystemRevamp
StatusFileSize
new3.14 KB
PASSED: [[SimpleTest]]: [MySQL] 58,734 pass(es).
[ View ]

Status:Needs review» Needs work

I do prefer to not change the patch size, thought we should support options.

Status:Needs work» Needs review
StatusFileSize
new1.86 KB
new4.14 KB
PASSED: [[SimpleTest]]: [MySQL] 58,436 pass(es).
[ View ]

Added a quick todo to maybe support a route name only, so the common case can be used really simple.

So when you don't need route params, you pass a string, but when you do, you passed an associative array?
Hm.
It might be nice for those cases, but it would be more confusing for the rest.

In ConfirmFormBase we have a similar construct, and it always needs an associative array.

Good call on the $options.

Status:Needs review» Needs work
  1. +++ b/core/includes/form.inc
    @@ -1325,6 +1325,18 @@ function drupal_redirect_form($form_state) {
    +    // @todo Decide whether we want to support to just pass a route_name only.

    I vote no, and to remove this @todo.

  2. +++ b/core/includes/form.inc
    @@ -1325,6 +1325,18 @@ function drupal_redirect_form($form_state) {
    +    $options['absolute'] = TRUE;
    +    $url = \Drupal::urlGenerator()->generateFromRoute($form_state['redirect_route']['route_name'], $form_state['redirect_route']['route_parameters'], $options);

    Those $options should be $form_state['redirect_route']['options']

Status:Needs work» Needs review
StatusFileSize
new4.04 KB
PASSED: [[SimpleTest]]: [MySQL] 59,026 pass(es).
[ View ]
new1.1 KB

I vote no, and to remove this @todo.

I am okay with that, though note: this idea comes directly from the current behavior of $form_state['redirect']

Those $options should be $form_state['redirect_route']['options']

Good catch!

+++ b/core/includes/form.inc
@@ -1326,6 +1326,16 @@ function drupal_redirect_form($form_state) {
+    $form_state['redirect_route'] += array(
+      'route_parameters' => array(),
+      'options' => array(),
+    );
+    $form_state['redirect_route']['absolute'] = TRUE;

Why'absolute' is not the part of defaults array?

Because it always needs to be absolute, and if they specify some other options like query, the absolute will be ignored.

If we deprecated the path-based redirect routing, how would one go about redirecting after form submit to an external site?

Hm, we could consider still checking is_string($form_state['redirect']) && url_is_external($form_state['redirect])

Hmm... Then we'd no longer be able to pass through $options for external URLs?

Hmm... Then we'd no longer be able to pass through $options for external URLs?

Well all you need is to call the $urlgenerator->generateFromPath() as it has all the parameters you need to also create external urls.

Which is fine if your redirect is generated in one place. It would be extremely inconvenient if submit_handler_a sets it to http://example.com?foo=bar, but submit_handler_b wants to set foo=baz, add new query param fizz, and also make it https.

I would consider that a pretty severe regression; even Drupal 5 had this capability.

Is this really a common usecase, just wondering.

I can't think of a time I've had a drupal form redirect to an external URL. That sounds of of scope of FAPI.

Much more common in a marketing context; particularly integrating Drupal forms with external CRM / marketing automation systems. Reasons you might need to do it:

  • The actual asset (or even just "thank you" page) being delivered after form submission doesn't live in Drupal, it lives on Platform X (e.g. register now for [free trial, whitepaper, on-demand video, etc])
  • There's conversion/goal tracking that takes place on a page served up on Platform X.
  • Company B is a partner to Company A. Company B does lead capture on their Drupal site, and passes leads on to assets/pages/etc. located on Company A's website.

Assigned:tim.plunkett» Unassigned

Okay, someone else can work on this.

Sorry, I didn't mean to sidetrack this completely, since my concerns are more with how we do the follow-up and less with the API addition, here.

#6 seemed fine to me; seems like it would be appropriate to throw in a test for route-based redirects in a similar place to the place we test the existing form redirect system (but I could not find that place).

Is this really a common usecase, just wondering.

I can't think of a time I've had a drupal form redirect to an external URL. That sounds of of scope of FAPI.

drupal_redirect_form() is the only code in FAPI that uses $form_state['redirect'] and it only does simple redirects by passing the value on to RedirectResponse, so we don't technically need routes. I wonder if this conversion is not overzealous, because we limit the flexibility, and I don't see much gain.

I would suggest to not require to use redirect objects but allow people to specify external urls.

In general I don't think that we have to support to easily alter external url redirects.

Issue tags:+phpunit
StatusFileSize
new7.77 KB
FAILED: [[SimpleTest]]: [MySQL] 59,672 pass(es), 20 fail(s), and 8 exception(s).
[ View ]

This adds some unit tests as well as direct redirect support.

Status:Needs review» Needs work
Issue tags:-phpunit

The last submitted patch, redirect-2105661-21.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new736 bytes
new7.78 KB
FAILED: [[SimpleTest]]: [MySQL] 59,258 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

There we go.

Status:Needs review» Needs work

The last submitted patch, redirect-2105661-23.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.74 KB
PASSED: [[SimpleTest]]: [MySQL] 59,810 pass(es).
[ View ]
new2.16 KB

I made this mistake a bunch while writing the other FormBuilderTest stuff...

Also fixed some docs.

+1 for the recent changes.

Status:Needs review» Reviewed & tested by the community

Looks great. I'll keep track of any follow-up with any concerns I had above, but the ability to use an instance of RedirectResponse directly may be good enough for my needs. Tests also look good to me.

RTBC!

Status:Reviewed & tested by the community» Needs work

This looks consistent with what we've done elsewhere to support routes over paths. The tests are nice, too!

Though, hm. It's a little funny to add this new, secondary "redirect_route" property that once a 100K follow-up happens will most likely be the 90%+ case, and leave the old "redirect" one in-place as the "primary"-looking API that is actually an edge case in D8. It probably makes more sense ultimately to accept an API break and swap them (so "redirect" == route, and "redirect_path" == path) to make this less confusing for new developers. However, I can see the value of getting this in as a non-BC breaking API addition first, doing the full conversions separately, and then seeing what we've got at that point. We should also switch the order of the condition checks in redirectForm() at that point, too.

No longer applies, though. :(

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new943 bytes
new7.78 KB
PASSED: [[SimpleTest]]: [MySQL] 59,908 pass(es).
[ View ]

Rerolled, there's a single use statement added to FormBuilderTest which broke the patch, see diffdiff for more.

Status:Reviewed & tested by the community» Fixed

Great, thanks!

Committed and pushed to 8.x.

This'll need a change notice, but probably best to wait until patch #2.

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.