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.

Files: 
CommentFileSizeAuthor
#174 drupal-goto_response-1668866-174.patch71.95 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 57,959 pass(es). View
#167 drupal-goto_response-1668866-167.patch71.94 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,170 pass(es). View
#162 drupal-goto_response-1668866-162.patch71.39 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,045 pass(es). View
#159 drupal-goto_response-1668866-159.patch68.41 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#159 interdiff.txt6.09 KBParisLiakos
#153 drupal-goto_response-1668866-153.patch67.95 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-goto_response-1668866-153.patch. Unable to apply patch. See the log in the details link for more information. View
#153 interdiff.txt575 bytesParisLiakos
#148 drupal-goto_respone-1668866-148.patch67.92 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 57,884 pass(es). View
#148 interdiff.txt2.7 KBParisLiakos
#141 drupal-goto_respone-1668866-141.patch66.78 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 57,552 pass(es), 9 fail(s), and 3 exception(s). View
#141 interdiff.txt4.43 KBParisLiakos
#139 drupal-goto_respone-1668866-139.patch65.41 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 57,564 pass(es), 126 fail(s), and 102 exception(s). View
#139 interdiff.txt7.92 KBParisLiakos
#138 drupal-goto_respone-1668866-138.patch58.98 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 55,954 pass(es), 129 fail(s), and 102 exception(s). View
#138 interdiff.txt6.71 KBParisLiakos
#136 drupal-goto_respone-1668866-136.patch56.45 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 55,669 pass(es), 231 fail(s), and 121 exception(s). View
#136 interdiff.txt2.84 KBParisLiakos
#134 drupal-goto_respone-1668866-134.patch54.26 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 54,894 pass(es), 323 fail(s), and 233 exception(s). View
#134 interdiff.txt946 bytesParisLiakos
#132 drupal-goto_respone-1668866-132.patch49.23 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 53,524 pass(es), 626 fail(s), and 303 exception(s). View
#129 drupal-goto_respone-1668866-129.patch54.46 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 53,999 pass(es), 1,586 fail(s), and 682 exception(s). View
#126 drupal-goto_respone-1668866-126.patch54.39 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 54,031 pass(es), 1,572 fail(s), and 1,019 exception(s). View
#120 drupal-redirectResponse-1668866-120.patch74.72 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 53,243 pass(es), 656 fail(s), and 253 exception(s). View
#118 drupal-redirectResponse-1668866-118.patch73.91 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 51,220 pass(es), 2,484 fail(s), and 347 exception(s). View
#114 drupal-redirect_response-1668866-114.patch69.57 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 50,032 pass(es), 3,015 fail(s), and 503 exception(s). View
#114 interdiff.txt2.38 KBParisLiakos
#112 drupal-redirect_response-1668866-112.patch69.11 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#112 interdiff.txt678 bytesParisLiakos
#110 drupal-redirect_response-1668866-110.patch69.77 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#110 interdiff.txt8.28 KBParisLiakos
#108 drupal-redirect_response-1668866-108.patch69.55 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 52,402 pass(es), 1,683 fail(s), and 377 exception(s). View
#108 interdiff.txt2.87 KBParisLiakos
#105 drupal-1668866-105.patch68.88 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View
#98 drupal-replace_drupal_goto-1668866-98.patch69.05 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View
#98 interdiff.txt1.55 KBdisasm
#91 1668866-91.patch67.81 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1668866-91.patch. Unable to apply patch. See the log in the details link for more information. View
#89 1668866-89.patch69.06 KBcorvus_ch
FAILED: [[SimpleTest]]: [MySQL] 49,734 pass(es), 1,854 fail(s), and 441 exception(s). View
#89 1668866-89-interdiff.txt8.58 KBcorvus_ch
#86 1668866-86.patch75.69 KBcorvus_ch
FAILED: [[SimpleTest]]: [MySQL] 49,329 pass(es), 2,021 fail(s), and 448 exception(s). View
#85 1668866-85.patch68.65 KBcorvus_ch
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#82 goto-redirect-1668866-82.patch71.15 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 45,515 pass(es), 1,598 fail(s), and 420 exception(s). View
#82 goto-redirect-1668866-82-interdiff.txt4.92 KBBerdir
#80 interdiff.txt2.71 KBneclimdul
#80 1668866-goto.patch69.24 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] 46,052 pass(es), 1,036 fail(s), and 709 exception(s). View
#76 1668866-goto.patch67.31 KBg.oechsler
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#74 replace-goto-with-redirect-response-1668866-74.patch67.17 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#74 replace-goto-with-redirect-response-1668866-74-interdiff.txt7.64 KBBerdir
#71 replace-goto-with-redirect-response-1668866-71.patch63.97 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch replace-goto-with-redirect-response-1668866-71.patch. Unable to apply patch. See the log in the details link for more information. View
#71 replace-goto-with-redirect-response-1668866-71-interdiff.txt4.46 KBBerdir
#65 drupal-replace_drupal_goto-1668866-65.patch63.64 KBpdrake
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#55 drupal_goto_55.patch40.46 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 39,990 pass(es), 176 fail(s), and 78 exception(s). View
#52 interdiff.txt5.24 KBtim.plunkett
#51 drupal_goto_51.patch40.46 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 39,992 pass(es), 178 fail(s), and 81 exception(s). View
#47 drupal_goto_47.patch39.83 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 40,019 pass(es), 111 fail(s), and 27 exception(s). View
#44 drupal_goto_43.patch39.47 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 39,971 pass(es), 127 fail(s), and 27 exception(s). View
#40 drupal_goto_40.patch38.6 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 39,964 pass(es), 112 fail(s), and 27 exception(s). View
#38 drupal_goto_38.patch36.64 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 39,491 pass(es), 303 fail(s), and 8,705 exception(s). View
#35 drupal_goto_listener_36.patch38.59 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 39,427 pass(es), 298 fail(s), and 8,761 exception(s). View
#34 drupal_goto_listener_34.patch38.52 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 38,171 pass(es), 1,820 fail(s), and 8,520 exception(s). View
#31 drupal_goto_listener.patch36.64 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View
#15 drupal_goto.patch36.38 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 37,083 pass(es), 23 fail(s), and 9 exception(s). View
#10 remove-goto-no-form.patch33.48 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 37,040 pass(es), 56 fail(s), and 12 exception(s). View
#5 remove-drupal-goto.patch40.73 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed attempting to get list of tests from run-tests.sh. View
#2 remove-drupal-goto.patch92.08 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Comments

