Follow-up to #171267-34: form redirects removes get variables like sort and order
Quoting Crell:
Of course, we have some Drupal-specific logic in drupal_goto(), which I don't know that we can entirely eliminate. (destination= handling, etc.) That makes me wonder if we need a DrupalRedirectResponse object of some sort, (or probably a Drupal-namespaced RedirectResponse) that replicates the handling from drupal_goto(). GET query parameter persistence should be part of that, where appropriate, but I also worry about that class ending up with hard dependencies in it if we're not careful.
Comment | File | Size | Author |
---|---|---|---|
#174 | drupal-goto_response-1668866-174.patch | 71.95 KB | ParisLiakos |
#167 | drupal-goto_response-1668866-167.patch | 71.94 KB | ParisLiakos |
#162 | drupal-goto_response-1668866-162.patch | 71.39 KB | ParisLiakos |
#159 | drupal-goto_response-1668866-159.patch | 68.41 KB | ParisLiakos |
#159 | interdiff.txt | 6.09 KB | ParisLiakos |
Comments
Comment #1
Crell CreditAttribution: Crell commentedTagging.
Also retitling slightly, because drupal_goto() the function should go away in favor of someone returning a RedirectResponse. Just it may be a subclass thereof.
Comment #2
marcingy CreditAttribution: marcingy commentedInitial start on this.
Adds a new helper sub class and converts references to drupal_goto. Docs still need to be cleaned up.
The major issue is that after this change is that batch processing no longer seems to work because we return to the calling function the simple test form then redirects on itself instead of the old means where drupal exit was excuted. Not sure what the solution is.
Comment #4
Crell CreditAttribution: Crell commentedRedirectResponse isn't really performed; it's more "returned", or maybe "issued".
This very much worries me. Why can't we just return new RedirectRepsonse, as we do a few lines down?
Remember that current_path() is going away, hopefully, so we shouldn't continue to rely on it in new code.
Can we replace everything but this for now to get testbot working, then fix this part?
This is no longer necessary. It's an object. We can create it and then call well-documented methods on it rather than shaving everything into an undocumented array.
request() is also temporary, so should not be relied upon.
Why is this here? Looks like flotsam from another patch.
I don't think we need an inception patch. :-)
I also had another thought regarding the destination question, which is really the only reason we need to override the base response class. There are two places that we could potentially mess with the redirect path OUTSIDE of the class.
One is in prepare(), which is passed a request object and is responsible for tidying up the response to match the request. Eg, matching the encoding, handling HEAD requests by stripping off the body, etc. We could subclass and override prepare(), call parent::, and then check the request object that was passed in for a destination parameter. If so, overwrite $this->targetUrl.
The other is in a listener for kernel.response. There, we also have access to the request and can check for a destination. In this case, we'd need to add a setTargetUrl() method (since there isn't one) and call it.
I think I prefer the former (prepare()), as it's cleaner and probably a bit more performant.
For eliminating url()... I'm not sure yet, but we do want to replace that with a UrlGenerator based on routes. So, perhaps going through the generator (which I presume will be responsible for aliasing) becomes something you have to do before you create the Response?
return new RedirectResponse($this->generator->generate('node_page', $nid));
Or something.
Comment #5
marcingy CreditAttribution: marcingy commentedRe-roll reverting the batch changes. I have tested locally and drupal still installs so hopefully this will work on d.o as well.
Removed and replaced with a simple instancation followed by a user func call. Added an @todo around the need to add additional methods and got rid of the patch cruff.
Lets see how many tests fail...
Comment #7
marcingy CreditAttribution: marcingy commentedWe still have issues I have traced back to effectively a change in logic flow. Previously drupal_exit terminated processing now of course we are returning so excuting continues.
Doing something along these lines seems to resolve some of the issue but this approach doesn't feel correct.
Comment #8
chx CreditAttribution: chx commentedmakes me run screaming. Is this still necessary in 5.3? Isn't there a nicer way now? I don't know, I hope.
Comment #9
marcingy CreditAttribution: marcingy commentedThat part is gone in the later patch. There a lot of strange things happen with form_state['redirect'] at the moment I am working on rolling a patch that swaps out all the none form elements as the idea in #7 leaves batches being triggered for tests correctly but form build processing still broken. The addition of debug() calls clearly show that the redirect is firing but the kernels response doesn't result in a redirect.
Comment #10
marcingy CreditAttribution: marcingy commentedRe-roll without the form changes for now se comments above re the issues.
Comment #12
marcingy CreditAttribution: marcingy commentedSo the failures above relate to cases where we are returning an object from a FAPI call when an array is expected. Not sure really what the next step is as in effect is going to require fundmental changes in FAPI or we provide a way for FAPI to throw an error later even in drupal_get_form where a $form_state['redirect'] may not be in play.
Comment #13
marcingy CreditAttribution: marcingy commentedSo something kind of like this which removes the test failures for languages
Comment #14
Crell CreditAttribution: Crell commentedCan't we have FAPI check at some point to see if it has a Response object, and if so just return that up the stack?
Comment #15
marcingy CreditAttribution: marcingy commentedSome progress with tests there are still broken items but we are getting there. Setting to needs review for the bot.
Comment #17
Crell CreditAttribution: Crell commentedThis should be done with instancreof, not is_a(). (instanceof is a language construct so is a wee bit faster.)
Comment #18
Crell CreditAttribution: Crell commentedI'll see about digging into this during the MWDS today and tomorrow. Should I just work off this patch, or is there a branch I can work from? If not, I'll make a branch in WSCCI for it.
Comment #19
marcingy CreditAttribution: marcingy commentedI don't have a branch set up for it at the moment just the patch that is here.
Comment #20
Crell CreditAttribution: Crell commentedI've filed a PR with Symfony to expose a SetTargetUrl() method on RedirectResponse: https://github.com/symfony/symfony/pull/5081
After toying with it a bit, I decided that we can replace both the destination override AND the drupal_alter() call with an event listener; then we won't even need the Drupal subclass, most likely.
Once that's in, we can reimplement the destination override as a listener. The listener itself would function as a replacement for the drupal_alter('drupal_goto') call.
Comment #21
Dave ReidJust curious, how would #1678348: Add redirects to be cached in drupal_goto() be possible once this lands? Also note that RedirectResponse doesn't look like it supports passing in an array of headers in it's constructor.
Comment #22
Crell CreditAttribution: Crell commentedCommented on the other issue. Short version, all responses get Http cached, rather than "page" cached.
Comment #23
sunTagging.
Comment #24
Crell CreditAttribution: Crell commentedWell that was fast. :-) Looks like we'll need to update Symfony again, and then we can implement destination as a listener; That event would also serve as both hook_drupal_goto_alter(), so that hook can be eliminated. Yay!
Comment #25
aspilicious CreditAttribution: aspilicious commentedI'm confused can someone expplain what has to be done here?
The symfony stuff is in btw :)
Comment #26
Crell CreditAttribution: Crell commented1) Write a new kernel.response listener that implements the destination override logic using the new and shiny setTargetUrl() method of RedirectResponse. It can also keep the alter there, or convert that into another event. (If the latter, then the listener should get the event dispatcher via the DIC in its constructor.)
2) Replace the Drupal-specific response object in the patch above with the normal Symfony one.
3) Profit.
Comment #27
marcingy CreditAttribution: marcingy commentedSo I have attempted to update the patch to utilise a subscriber which responses to the following
Now this all well and good the listener fires but.....we then get
Because by the time the listener fires it seems that we have a response object rather than RedirectResponse object.
Now it maybe that I am doing something crazy wrong. (on the plus side I now understand subscribers!)
Comment #28
aspilicious CreditAttribution: aspilicious commentedCan you upload a patch?
Comment #29
tim.plunkettShould be setTargetUrl not SetTargetUrl
Comment #30
marcingy CreditAttribution: marcingy commentedI am just tidying some stuff up so as the patch actually isn't just a load of debug. @tim.plunkett thanks for noticing that still no joy.
Comment #31
marcingy CreditAttribution: marcingy commentedThis will fail so not even going to set to needs review. The listener is very much sudo code for what I assume should happen if I understand correctly. Although it really comes over as if it is too late in the call stack as the redirect has already happened.
Comment #32
tim.plunkettWell I'd rather see where it fails, so I can start debugging in the right place :)
Comment #33
aspilicious CreditAttribution: aspilicious commentedWell, you're creating a repsonse listener that responds on any kind of response.
Your page can fire multiple requests, see
http://symfony.com/doc/current/cookbook/service_container/event_listener...
So if you want to be sure it is a RedirectResponse you could do something like
if(!($event->getResponse() instanceOf 'RedirectResponse'))
return;
Or another condition, just be sure you need to redirect :)
Comment #34
aspilicious CreditAttribution: aspilicious commentedThis works for certain paths. Not yet the user login stuff...
Comment #35
aspilicious CreditAttribution: aspilicious commentedthis maybe can work...
Comment #36
aspilicious CreditAttribution: aspilicious commentedSince the beginning of the kernel there was an ugly redirect problem in overlay when clearing caches. This fixes this too somehow :)
Nice
Comment #38
marcingy CreditAttribution: marcingy commentedThis should fix a number of exceptions
Comment #40
aspilicious CreditAttribution: aspilicious commentedComment #42
aspilicious CreditAttribution: aspilicious commentedAt least some (I think most) of those test failures are cause by "broken" tests.
drupalLogout:
We should incoporate a RedirectResponse somewhere in WebTest....
Not sure how to do that... :(
EDIT: maybe they are real those fails... *digs deeper*
Comment #43
tim.plunkettIf you read through the original issue linked in the OP, you'll need to change the 'destination' handling directly.
Comment #44
aspilicious CreditAttribution: aspilicious commentedHmm, "user_menu_site_status_alter" does too much. It sets the status but it also redirects...
Nasty...
So.... I hacked my way around it by passing a new argument but I think the "setting of the status" and the "redirect" should be seperated.
We could make a UserBundle, register a new response listener that redirects.
Would be a tripple win:
1) this function does what it supposed to be doing
2) one function/class that handles user login responses, makes it also easier to override
3) I don't have to touch the maintenance mode subscriber for this
Now lets see what the bot thinks of my hack
Comment #46
aspilicious CreditAttribution: aspilicious commentedI'm working on a patch that should work with destination responses...
Comment #47
aspilicious CreditAttribution: aspilicious commentedThis should hopefully fix some tests :)
Comment #49
aspilicious CreditAttribution: aspilicious commentedAha! Interesting... I'm a newb...
So...
When you pass ?destination to a page there are 2 options:
1) go the stuff added at the destination part
2) fill in the form and than go to the destination
o_O
*needs help, what do we need to do*
Example of 1:
$this->drupalGet('user/logout', array('query' => array('destination' => 'user')));
Example of 2:
http://localhost/drupal8/#overlay=taxonomy/term/2/edit%3Fdestination%3Da...
Comment #50
aspilicious CreditAttribution: aspilicious commentedOk I'm getting closer. Form.inc needs a cleanup but I have this problem.
In form.inc I call "return new RedirectResponse(....)" but in contrast to other parts of drupal form system is slightly more complicated and so the "return new RedirectResponse" doesn't trigger the code that needs to parse the destination. Help?
Comment #51
aspilicious CreditAttribution: aspilicious commentedSucces
Comment #52
tim.plunkettHere's the interdiff for those that were curious, like me.
Comment #54
aspilicious CreditAttribution: aspilicious commentedAh crap...
Comment #55
aspilicious CreditAttribution: aspilicious commentedThis patch fixes a few issues but is still broken as hell...
The fact that form.inc is mixed with returning response objects and $form arrays is prety dangerous and ugly. Can't we do this differently?
Comment #56
aspilicious CreditAttribution: aspilicious commentedchanges
+ return new RedirectResponse(current_path(), '302', array('query' => drupal_container()->get('request')->query->all()));
+ return new RedirectResponse(current_path(), 302, array('query' => drupal_container()->get('request')->query->all()));
return new RedirectResponse('common-test/drupal_goto', array('query' => array('foo' => '123')), 301);
return new RedirectResponse('common-test/drupal_goto', 301, array('query' => array('foo' => '123')));
Comment #58
Crell CreditAttribution: Crell commentedIt's safer, I think, to get the current path from the request object in the container than current_path(). That function should be going away, I hope. :-) (Although, maybe that won't work 100% in testing. There's another issue open for that.)
instanceof is preferred over is_a. It also means you can instanceof RedirectResponse and it will make use of a "use" statement rather than having to specify the whole class name here.
I don't understand this at all. If we're not redirecting, force us to redirect? That doesn't make any sense to me...
I'd be OK with putting the "empty is " logic into the listener. Hm, although the parameter for the class is required, isn't it? Never mind.
Comment #59
aspilicious CreditAttribution: aspilicious commentedThats a left over, it's alrdy testing for that in the first "if" so this will always get called. So it never won't do the second part.
It's the class that doesn't accept empty paths
Don't I need a real object to check if I want to use instanceof?
This still doesn't solve the form api problem....
Comment #60
Crell CreditAttribution: Crell commented$response is a real object.
Comment #61
BrockBoland CreditAttribution: BrockBoland commentedNeeds issue summary
Comment #62
Crell CreditAttribution: Crell commentedIs anyone working on this still? It still has to happen...
Comment #63
catchIf it has to happen, why is it not critical or at least major?
Comment #64
Crell CreditAttribution: Crell commentedI just closed another major, so I guess I have a little bit of karma to burn moving this one up and restoring balance to the Force.
Comment #65
pdrake CreditAttribution: pdrake commentedCore has changed a lot since the last patch. I did my best to apply the previous patch as well as to address all of the new drupal_goto calls in core. Unfortunately, this has fundamentally broken tests on my local environment because they redirect funny. I'm posting the patch to share my work thus far and in hopes the testbot can tell me something I haven't learned running tests locally, or perhaps I'll have more luck after I get some sleep.
Comment #67
pdrake CreditAttribution: pdrake commented#65: drupal-replace_drupal_goto-1668866-65.patch queued for re-testing.
Comment #69
aspilicious CreditAttribution: aspilicious commentedSee below
This isn't allowed should url should be build with 'url' I guess before passing through the response.
Comment #70
BerdirI'm working on this a bit. Have some improvements but wasn't able to get the installer working yet. It just seems to skip the installation batch and jumps right to the site configuration part.
Comment #71
BerdirOk, I'm not making any progress here.
Here is the updated patch with some smaller changes that I made.
Apart from it not working anyway currently, I see two big issues with the current code*:
- As pointed out in #69, RedirectResponse currently doesn't allow for the same options as url()/drupal_goto(). I fixed those two in this patch but I really think that is wrong. Because right now, it directly returns the passed in path (which is a *system path*), which returns in wrong redirections, so you end up on my/path/my/path on form submits, ignoring aliases and so on. Having to manually convert all calls to use url() would be a big DX problem and it would also make it close to impossible to actually alter a redirect URL. I think Response needs to accept and support these options and explicitly work with internal path internally and only convert it to the external path right before the header is set. Maybe we can use the new url generator stuff for this?
- We need a way to set the response somewhere, e.g. on the kernel or a special service/subscriber. That might not be nice and clean but there is *no* way a return RedirectResponse() in a form builder function is ever going to work and that is exactly what this patch is currently doing.
* Haven't read the issue comments, not sure how much of this has been discussed already.
Comment #72
Crell CreditAttribution: Crell commentedI brainstormed a bit with Berdir in IRC. Suggestion:
Add a _drupal_register_redirect(RedirectResponse $response) function. Document it as a last-resort and for internal use only. Then add a response listener (or maybe just view listener?) that checks for the presence of a registered redirect response and swaps that in as the response object to send. Ugly, but in some cases (like FAPI) it may be all we can do. Deeper changes to those systems to eliminate that hack would come in Drupal 9. Client code though we say *must* return RedirectResponse directly.
Comment #73
BerdirTwo more points from that brainstorming.
- In general, redirect logic does not belong into form builder functions and other deeply nested functions but page callbacks. The example from simpletest_result_form() is easy to fix by adding a page callback that does that check and afterwards calls the form. Others might not be that easy. That's where the mentioned drupal_register_redirect() function comes in, allowing to remove drupal_goto() (which I'm quite sure is necessary to remove drupal_exit(), for which we have a critical task), possibly + a follow-up issue to take care of as much uses of that function as possible in 8.x.
- The ResponseListener is responsible for normalizing the URL (which currently means, calling url()), which also means that RedirectResponse() needs to support $options.
I think this should allow to make progress again this issue. I won't have time tomorrow, but possibly on the weekend.
Comment #74
BerdirMade some hackish progress, this gets batch API working, we will need our own redirect response that supports options I think. Using the array there results in notices.
Also fixed the simpletest test results page.
The interactive installer is still broken. It's the same problem (batch_process() not redirecting to the batch page) but complicated by the fact that we're in early bootstrap mode and want to use a custom page page.
Tried various things like returning the response, using a custom redirect callback but it is complicated by the fact that the installer passes in an URL that looks like core/install.php?langcode=en&profile=standard and expects that the batch api arguments are added to the existing ones, which somehow does not happen. Using install_goto() including adding support for $options allowed me to reach the batch processing page but then something encoded the url and it failed to execute the js request.
Comment #76
g.oechsler CreditAttribution: g.oechsler commentedThis is the patch from #74 applied and "ported" to HEAD. There were lots of reject, so I hope I haven't messed it up.
Comment #78
tim.plunkettThis is likely the "best practice", but as I was trying to use RedirectResponse today, I was thinking maybe we should just subclass it in \Drupal\Core.
Comment #79
Crell CreditAttribution: Crell commentedtim: I don't know what you mean...
Comment #80
neclimdulRe-roll and a couple hacks to get this installing. Rough conceptual interdiff minus fixing merge changes attached as well. Its hacky but should help get through the roadblock.
Comment #82
BerdirThanks!
The attached patch introduces a proof of concept drupal_set_redirect() helper function to solve the problem of getting the redirect out of places like form builder functions and direct handling. A lot of tests above failed because drupal_get_form() was used within a render array and that totally broke stuff when it suddenly returned an object.
Also removed a leading / that resulted in a direct path like http://domain//path/to/something, breaking some getUrl() assertions.
Comment #84
corvus_ch CreditAttribution: corvus_ch commentedComment #85
corvus_ch CreditAttribution: corvus_ch commentedRerolled patch against latest dev and removed the double url encoding.
Comment #86
corvus_ch CreditAttribution: corvus_ch commentedFixed form redirect.
Comment #87
disasm CreditAttribution: disasm commentedIt would be nice to add a little more to this doctag. Specifically what happens when the function is called when $response == NULL.
should this be replaced with @see RedirectResponse instead of just removed?
why are we replacing a CMI config with a variable_get here?
should this be new RedirectResponse('');
Why are we replacing CMI config with variable_get here?
Why are we replacing CMI config with variable_get here?
ok, this explains why CMI config was being replaced with variable_get. It looks like you didn't rebase after something was committed.
Comment #88
ParisLiakos CreditAttribution: ParisLiakos commentedComment #89
corvus_ch CreditAttribution: corvus_ch commentedSo here is patch that adresses the not happening redirect in from redirect.
@disasm: The reverte CMI stuff should not be in the patch any more. As of your comments addressing improvements. It is certainly too early for them. We should consider them when the tests are passing agin.
Comment #91
damiankloip CreditAttribution: damiankloip commentedRerolled. Having a look at this today.
Comment #92
damiankloip CreditAttribution: damiankloip commentedComment #94
damiankloip CreditAttribution: damiankloip commentedI'm confused by this now...
Comment #95
disasm CreditAttribution: disasm commented#91: 1668866-91.patch queued for re-testing.
Comment #96
disasm CreditAttribution: disasm commentedretesting. Install was fine manually for me. Logged in as admin user after install, and was able to enable testing module.
Comment #98
disasm CreditAttribution: disasm commentedreroll. Also, two more drupal_goto functions found using git grep. interdiff attached.
Comment #99
disasm CreditAttribution: disasm commentedComment #101
disasm CreditAttribution: disasm commentedok, I was able to reproduce. I thought this error meant it couldn't login to drupal after installation, but what it really is saying is it can't run the tests. On my local installation this manifests itself as redisplaying the list of tests to run after clicking run tests with no error message.
Comment #102
tim.plunkettI know elsewhere when using RedirectResponse, we do:
return new RedirectResponse(url('admin/structure/config_test', array('absolute' => TRUE)));
Comment #103
Crell CreditAttribution: Crell commentedI think this function is about to die anyway in favor of the generator.
Comment #104
dawehnerAssign myself for rerolling
Comment #105
dawehnerIt's not working perfect yet (for example the comment tests break), though there are a couple of fixes in this patch.
Comment #106
ParisLiakos CreditAttribution: ParisLiakos commentedComment #108
ParisLiakos CreditAttribution: ParisLiakos commentedLets try now with the ResponseSubscriber actually registered:)
Comment #110
ParisLiakos CreditAttribution: ParisLiakos commentedyeah, well, this trick with drupal_set_redirect is not gonna work, cause the view kernel event only fires if the controller does *not* return a Response object. and thats what happens on HtmlFormController::content()
So with view not firing, someone can use the response event..but swapping response events leads to some nice results, eg lost messages from dsm..not to mention that redirection happens after drupal has theme the entire page..
well i am really not sure how to make $form_state['redirect'] work with RedirectResponse
Comment #112
ParisLiakos CreditAttribution: ParisLiakos commentedand without the experimental line:P
Comment #114
ParisLiakos CreditAttribution: ParisLiakos commentedyeah, actually the solution is easy..getting some air actually helps sometimes
now, lets see this
Comment #116
BerdirI was wondering if we want to split this issue up and first get the subscriber and other things in that make it possible to actually return redirect responses and then do the conversions in smaller issues that are easier to review. I feel like we're moving in circles here and aren't really getting anywhere.
Of course, for that we need to be somewhat sure that the code that we do get in is correct and I still have no clue about that :)
Comment #117
Crell CreditAttribution: Crell commentedParis: What about using the kernel.response event?
Or, alternately, can HtmlFormController do the check itself? We probably would still need the listener for non-page forms, but it would take care of the common case.
Comment #118
ParisLiakos CreditAttribution: ParisLiakos commentedi already use kernel.response but i cant override response objects there. It brings the problems i mentioned in #110
doing it in HtmlFormController is the only solution i can come up with thats what i did in #114
here are some improvements, i hope it will get the test failures down a bit
Comment #120
ParisLiakos CreditAttribution: ParisLiakos commentedi think Berdir is right
Right now it feels i am replacing
drupal_goto('url here')
withdrupal_set_redirect(new RedirectResponse('url here'))
and then try to figure out what of the code that executes after that doesnt break stuff.maybe we should first open a new issue to make drupal_goto a wrapper of RedirectResponse(url())->send()
and then after we have convert everything to the new routing system and form interface get rid of ->send() and perform the redirection on kernel.response
here is how my last patch looks like.
Comment #122
ParisLiakos CreditAttribution: ParisLiakos commentedlets do this the other way around #1623114: Remove drupal_exit()
Comment #123
David_Rothstein CreditAttribution: David_Rothstein commentedDoes this issue need to be bumped to critical as a result of #1911178: Remove hook_exit()? Right now, it doesn't look to me like modules have any way to respond to the end of a page request that ends in drupal_goto(), but I could be missing something.
See also: #1991008: Regression: Adding/removing a shortcut (or otherwise changing the toolbar) while inside the overlay doesn't update the toolbar
Comment #124
ParisLiakos CreditAttribution: ParisLiakos commentedit definitely feels critical, we cant ship with half core using RedirectResponse and other half drupal_goto.
as per #122
Comment #125
ParisLiakos CreditAttribution: ParisLiakos commentednow that #1998228: Remove hook_menu_site_status_alter() in favor of request listeners is in, this got even easier, the hard part remains the form API, but thats almost solved already
Comment #126
ParisLiakos CreditAttribution: ParisLiakos commentedjust checking..i know it will fail, but lets see how badly
Comment #128
ParisLiakos CreditAttribution: ParisLiakos commentedI tried to avoid completely
drupal_set_redirect()
It all worked, when i got to the point to run some action tests...well, the problem is with $forms that live in render arrays..
So, any form like that:
is not going to work, cause the RedirectResponse is hidden inside the array.
An example of how i fixed this for and instance in action module:
https://privatepaste.com/7a8eaffe2c
So, i brought back drupal_set_redirect()
But that does not fix our problems...forms that need to return lets say a different kind of responses..eg a file response, like locale's translation export form does..well they still have no way to work with the kernel. They will just
send()
the response breaking the whole flow.Not to mention that HtmlFormController has changed a bit since last time i was there, it now returns an array not a response..
But in the end, why go into all the trouble to replace one hack (
drupal_goto
) with another (drupal_set_(redirect|file|json..)response
)The only way i can see past that, like i said @timplunkett in irc is using sub-requests to embed forms in render arrays...but it is just a hope, i have nothing solid there, i dont event know whether that would work
Comment #129
ParisLiakos CreditAttribution: ParisLiakos commentedthis will take care some exceptions
Comment #131
Crell CreditAttribution: Crell commentedParis and I discussed this in IRC a bit, and have a revised plan forward:
The root problem here is that FAPI is just fundamentally incompatible with the single-exit-point approach that Drupal 8 is (trying to be) built upon. Trying to make it so is not working. To make it work we'd have to redesign FAPI and Render API... which is so not on the table right now it's not even funny. :-)
Instead, we're going to accept that there will be some fugly to make this work but it will at least be contained to FAPI. That way we can take a sledge-hammer approach to FAPI in Drupal 9, which would fix this and various other related issues.
To that end, drupal_goto() will be removed and replaced with _drupal_form_send_response(). _dfsr() will:
* Take a response object passed to it by FAPI (usually either a RedirectResponse or BinaryFileResponse)
* Call the kernel.response event on it directly via Drupal::
* prepare() and send() the response
* Call Drupal::service('http_kernel')->terminate()
* PHP exit;
That will allow FAPI to still bail in the middle of the request, which is what it has to do, but still keep the response going through the kernel.response event and still keep kernel.terminate firing to do end-of-process cleanup. All other uses of drupal_goto() will be replaced with a proper response object return.
_drupal_form_send_response() will also be documented to explain that it is a hack and should never, ever, ever be called except by FAPI. We probably should go as far as marking it @deprecated now, and document that it is only a shiv for FAPI and may be removed without notice if we ever figure out a better way. (We probably won't until D9, but we don't want people to think that it's just a renamed drupal_goto() and try using it themselves.)
Comment #132
ParisLiakos CreditAttribution: ParisLiakos commentedlets see where this get us
Comment #134
ParisLiakos CreditAttribution: ParisLiakos commentedComment #136
ParisLiakos CreditAttribution: ParisLiakos commentedthis should fix upgrade path tests and some entity translation ones
Comment #137
Crell CreditAttribution: Crell commentedDamnit, Drupal!
This docblock still refers to "Function arguments". ALso, I don't know what rget is. :-)
Why is this not injectable? And... setting a value on a service is bad news. Services should be read-only.
Also, we really should start using the generator rather than url() for classes.
Comment #138
ParisLiakos CreditAttribution: ParisLiakos commentedi removed two files that are not in the interdiff..the action event listener and action.services.yml, no more needed
Comment #139
ParisLiakos CreditAttribution: ParisLiakos commentedlets fix overlay and session_test and see whether something will break with the changes i did in
system_authorized_batch_processing_url
Comment #141
ParisLiakos CreditAttribution: ParisLiakos commentedComment #142
aspilicious CreditAttribution: aspilicious commented+ // If the form returns some kind of response, deliver it.
+ if ($form instanceof Response) {
+ _drupal_form_send_response($form);
+ }
This so feels like the things I tried a few months ago. (see earlier patches) I REALY hope this new approach will get this done :s
Comment #148
ParisLiakos CreditAttribution: ParisLiakos commentedthis should be green
Comment #150
ParisLiakos CreditAttribution: ParisLiakos commented#148: drupal-goto_respone-1668866-148.patch queued for re-testing.
Comment #151
tim.plunkettWe can't commit it with this comment. I'd much like to get this in though, let's fix this.
Comment #152
cweagansWell, obviously. It's over the 80 character limit ;)
Comment #153
ParisLiakos CreditAttribution: ParisLiakos commentedlol
poor dwarfs:(
Comment #155
tim.plunkett#153: drupal-goto_response-1668866-153.patch queued for re-testing.
Comment #157
tim.plunkett#153: drupal-goto_response-1668866-153.patch queued for re-testing.
Comment #159
ParisLiakos CreditAttribution: ParisLiakos commentedsince i am rerolling, here is with the url_generator injected
Comment #161
chx CreditAttribution: chx commentedsniff. if we can't have dwarf ninjas what about some Jedi Ninjas?
Comment #162
ParisLiakos CreditAttribution: ParisLiakos commentedgit stash failed me..forgot RedirectResponseSubscriber.php in patch above
@chx: hilarious video:P
Comment #163
pdrake CreditAttribution: pdrake commentedWe should not be perpetuating the use of exit in core.
Comment #164
ParisLiakos CreditAttribution: ParisLiakos commentedpls read #131
Comment #165
pdrake CreditAttribution: pdrake commentedI have read #131 and I disagree with continuing the use of exit D8.
Comment #166
tim.plunkettPlease propose another solution then.
Comment #167
ParisLiakos CreditAttribution: ParisLiakos commentedreroll
(removed also an instance of drupal_goto in update.inc's update_bach default arguments)
Comment #169
tim.plunkett#167: drupal-goto_response-1668866-167.patch queued for re-testing.
Comment #171
ParisLiakos CreditAttribution: ParisLiakos commented#167: drupal-goto_response-1668866-167.patch queued for re-testing.
Comment #172
tim.plunkettLooks great, and is green!
Let's finish this one off.
Comment #173
Crell CreditAttribution: Crell commentedThis gets a +1 from me as well. ParisLiakos++
Comment #174
ParisLiakos CreditAttribution: ParisLiakos commentedreroll
Comment #175
catchThe array('absolute' => TRUE) is going to be easily forgotten and it's very repetitive. I realise this is what RedirectResponse requires, but we could subclass it something.
Overall it's taking us away from (mainly) system paths, to complete URL strings from url() - which is the opposite of the convenience that the generator is supposed to add in terms of decoupling routes from paths.
It's more obvious with this hunk:
In this case I want to redirect to the system path for the node, not the absolute URL the node happens to be at - the system should handle that for me.
Not particularly concerned about the FAPI hacks. Ages ago there was talk of having forms submit to a dedicated route, that'd be an internal change so feels still on the table for 8.x (would be a big performance improvement most likely since no more wasted page render). If we did that it feels like it might be possible to remove the hack, although that's probably over-optimistic.
Comment #176
ParisLiakos CreditAttribution: ParisLiakos commentedi prefer introducing a method to url generator...maybe
generateAbsolute
than subclassing RedirectResponsebut we already follow this pattern in core..we can do this in a followup
Comment #177
alexpott@Parisliakos, @catch, @timplunkett discussed this is IRC and decided to bikeshed the DX after the patch has been committed in a followup.
Comment #178
alexpottThe followup issue... #2023445: Improve DX of doing a redirect
Comment #179
catchCommitted/pushed to 8.x, thanks!
Needs a change notice for now, even if we change the change notice.
Comment #180
ParisLiakos CreditAttribution: ParisLiakos commentedchange notice here https://drupal.org/node/2023537
i linked the DX issue so we can get some feedback there
Comment #181
ParisLiakos CreditAttribution: ParisLiakos commentedComment #182
Crell CreditAttribution: Crell commentedChange notice looks good. Thanks, Paris!
Comment #183
Dave ReidThere is a regression in hook_drupal_goto_alter(). In Drupal 7 the alter hook runs *before* the call to url() is made to construct the final URL. Now my chance to alter the URL is only after the absolute URL has been constructed and we lose all context of $path and $options. :(
Comment #184
Dave ReidDo we need to open a follow-up issue for performance? Before, in hook_drupal_goto_alter() the code would only get executed when a redirect is called. Now, we have to add a Kernel listener that runs and does on every single request.
Comment #185
ParisLiakos CreditAttribution: ParisLiakos commentedwe could fire an event from the base controller that we talk about in #2023445: Improve DX of doing a redirect passing the same parameters, which would make the performance argument obsolete too
Comment #187
joachim CreditAttribution: joachim commentedThe change request is missing an example of how do the equivalent of drupal_goto() with no parameter, to cause a reload of the same page.
Comment #188
tim.plunkettUpdated. Next time use the right title, status, and tags. Or just update it yourself (I didn't know the answer, I had to do research).
Comment #189
Dave ReidI don't see how the updated change notice includes redirecting to the homepage. I'll try and look it up and update it, but having to remove the back to active for now.
Comment #190
tim.plunkettHe asked for same page, not the home page. I added that too.
Next time someone opens this, please tag it "needs change notification" and update the title accordingly.
Comment #191
SiliconMind CreditAttribution: SiliconMind commentedThis is great yet sad example of why Perfect is the enemy of good. In D7 the only thing that was needed in order to alter a redirection, was to implement a hook. A three liner solution. In D8 two files are needed and a bunch of "use" calls that take more lines than the actual implementation itself :(
Who the hell will memorize this?