Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#29 | 2105661_29.patch | 7.78 KB | chx |
#29 | diffdiff.txt | 943 bytes | chx |
#25 | interdiff.txt | 2.16 KB | tim.plunkett |
#25 | form-redirect-2105661-25.patch | 7.74 KB | tim.plunkett |
#23 | redirect-2105661-23.patch | 7.78 KB | dawehner |
Comments
Comment #1
tim.plunkettComment #2
dawehnerI do prefer to not change the patch size, thought we should support options.
Comment #3
dawehnerAdded a quick todo to maybe support a route name only, so the common case can be used really simple.
Comment #4
tim.plunkettSo 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.
Comment #5
tim.plunkettI vote no, and to remove this @todo.
Those $options should be $form_state['redirect_route']['options']
Comment #6
dawehnerI am okay with that, though note: this idea comes directly from the current behavior of $form_state['redirect']
Good catch!
Comment #7
jibranWhy'absolute' is not the part of defaults array?
Comment #8
tim.plunkettBecause it always needs to be absolute, and if they specify some other options like query, the absolute will be ignored.
Comment #9
iamEAP CreditAttribution: iamEAP commentedIf we deprecated the path-based redirect routing, how would one go about redirecting after form submit to an external site?
Comment #10
tim.plunkettHm, we could consider still checking
is_string($form_state['redirect']) && url_is_external($form_state['redirect])
Comment #11
iamEAP CreditAttribution: iamEAP commentedHmm... Then we'd no longer be able to pass through $options for external URLs?
Comment #12
dawehnerWell all you need is to call the $urlgenerator->generateFromPath() as it has all the parameters you need to also create external urls.
Comment #13
iamEAP CreditAttribution: iamEAP commentedWhich 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, butsubmit_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.
Comment #14
dawehnerIs this really a common usecase, just wondering.
Comment #15
tim.plunkettI can't think of a time I've had a drupal form redirect to an external URL. That sounds of of scope of FAPI.
Comment #16
iamEAP CreditAttribution: iamEAP commentedMuch more common in a marketing context; particularly integrating Drupal forms with external CRM / marketing automation systems. Reasons you might need to do it:
Comment #17
tim.plunkettOkay, someone else can work on this.
Comment #18
iamEAP CreditAttribution: iamEAP commentedSorry, 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).
Comment #19
Xanodrupal_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.Comment #20
dawehnerI 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.
Comment #21
dawehnerThis adds some unit tests as well as direct redirect support.
Comment #23
dawehnerThere we go.
Comment #25
tim.plunkettI made this mistake a bunch while writing the other FormBuilderTest stuff...
Also fixed some docs.
Comment #26
dawehner+1 for the recent changes.
Comment #27
iamEAP CreditAttribution: iamEAP commentedLooks 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!
Comment #28
webchickThis 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. :(
Comment #29
chx CreditAttribution: chx commentedRerolled, there's a single use statement added to FormBuilderTest which broke the patch, see diffdiff for more.
Comment #30
webchickGreat, thanks!
Committed and pushed to 8.x.
This'll need a change notice, but probably best to wait until patch #2.