Crell’s picture

Title: Rewrite drupal_goto() to use RedirectResponse » Replace drupal_goto() with RedirectResponse
Issue tags: +WSCCI, +symfony

Tagging.

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.

marcingy’s picture

Status: Active » Needs review
FileSize
92.08 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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

Status: Needs review » Needs work

The last submitted patch, remove-drupal-goto.patch, failed testing.

Crell’s picture

+++ b/core/includes/form.inc
@@ -186,10 +187,10 @@ function drupal_get_form($form_id) {
+ *   - no_redirect: If set to TRUE the form will NOT perform a
+ *     RedirectResponse(), even if 'redirect' is set.

RedirectResponse isn't really performed; it's more "returned", or maybe "issued".

+++ b/core/includes/form.inc
@@ -1251,21 +1252,26 @@ function drupal_redirect_form($form_state) {
-        call_user_func_array('drupal_goto', $form_state['redirect']);
+        $reflect = new ReflectionClass('RedirectResponse');
+        return $reflect->newInstanceArgs($form_state['redirect']);

This very much worries me. Why can't we just return new RedirectRepsonse, as we do a few lines down?

+++ b/core/includes/form.inc
@@ -1251,21 +1252,26 @@ function drupal_redirect_form($form_state) {
-    drupal_goto(current_path(), array('query' => request()->query->all()));
+    return new RedirectResponse(current_path(), array('query' => request()->query->all()));

Remember that current_path() is going away, hopefully, so we shouldn't continue to rely on it in new code.

+++ b/core/includes/form.inc
@@ -4870,8 +4877,14 @@ function batch_process($redirect = NULL, $url = 'batch', $redirect_callback = 'd
-      $function = $batch['redirect_callback'];
-      $function($batch['url'], array('query' => array('op' => 'start', 'id' => $batch['id'])));
+      // @todo for some reason this results in batches breaking.
+      if ($batch['redirect_callback'] == 'drupal_goto') {
+        return new RedirectResponse($batch['url'], array('query' => array('op' => 'start', 'id' => $batch['id'])));
+      }
+      else {
+        $function = $batch['redirect_callback'];
+        $function($batch['url'], array('query' => array('op' => 'start', 'id' => $batch['id'])));
+      }

Can we replace everything but this for now to get testbot working, then fix this part?

+++ b/core/lib/Drupal/Core/Utility/RedirectResponse.php
@@ -0,0 +1,80 @@
+   * @param $options
+   *   (optional) An associative array of additional URL options to pass to ¶
+   *   url().

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.

+++ b/core/lib/Drupal/Core/Utility/RedirectResponse.php
@@ -0,0 +1,80 @@
+    $destination = request()->query->get('destination', '');

request() is also temporary, so should not be relied upon.

+++ b/core/modules/comment/comment.pages.inc
@@ -126,7 +127,7 @@ function comment_approve($cid) {
diff --git a/core/modules/field/modules/text/text.module b/core/modules/field/modules/text/text.module

diff --git a/core/modules/field/modules/text/text.module b/core/modules/field/modules/text/text.module
index dcf4d1e..2d6aa50 100644

index dcf4d1e..2d6aa50 100644
--- a/core/modules/field/modules/text/text.module

--- a/core/modules/field/modules/text/text.module
+++ b/core/modules/field/modules/text/text.module

+++ b/core/modules/field/modules/text/text.module
+++ b/core/modules/field/modules/text/text.module
@@ -72,16 +72,26 @@ function text_field_settings_form($field, $instance, $has_data) {

@@ -72,16 +72,26 @@ function text_field_settings_form($field, $instance, $has_data) {
       '#required' => TRUE,
       '#description' => t('The maximum length of the field in characters.'),
       '#min' => 1,
-      // @todo: If $has_data, add a validate handler that only allows
-      // max_length to increase.
-      '#disabled' => $has_data,
     );
+    // If the field has data we only allow length to be increased.
+    if ($has_data) {
+      $form['max_length']['#element_validate'] = array('text_field_setting_max_length_validate');
+    }
   }
 
   return $form;
 }
 
 /**
+ * Form element validation handler to check the length of text fields.
+ */
+function text_field_setting_max_length_validate($element, &$form_state) {
+  if ($element['#value'] < $element['#default_value']) {
+    form_error($element, t('%name cannot be decreased in size.', array('%name' => $element['#title'])));
+  }
+}
+

Why is this here? Looks like flotsam from another patch.

+++ b/core/modules/user/user.pages.inc
@@ -482,7 +483,7 @@ function user_cancel_confirm($account, $timestamp = 0, $hashed_pass = '') {
diff --git a/remove-drupal-goto.patch b/remove-drupal-goto.patch

diff --git a/remove-drupal-goto.patch b/remove-drupal-goto.patch
new file mode 100644
index 0000000..e7803c0

index 0000000..e7803c0
--- /dev/null

--- /dev/null
+++ b/remove-drupal-goto.patch

+++ b/remove-drupal-goto.patch
+++ b/remove-drupal-goto.patch
@@ -0,0 +1,1156 @@

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.

marcingy’s picture

Status: Needs work » Needs review
FileSize
40.73 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed attempting to get list of tests from run-tests.sh. View

Re-roll reverting the batch changes. I have tested locally and drupal still installs so hopefully this will work on d.o as well.

+++ b/core/includes/form.inc
@@ -1251,21 +1252,26 @@ function drupal_redirect_form($form_state) {
-        call_user_func_array('drupal_goto', $form_state['redirect']);
+        $reflect = new ReflectionClass('RedirectResponse');
+        return $reflect->newInstanceArgs($form_state['redirect']);

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

Status: Needs review » Needs work

The last submitted patch, remove-drupal-goto.patch, failed testing.

marcingy’s picture

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

@@ -511,7 +511,9 @@ function _batch_finished() {
     }
 
     // Use drupal_redirect_form() to handle the redirection logic.
-    drupal_redirect_form($_batch['form_state']);
+    if ($redirect = drupal_redirect_form($_batch['form_state'])) {
+      return $redirect;
+    }
@@ -908,8 +908,10 @@ function drupal_process_form($form_id, &$form, &$form_state) {
       // Set a flag to indicate the the form has been processed and executed.
       $form_state['executed'] = TRUE;
 
-      // Redirect the form based on values in $form_state.
-      drupal_redirect_form($form_state);
+      // Use drupal_redirect_form() to handle the redirection logic.
+      if ($redirect = drupal_redirect_form($form_state)) {
+        return $redirect;
+      }
     }
chx’s picture

        $reflect = new ReflectionClass('RedirectResponse');
        return $reflect->newInstanceArgs($form_state['redirect']);

makes me run screaming. Is this still necessary in 5.3? Isn't there a nicer way now? I don't know, I hope.

marcingy’s picture

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

marcingy’s picture

Status: Needs work » Needs review
FileSize
33.48 KB
FAILED: [[SimpleTest]]: [MySQL] 37,040 pass(es), 56 fail(s), and 12 exception(s). View

Re-roll without the form changes for now se comments above re the issues.

Status: Needs review » Needs work

The last submitted patch, remove-goto-no-form.patch, failed testing.

marcingy’s picture

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

marcingy’s picture

So something kind of like this which removes the test failures for languages

@@ -341,6 +342,11 @@ function drupal_build_form($form_id, &$form_state) {
     }
 
     $form = drupal_retrieve_form($form_id, $form_state);
+    if (!empty($form_state['redirectresponse'])) {
+      $redirect = $form_state['redirectresponse'];
+      unset($form_state['redirectresponse']);
+      return new RedirectResponse($redirect);
+    }
@@ -391,7 +392,8 @@ function language_admin_delete_form($form, &$form_state, $language) {
 
   if (language_default()->langcode == $langcode) {
     drupal_set_message(t('The default language cannot be deleted.'));
-    drupal_goto('admin/config/regional/language');
+    $form_state['redirectresponse'] = 'admin/config/regional/language';
+    return;
   }
Crell’s picture

Can'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?

marcingy’s picture

Status: Needs work » Needs review
FileSize
36.38 KB
FAILED: [[SimpleTest]]: [MySQL] 37,083 pass(es), 23 fail(s), and 9 exception(s). View

Some progress with tests there are still broken items but we are getting there. Setting to needs review for the bot.

Status: Needs review » Needs work

The last submitted patch, drupal_goto.patch, failed testing.

Crell’s picture

+++ b/core/includes/form.inc
@@ -454,6 +459,9 @@ function form_state_defaults() {
+  if (is_object($form) && is_a($form, 'Drupal\Core\Utility\RedirectResponse')) {

This should be done with instancreof, not is_a(). (instanceof is a language construct so is a wee bit faster.)

Crell’s picture

I'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.

marcingy’s picture

I don't have a branch set up for it at the moment just the patch that is here.

Crell’s picture

I'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.

Dave Reid’s picture

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

Crell’s picture

Commented on the other issue. Short version, all responses get Http cached, rather than "page" cached.

sun’s picture

Issue tags: +API clean-up

Tagging.

Crell’s picture

Well 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!

aspilicious’s picture

I'm confused can someone expplain what has to be done here?
The symfony stuff is in btw :)

Crell’s picture

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

marcingy’s picture

So I have attempted to update the patch to utilise a subscriber which responses to the following

$events[KernelEvents::RESPONSE][] = array('onKernelResponse', 100);

Now this all well and good the listener fires but.....we then get

Fatal error: Call to undefined method Symfony\Component\HttpFoundation\Response::SetTargetUrl() 

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

aspilicious’s picture

Can you upload a patch?

tim.plunkett’s picture

Should be setTargetUrl not SetTargetUrl

marcingy’s picture

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

marcingy’s picture

FileSize
36.64 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

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

tim.plunkett’s picture

Status: Needs work » Needs review

Well I'd rather see where it fails, so I can start debugging in the right place :)

aspilicious’s picture

Status: Needs review » Needs work

Well, 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 :)

aspilicious’s picture

Status: Needs work » Needs review
FileSize
38.52 KB
FAILED: [[SimpleTest]]: [MySQL] 38,171 pass(es), 1,820 fail(s), and 8,520 exception(s). View

This works for certain paths. Not yet the user login stuff...

aspilicious’s picture

FileSize
38.59 KB
FAILED: [[SimpleTest]]: [MySQL] 39,427 pass(es), 298 fail(s), and 8,761 exception(s). View

this maybe can work...

aspilicious’s picture

Since the beginning of the kernel there was an ugly redirect problem in overlay when clearing caches. This fixes this too somehow :)
Nice

Status: Needs review » Needs work

The last submitted patch, drupal_goto_listener_36.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
36.64 KB
FAILED: [[SimpleTest]]: [MySQL] 39,491 pass(es), 303 fail(s), and 8,705 exception(s). View

This should fix a number of exceptions

Status: Needs review » Needs work

The last submitted patch, drupal_goto_38.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
38.6 KB
FAILED: [[SimpleTest]]: [MySQL] 39,964 pass(es), 112 fail(s), and 27 exception(s). View

Status: Needs review » Needs work

The last submitted patch, drupal_goto_40.patch, failed testing.

aspilicious’s picture

At least some (I think most) of those test failures are cause by "broken" tests.

drupalLogout:

// Make a request to the logout page, and redirect to the user page, the
// idea being if you were properly logged out you should be seeing a login
// screen.
$this->drupalGet('user/logout', array('query' => array('destination' => 'user')));

We should incoporate a RedirectResponse somewhere in WebTest....
Not sure how to do that... :(

EDIT: maybe they are real those fails... *digs deeper*

tim.plunkett’s picture

If you read through the original issue linked in the OP, you'll need to change the 'destination' handling directly.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
39.47 KB
FAILED: [[SimpleTest]]: [MySQL] 39,971 pass(es), 127 fail(s), and 27 exception(s). View

Hmm, "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

Status: Needs review » Needs work

The last submitted patch, drupal_goto_43.patch, failed testing.

aspilicious’s picture

I'm working on a patch that should work with destination responses...

aspilicious’s picture

Status: Needs work » Needs review
FileSize
39.83 KB
FAILED: [[SimpleTest]]: [MySQL] 40,019 pass(es), 111 fail(s), and 27 exception(s). View

This should hopefully fix some tests :)

Status: Needs review » Needs work

The last submitted patch, drupal_goto_47.patch, failed testing.

aspilicious’s picture

Aha! 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...

aspilicious’s picture

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

aspilicious’s picture

Status: Needs work » Needs review
FileSize
40.46 KB
FAILED: [[SimpleTest]]: [MySQL] 39,992 pass(es), 178 fail(s), and 81 exception(s). View

Succes

tim.plunkett’s picture

FileSize
5.24 KB

Here's the interdiff for those that were curious, like me.

Status: Needs review » Needs work

The last submitted patch, drupal_goto_51.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

Ah crap...

aspilicious’s picture

FileSize
40.46 KB
FAILED: [[SimpleTest]]: [MySQL] 39,990 pass(es), 176 fail(s), and 78 exception(s). View

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

aspilicious’s picture

changes

+ 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')));

Status: Needs review » Needs work

The last submitted patch, drupal_goto_55.patch, failed testing.

Crell’s picture

+++ b/core/includes/form.inc
@@ -1273,7 +1283,7 @@ function drupal_redirect_form($form_state) {
-    drupal_goto(current_path(), array('query' => drupal_container()->get('request')->query->all()));
+    return new RedirectResponse(current_path(), 302, array('query' => drupal_container()->get('request')->query->all()));

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

+++ b/core/lib/Drupal/Core/EventSubscriber/ResponseSubscriber.php
@@ -0,0 +1,78 @@
+    $is_redirect = is_a($response, 'Symfony\Component\HttpFoundation\RedirectResponse');

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.

+++ b/core/lib/Drupal/Core/EventSubscriber/ResponseSubscriber.php
@@ -0,0 +1,78 @@
+      if ($is_redirect) {
+        $response->setTargetUrl($url);
+      }
+      // In case of a normal response create a new redirectResponse
+      else {
+        $event->setResponse(new RedirectResponse($url));
+      }

I don't understand this at all. If we're not redirecting, force us to redirect? That doesn't make any sense to me...

+++ b/core/modules/openid/openid.module
@@ -741,7 +743,7 @@ function openid_authentication($response) {
-      drupal_goto();
+      return new RedirectResponse('<front>');

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.

aspilicious’s picture

I don't understand this at all. If we're not redirecting, force us to redirect? That doesn't make any sense to me...

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

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.

It's the class that doesn't accept empty paths

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.

Don't I need a real object to check if I want to use instanceof?

This still doesn't solve the form api problem....

Crell’s picture

use Symfony\Component\HttpFoundation\RedirectResponse;

$is_redirect = $response instanceof RedirectResponse;

$response is a real object.

BrockBoland’s picture

Needs issue summary

Crell’s picture

Is anyone working on this still? It still has to happen...

catch’s picture

If it has to happen, why is it not critical or at least major?

Crell’s picture

Priority: Normal » Major

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

pdrake’s picture

Status: Needs work » Needs review
FileSize
63.64 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -API clean-up, -WSCCI, -symfony

The last submitted patch, drupal-replace_drupal_goto-1668866-65.patch, failed testing.

pdrake’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +API clean-up, +WSCCI, +symfony

The last submitted patch, drupal-replace_drupal_goto-1668866-65.patch, failed testing.

aspilicious’s picture

+++ b/core/modules/openid/openid.moduleundefined
@@ -776,13 +778,13 @@ function openid_authentication($response) {
+    return new RedirectResponse('user/register', array('query' => $destination));

See below

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -133,7 +134,7 @@ function overlay_init() {
+      return new RedirectResponse('<front>', array('fragment' => 'overlay=' . $current_path));

This isn't allowed should url should be build with 'url' I guess before passing through the response.

Berdir’s picture

Assigned: Unassigned » Berdir

I'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.

Berdir’s picture

Assigned: Berdir » Unassigned
FileSize
4.46 KB
63.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch replace-goto-with-redirect-response-1668866-71.patch. Unable to apply patch. See the log in the details link for more information. View

Ok, 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.

Crell’s picture

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

Berdir’s picture

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
7.64 KB
67.17 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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

Status: Needs review » Needs work

The last submitted patch, replace-goto-with-redirect-response-1668866-74.patch, failed testing.

g.oechsler’s picture

Status: Needs work » Needs review
FileSize
67.31 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

This is the patch from #74 applied and "ported" to HEAD. There were lots of reject, so I hope I haven't messed it up.

Status: Needs review » Needs work

The last submitted patch, 1668866-goto.patch, failed testing.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/ResponseSubscriber.phpundefined
@@ -0,0 +1,71 @@
+    if ($response instanceOf RedirectResponse) {
...
+      $options['absolute'] = TRUE;

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

Crell’s picture

tim: I don't know what you mean...

neclimdul’s picture

Status: Needs work » Needs review
FileSize
69.24 KB
FAILED: [[SimpleTest]]: [MySQL] 46,052 pass(es), 1,036 fail(s), and 709 exception(s). View
2.71 KB

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

Status: Needs review » Needs work

The last submitted patch, 1668866-goto.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.92 KB
71.15 KB
FAILED: [[SimpleTest]]: [MySQL] 45,515 pass(es), 1,598 fail(s), and 420 exception(s). View

Thanks!

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.

Status: Needs review » Needs work

The last submitted patch, goto-redirect-1668866-82.patch, failed testing.

corvus_ch’s picture

Assigned: Unassigned » corvus_ch
corvus_ch’s picture

Status: Needs work » Needs review
FileSize
68.65 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Rerolled patch against latest dev and removed the double url encoding.

corvus_ch’s picture

FileSize
75.69 KB
FAILED: [[SimpleTest]]: [MySQL] 49,329 pass(es), 2,021 fail(s), and 448 exception(s). View

Fixed form redirect.

disasm’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -3479,6 +3480,22 @@ function drupal_check_memory_limit($required, $memory_limit = NULL) {
+ * Set a redirect response.
+ *
+ * This should only be used if there is no other way to return the response
+ * as the page callback.
+ *

It would be nice to add a little more to this doctag. Specifically what happens when the function is called when $response == NULL.

+++ b/core/includes/common.incundefined
@@ -582,7 +582,6 @@ function drupal_get_destination() {
- * @see drupal_goto()

should this be replaced with @see RedirectResponse instead of just removed?

+++ b/core/modules/field/lib/Drupal/field/Tests/FormTest.phpundefined
@@ -65,7 +65,7 @@ function testFieldFormSingle() {
-    $token_description = check_plain(config('system.site')->get('name')) . '_description';

why are we replacing a CMI config with a variable_get here?

+++ b/core/modules/openid/openid.moduleundefined
@@ -785,7 +787,7 @@ function openid_authentication($response) {
+      return new RedirectResponse();

should this be new RedirectResponse('');

+++ b/core/modules/user/lib/Drupal/user/Tests/UserPasswordResetTest.phpundefined
@@ -60,7 +60,7 @@ function testUserPasswordReset() {
     $this->assertMail('to', $this->account->mail, t('Password e-mail sent to user.'));
-    $subject = t('Replacement login information for @username at @site', array('@username' => $this->account->name, '@site' => config('system.site')->get('name')));
+    $subject = t('Replacement login information for @username at @site', array('@username' => $this->account->name, '@site' => variable_get('site_name', 'Drupal')));
     $this->assertMail('subject', $subject, 'Password reset e-mail subject is correct.');

Why are we replacing CMI config with variable_get here?

+++ b/core/modules/user/lib/Drupal/user/Tests/UserPasswordResetTest.phpundefined
@@ -73,7 +73,7 @@ function testUserPasswordReset() {
-    $this->assertTitle(t('@name | @site', array('@name' => $this->account->name, '@site' => config('system.site')->get('name'))), 'Logged in using password reset link.');
+    $this->assertTitle(t('@name | @site', array('@name' => $this->account->name, '@site' => variable_get('site_name', 'Drupal'))), 'Logged in using password reset link.');
 
     // Log out, and try to log in again using the same one-time link.

Why are we replacing CMI config with variable_get here?

+++ b/core/modules/views/views_ui/views_ui.moduleundefined
@@ -11,6 +11,7 @@
diff --git a/core/profiles/standard/config/editor.editor.basic_html.yml b/core/profiles/standard/config/editor.editor.basic_html.yml

diff --git a/core/profiles/standard/config/editor.editor.basic_html.yml b/core/profiles/standard/config/editor.editor.basic_html.yml
deleted file mode 100644
index e66cc2e..0000000

index e66cc2e..0000000
--- a/core/profiles/standard/config/editor.editor.basic_html.yml

--- a/core/profiles/standard/config/editor.editor.basic_html.yml
+++ /dev/nullundefined

+++ /dev/nullundefined
+++ /dev/nullundefined
@@ -1,24 +0,0 @@

@@ -1,24 +0,0 @@
-format: basic_html
snip...
diff --git a/core/profiles/standard/config/filter.format.basic_html.yml b/core/profiles/standard/config/filter.format.basic_html.yml

diff --git a/core/profiles/standard/config/filter.format.basic_html.yml b/core/profiles/standard/config/filter.format.basic_html.yml
deleted file mode 100644
index e97db8d..0000000

index e97db8d..0000000
--- a/core/profiles/standard/config/filter.format.basic_html.yml

--- a/core/profiles/standard/config/filter.format.basic_html.yml
+++ /dev/nullundefined

+++ /dev/nullundefined
+++ /dev/nullundefined
@@ -1,21 +0,0 @@

@@ -1,21 +0,0 @@
-format: basic_html
snip...
diff --git a/core/profiles/standard/config/filter.format.restricted_html.yml b/core/profiles/standard/config/filter.format.restricted_html.yml

diff --git a/core/profiles/standard/config/filter.format.restricted_html.yml b/core/profiles/standard/config/filter.format.restricted_html.yml
deleted file mode 100644
index 2e14ce2..0000000

index 2e14ce2..0000000
--- a/core/profiles/standard/config/filter.format.restricted_html.yml

--- a/core/profiles/standard/config/filter.format.restricted_html.yml
+++ /dev/nullundefined

+++ /dev/nullundefined
+++ /dev/nullundefined
@@ -1,23 +0,0 @@

@@ -1,23 +0,0 @@
-format: restricted_html
snip...

ok, this explains why CMI config was being replaced with variable_get. It looks like you didn't rebase after something was committed.

ParisLiakos’s picture

Status: Needs review » Needs work
corvus_ch’s picture

Status: Needs work » Needs review
FileSize
8.58 KB
69.06 KB
FAILED: [[SimpleTest]]: [MySQL] 49,734 pass(es), 1,854 fail(s), and 441 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 1668866-89.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
67.81 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1668866-91.patch. Unable to apply patch. See the log in the details link for more information. View

Rerolled. Having a look at this today.

damiankloip’s picture

Assigned: corvus_ch » damiankloip

Status: Needs review » Needs work

The last submitted patch, 1668866-91.patch, failed testing.

damiankloip’s picture

Assigned: damiankloip » Unassigned

I'm confused by this now...

disasm’s picture

Status: Needs work » Needs review

#91: 1668866-91.patch queued for re-testing.

disasm’s picture

retesting. Install was fine manually for me. Logged in as admin user after install, and was able to enable testing module.

Status: Needs review » Needs work

The last submitted patch, 1668866-91.patch, failed testing.

disasm’s picture

FileSize
1.55 KB
69.05 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View

reroll. Also, two more drupal_goto functions found using git grep. interdiff attached.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-replace_drupal_goto-1668866-98.patch, failed testing.

disasm’s picture

ok, 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.

tim.plunkett’s picture

I know elsewhere when using RedirectResponse, we do:
return new RedirectResponse(url('admin/structure/config_test', array('absolute' => TRUE)));

Crell’s picture

+++ b/core/includes/common.inc
@@ -541,7 +541,7 @@ function drupal_http_build_query(array $query, $parent = '') {
- * Prepares a 'destination' URL query parameter for use with drupal_goto().
+ * Prepares a 'destination' URL query parameter for use with RedirectResponse.
  *
  * Used to direct the user back to the referring page after completing a form.
  * By default the current URL is returned. If a destination exists in the
@@ -554,7 +554,7 @@ function drupal_http_build_query(array $query, $parent = '') {

I think this function is about to die anyway in favor of the generator.

dawehner’s picture

Assigned: Unassigned » dawehner

Assign myself for rerolling

dawehner’s picture

FileSize
68.88 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View

It's not working perfect yet (for example the comment tests break), though there are a couple of fixes in this patch.

ParisLiakos’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-1668866-105.patch, failed testing.

ParisLiakos’s picture

Assigned: dawehner » ParisLiakos
Status: Needs work » Needs review
FileSize
2.87 KB
69.55 KB
FAILED: [[SimpleTest]]: [MySQL] 52,402 pass(es), 1,683 fail(s), and 377 exception(s). View

Lets try now with the ResponseSubscriber actually registered:)

Status: Needs review » Needs work

The last submitted patch, drupal-redirect_response-1668866-108.patch, failed testing.

ParisLiakos’s picture

Assigned: ParisLiakos » Unassigned
Status: Needs work » Needs review
FileSize
8.28 KB
69.77 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

yeah, 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

Status: Needs review » Needs work

The last submitted patch, drupal-redirect_response-1668866-110.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
678 bytes
69.11 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

and without the experimental line:P

Status: Needs review » Needs work

The last submitted patch, drupal-redirect_response-1668866-112.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
2.38 KB
69.57 KB
FAILED: [[SimpleTest]]: [MySQL] 50,032 pass(es), 3,015 fail(s), and 503 exception(s). View

yeah, actually the solution is easy..getting some air actually helps sometimes
now, lets see this

Status: Needs review » Needs work

The last submitted patch, drupal-redirect_response-1668866-114.patch, failed testing.

Berdir’s picture

I 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 :)

Crell’s picture

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

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
73.91 KB
FAILED: [[SimpleTest]]: [MySQL] 51,220 pass(es), 2,484 fail(s), and 347 exception(s). View

i 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

Status: Needs review » Needs work

The last submitted patch, drupal-redirectResponse-1668866-118.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
74.72 KB
FAILED: [[SimpleTest]]: [MySQL] 53,243 pass(es), 656 fail(s), and 253 exception(s). View

i think Berdir is right

Right now it feels i am replacing drupal_goto('url here') with drupal_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.

Status: Needs review » Needs work

The last submitted patch, drupal-redirectResponse-1668866-120.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Postponed

lets do this the other way around #1623114: Remove drupal_exit()

David_Rothstein’s picture

Does 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

ParisLiakos’s picture

Priority: Major » Critical
Status: Postponed » Needs work

it definitely feels critical, we cant ship with half core using RedirectResponse and other half drupal_goto.
as per #122

ParisLiakos’s picture

Assigned: Unassigned » ParisLiakos

now 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

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
54.39 KB
FAILED: [[SimpleTest]]: [MySQL] 54,031 pass(es), 1,572 fail(s), and 1,019 exception(s). View

just checking..i know it will fail, but lets see how badly

Status: Needs review » Needs work

The last submitted patch, drupal-goto_respone-1668866-126.patch, failed testing.

ParisLiakos’s picture

Assigned: ParisLiakos » Unassigned

I 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:

  $build['sth'] = $render_array;
  $build['sth_else'] = drupal_get_form(..);
  return $build;

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

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
54.46 KB
FAILED: [[SimpleTest]]: [MySQL] 53,999 pass(es), 1,586 fail(s), and 682 exception(s). View

this will take care some exceptions

Status: Needs review » Needs work

The last submitted patch, drupal-goto_respone-1668866-129.patch, failed testing.

Crell’s picture

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

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
49.23 KB
FAILED: [[SimpleTest]]: [MySQL] 53,524 pass(es), 626 fail(s), and 303 exception(s). View

lets see where this get us

Status: Needs review » Needs work
Issue tags: -WSCCI, -symfony

The last submitted patch, drupal-goto_respone-1668866-132.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
946 bytes
54.26 KB
FAILED: [[SimpleTest]]: [MySQL] 54,894 pass(es), 323 fail(s), and 233 exception(s). View

Status: Needs review » Needs work

The last submitted patch, drupal-goto_respone-1668866-134.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
2.84 KB
56.45 KB
FAILED: [[SimpleTest]]: [MySQL] 55,669 pass(es), 231 fail(s), and 121 exception(s). View

this should fix upgrade path tests and some entity translation ones

Crell’s picture

Status: Needs review » Needs work
Issue tags: +WSCCI, +symfony

Damnit, Drupal!

+++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
@@ -0,0 +1,63 @@
+      // A destination in $_GET always overrides the function arguments.
+      // We do not allow absolute URLs to be passed via $_GET, as this can be an
+      // attack vector, with the following exception:
+      // - Absolute URLs that point to this site (i.e. same base URL and
+      //   base path) are allowed.rget

This docblock still refers to "Function arguments". ALso, I don't know what rget is. :-)

+++ b/core/modules/action/lib/Drupal/action/Plugin/Action/GotoAction.php
@@ -26,7 +27,8 @@ class GotoAction extends ConfigurableActionBase {
   public function execute($object = NULL) {
-    drupal_goto($this->configuration['url']);
+    $url = url($this->configuration['url'], array('absolute' => TRUE));
+    Drupal::service('action_redirect_subscriber')->setRedirect(new RedirectResponse($url));
   }

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.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
6.71 KB
58.98 KB
FAILED: [[SimpleTest]]: [MySQL] 55,954 pass(es), 129 fail(s), and 102 exception(s). View

i removed two files that are not in the interdiff..the action event listener and action.services.yml, no more needed

ParisLiakos’s picture

FileSize
7.92 KB
65.41 KB
FAILED: [[SimpleTest]]: [MySQL] 57,564 pass(es), 126 fail(s), and 102 exception(s). View

lets fix overlay and session_test and see whether something will break with the changes i did in
system_authorized_batch_processing_url

Status: Needs review » Needs work

The last submitted patch, drupal-goto_respone-1668866-139.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
4.43 KB
66.78 KB
FAILED: [[SimpleTest]]: [MySQL] 57,552 pass(es), 9 fail(s), and 3 exception(s). View
aspilicious’s picture

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

Status: Needs review » Needs work

The last submitted patch, drupal-goto_respone-1668866-141.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
2.7 KB
67.92 KB
PASSED: [[SimpleTest]]: [MySQL] 57,884 pass(es). View

this should be green

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -API clean-up, -WSCCI, -symfony

The last submitted patch, drupal-goto_respone-1668866-148.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update, +API clean-up, +WSCCI, +symfony
tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/core/includes/form.incundefined
@@ -4854,6 +4882,26 @@ function _form_set_attributes(&$element, $class = array()) {
+ * If you ever use this directly dwarf ninjas will visit you in your sleep and shave the shit out of you.

We can't commit it with this comment. I'd much like to get this in though, let's fix this.

cweagans’s picture

We can't commit it with this comment.

Well, obviously. It's over the 80 character limit ;)

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
575 bytes
67.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-goto_response-1668866-153.patch. Unable to apply patch. See the log in the details link for more information. View

lol
poor dwarfs:(

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -API clean-up, -WSCCI, -symfony

The last submitted patch, drupal-goto_response-1668866-153.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-goto_response-1668866-153.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +API clean-up, +WSCCI, +symfony

The last submitted patch, drupal-goto_response-1668866-153.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
6.09 KB
68.41 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

since i am rerolling, here is with the url_generator injected

Status: Needs review » Needs work

The last submitted patch, drupal-goto_response-1668866-159.patch, failed testing.

chx’s picture

sniff. if we can't have dwarf ninjas what about some Jedi Ninjas?

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
71.39 KB
PASSED: [[SimpleTest]]: [MySQL] 58,045 pass(es). View

git stash failed me..forgot RedirectResponseSubscriber.php in patch above
@chx: hilarious video:P

pdrake’s picture

Status: Needs review » Needs work
+++ b/core/includes/form.incundefined
@@ -4854,6 +4882,28 @@ function _form_set_attributes(&$element, $class = array()) {
+  exit;

We should not be perpetuating the use of exit in core.

ParisLiakos’s picture

Status: Needs work » Needs review

pls read #131

pdrake’s picture

I have read #131 and I disagree with continuing the use of exit D8.

tim.plunkett’s picture

Please propose another solution then.

ParisLiakos’s picture

FileSize
71.94 KB
PASSED: [[SimpleTest]]: [MySQL] 56,170 pass(es). View

reroll
(removed also an instance of drupal_goto in update.inc's update_bach default arguments)

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -API clean-up, -WSCCI, -symfony

The last submitted patch, drupal-goto_response-1668866-167.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-goto_response-1668866-167.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update, +API clean-up, +WSCCI, +symfony
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks great, and is green!
Let's finish this one off.

Crell’s picture

This gets a +1 from me as well. ParisLiakos++

ParisLiakos’s picture

FileSize
71.95 KB
PASSED: [[SimpleTest]]: [MySQL] 57,959 pass(es). View

reroll

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/includes/form.incundefined
@@ -1305,22 +1322,33 @@ function drupal_redirect_form($form_state) {
+        else {
+          return new RedirectResponse(url($form_state['redirect'], array('absolute' => TRUE)));

The 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:

+++ b/core/modules/comment/comment.pages.incundefined
@@ -62,19 +63,19 @@ function comment_reply(EntityInterface $node, $pid = NULL) {
-          drupal_goto("node/$node->nid");
+          return new RedirectResponse(url("node/$node->nid", array('absolute' => TRUE)));
         }

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.

ParisLiakos’s picture

i prefer introducing a method to url generator...maybe generateAbsolute than subclassing RedirectResponse
but we already follow this pattern in core..we can do this in a followup

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

@Parisliakos, @catch, @timplunkett discussed this is IRC and decided to bikeshed the DX after the patch has been committed in a followup.

alexpott’s picture

catch’s picture

Title: Replace drupal_goto() with RedirectResponse » Change notice: Replace drupal_goto() with RedirectResponse
Status: Reviewed & tested by the community » Active

Committed/pushed to 8.x, thanks!

Needs a change notice for now, even if we change the change notice.

ParisLiakos’s picture

change notice here https://drupal.org/node/2023537
i linked the DX issue so we can get some feedback there

ParisLiakos’s picture

Status: Active » Needs review
Crell’s picture

Title: Change notice: Replace drupal_goto() with RedirectResponse » Replace drupal_goto() with RedirectResponse
Status: Needs review » Fixed
Issue tags: -Needs issue summary update

Change notice looks good. Thanks, Paris!

Dave Reid’s picture

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

Dave Reid’s picture

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

ParisLiakos’s picture

we 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

Status: Fixed » Closed (fixed)

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

joachim’s picture

Status: Closed (fixed) » Active

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

tim.plunkett’s picture

Issue summary: View changes
Status: Active » Closed (fixed)

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

Dave Reid’s picture

Status: Closed (fixed) » Active

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

tim.plunkett’s picture

Status: Active » Closed (fixed)

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

SiliconMind’s picture

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