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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
Issue tags: +MenuSystemRevamp
FileSize
3.14 KB
dawehner’s picture

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.86 KB
4.14 KB

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

tim.plunkett’s picture

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.

tim.plunkett’s picture

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']

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.04 KB
1.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!

jibran’s picture

+++ 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?

tim.plunkett’s picture

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

iamEAP’s picture

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

tim.plunkett’s picture

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

iamEAP’s picture

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

dawehner’s picture

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.

iamEAP’s picture

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.

dawehner’s picture

Is this really a common usecase, just wondering.

tim.plunkett’s picture

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.

iamEAP’s picture

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.
tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

Okay, someone else can work on this.

iamEAP’s picture

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).

Xano’s picture

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.

dawehner’s picture

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.

dawehner’s picture

Issue tags: +PHPUnit
FileSize
7.77 KB

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
736 bytes
7.78 KB

There we go.

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
7.74 KB
2.16 KB

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

Also fixed some docs.

dawehner’s picture

+1 for the recent changes.

iamEAP’s picture

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!

webchick’s picture

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. :(

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
943 bytes
7.78 KB

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

webchick’s picture

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.