system_goto_action breaks core APIs

Did you custom code a trigger, or use a provided one?

More detail, including the code used (either a function reference or a copy) required.

Confirmed. See the screenshot for configuration

This is a bug in Drupal core. If I understand correctly, the action 'system_goto_action' issues a drupal_goto() on invocation of hook_nodeapi().

Problem is, you can't do that, because it will fire _before_ node access hooks are triggered. Which is why your Domain Access settings are never stored on first save.

See the very bottom of http://api.drupal.org/api/function/node_save/6.

Refiling as a core bug.

For the time being, it should work, but only for 'Trigger: After saving an updated post', so long as you didn't alter the node grants.

Typo in title. And this bug is still present in D7, so it needs a fix and a backport.

The only solution I see is a new hook_node_postsave() to allow this type of feature.

It breaks other things, too, like search_wipe on delete.

http://api.drupal.org/api/function/node_delete

Why not fix it?

That might work for part of it, but the other $op hooks have to be checked, too.

But, do we introduce a problem where some modules expect to interact with hook_node_save() in order to alter grants? Not in D7, of course, but in D6.

/me explores.

Nope. Executing drupal_goto() here makes it so that other invocations of the hook (ones that execute after this one) never get called.

I have seen this bug in contrib before, actually, and will look for a reference.

For instance http://drupal.org/node/683748#comment-2475956

New one! :)

That's a nice little workaround, which I think can be generalized (through a wrapper function, perhaps) so that other modules can override the hardcoded redirect in node_form_submit().

How does this show up for anonymous users? Will it cause an entry to be made in the sessions table for them?

Anon users creating nodes is an edge case, but needs to be considered.

+++ includes/form.inc	4 Mar 2010 22:25:48 -0000
@@ -850,6 +850,12 @@ function drupal_redirect_form($form_stat
+    unset($_SESSION['drupal_goto']);

No, the session data is immediately removed within the same request, before the redirection happens.

Powered by Dreditor.

even cleaner would be for the action to set $_REQUEST[''destination'] and then let fapi do its usual redirect.

Subscribing so I can help test / modify patch per moshe's suggestion later.

@moshe will that overrule the $form_state['redirect'] added by node_form_submit() without additional work?

yes. see drupal_goto()

The problem with this approach is that it doesn't work for all possible triggers:

http://api.drupal.org/api/function/system_action_info/7

e.g. it doesn't work on node view

@tha_sun That trigger should never be allowed to affect Node View, IMO. What kind of crazy feature is that?

@moshe Wow. Took a while in the call stack, but that might work. [Edit]

This is just a poorly thought-out feature. I would consider removing it from core.

You gotta follow the trail. redirect_form calls drupal_goto which honors $_GET[destination]. See http://api.drupal.org/api/function/drupal_goto/7.

Agreed that this trigger need not work on view operation.

Here's a working patch. Note that if you try to use this action and batch update content, you will get redirected, which is probably a Bad Thing, so I banned redirects on /admin and /batch. This could also (conceivably) mess with system updates, if node_save() or similar is invoked from a hook_update_N().

The array('any') is replaced, and shows a horrible design flaw in actions/triggers. We simply need to _deny_ this action to certain events. Or we can put 'any' back, and let users redirect people away from node/10 to some preset page if they choose to. I don't care, as this should fix the primary issue.

@held69

Unfortunately, you stumbled into an issue that is much larger than you or I first suspected. The current feature breaks parts of Drupal if you use it. Until that gets fixed, I advise you not to use this Trigger, as it will always fail.

The problem is that we have to fix the underlying code in Drupal 7 (which is not the version you are using) and then port it back to Drupal 6.

So this issue is now a developer discussion of the best way to solve this problem. When we have a patch for Drupal 6, I'll try to let you know.

Thinking this issue might be the cause of a problem in a site I'm working on, I "ported" the patch to D6. However, since it didn't fix the problem, I have no idea if it's actually effective or not. Might be the base of a D6 patch, though.

Approach looks decent but we need comments for the trigger restrictions, and probably a comment as to why we use $_GET['destination'] instead of drupal_goto().

With catch's requests incorporated. Also expands the list of allowed triggers to match non-view operations in trigger module.

This looks good to me. I don't like that we have to specify 'admin' and 'batch' to avoid things running on admin pages but fixing that is out of scope here.

Absolutely nothing should go into the triggers or actions subsystems without tests. There is an enormous amount of code there that has never even (apparently) been executed before, and tests help mitigate that. andypost and I have rolled quite a number of tests and fixed many bugs uncovered in #721086: Create tests for system actions, clean up token replacement, clean up triggers. More test coverage and more bug fixes are still required.

I disagree with that, insofar as this is an obvious showstopper of a bug.

This should go in, and tests should be handled separately. Otherwise, we'll waste time testing functionality that we know to be broken. And I have no idea how you're going to test redirects, especially when we ban them on batch actions.

It is also a critical issue for D6, and needs a quick backport.

Yes unless you want to make #721086: Create tests for system actions, clean up token replacement, clean up triggers a dependency of this issue, I disagree with having to write a dedicated test here. The test could probably assert that it's impossible to add the redirect action to a 'view' $op, so I'm not sure testing the absence of the drupal_goto() itself is such a big deal (although it would be for the redirect functionality itself), but it's much easier to add once that initial test harness has gone in. This is assuming there's not already tests in that issue for restricting actions to certain triggers, which would cover this if so.

It's easy enough to just add some tests for this action. I won't get into a status-setting battle with you, but just adding correct-behavior tests for the system redirect action would be a step forward.

Essentially, this entire subsystem is desperately in need of attention. Just fixing here and fixing there will not get us where we need to go. IMO every fix in this subsystem should carry associated tests, preferably extensive ones.

I would be happy with a set of tests that test correct behavior of this action in a number of circumstances. And I have no problem with making #721086: Create tests for system actions, clean up token replacement, clean up triggers a dependency of this one, if tests are contributed there.

Also related:
There's a test #286668: Trigger "User logging in" doesn't work with Action "Forward to URL"
Required for user.module triggers work properly #403446: Increase default weight of trigger module

The patch in #403446: Increase default weight of trigger module is misguided and will be solved by this patch. Changing the module weight is a false solution. Correcting the symptom, not the cause.

Same patch with a test from #286668: Trigger "User logging in" doesn't work with Action "Forward to URL"

I think to mark as duplicate #403446: Increase default weight of trigger module too

Nice! New patch should still be RTBC.

Couple of minor things and some questions...

+++ modules/system/system.module	29 Mar 2010 22:24:50 -0000
@@ -2851,11 +2851,32 @@ function system_action_info() {
+      'triggers' => array(
+        'comment_delete',
+        'comment_insert',
+        'comment_presave',
+        'comment_update',
[snip]

Dear lord.

Should we stick this into some kind of helper function so that if another action comes across this same requirement we don't have to duplicate this 70,000 entry long list? Or is this a super-special case?

+++ modules/system/system.module	29 Mar 2010 22:24:50 -0000
@@ -3068,7 +3098,11 @@ function system_goto_action_submit($form
+  // Do not redirect on administrative or batch actions.
+  $path = arg(0);
+  if ($path != 'admin' && $path != 'batch') {

Same question.. should we encapsulate this in a function so other actions that are batch/admin sensitive can re-use the same logic? Or too special-cased?

Also, can the comment explain why this can't be used on admin pages..? I get batch, but not admin.

+++ modules/trigger/trigger.test	29 Mar 2010 22:24:50 -0000
@@ -302,15 +302,37 @@ class TriggerOtherTestCase extends Trigg
+      'pass' => $contact_user->pass_raw

Comma at the end here.

+++ modules/trigger/trigger.test	29 Mar 2010 22:24:50 -0000
@@ -302,15 +302,37 @@ class TriggerOtherTestCase extends Trigg
+    // Verify that redirect happen.

that "the" redirect "happened".

Powered by Dreditor.

Getting fresh look on this
- should this action check for AJAX operations?
- we already have 'behavior' => 'changes_property' for triggers. Should we implement something like this for actions - 'changes_url'?

The argument for not redirecting on Admin is if you are doing a hook_*_operations action that triggers a condition, you probably don't want to invoke the redirect rule. The rule should probably only happen when a single entity is being operated on. But if all admin pages use batch API for this, it may not be needed.

All batches and batch operations are executed on /batch.

At least devel_generate module skips batch if the number to process is below a certain number, so the admin check would be useful for that. Possibly for things like admin/people/add too.

One other question I had was whether we should ignore this action if $_GET['destination'] is already set.

@agentrickard $_GET['destination'] could be set by formAPI so action should have more priority on this. And that's this issue about!

Agreed. Just checking.

Updated patch, includes fixes from #39. Also pointed about using hook_action_info_alter() for contrib triggers.

We should probably separate the 'non-view' core actions to a function or list, for ease of reuse, as webchick suggested.

Maybe a tag other than 'any', such as 'action' or 'change'.

@agentrickard it's a good idea to introduce new tag but it's name should be like 'non-view'

Revised patch with the new tag, 'non-view' and updated documentation.

Better title to get more attention.

That's much cleaner. admin/batch is a bit of band-aid but don't have any better ideas except removing the whole thing. Unless we guarantee that all admin operations are batched, which they aren't, we can't only do batch.

+++ modules/system/system.api.php	7 Apr 2010 13:57:58 -0000
@@ -2683,7 +2683,9 @@ function hook_file_mimetype_mapping_alte
+ *     declare support for any trigger by returning array('any') for this value. If
+ *     the action should not be available on a View operation, you may return
+ *     array('non-view') to enable this action for all other triggers.

This comment is puzzling. It is unclear why one would not want to make an action available on a View operation. What about other modules that need exceptions? Sounds like a hack to me.

+++ modules/system/system.module	7 Apr 2010 13:58:03 -0000
@@ -2848,11 +2848,14 @@ function system_action_info() {
+    // The 'any' trigger includes view operations, which
+    // should not be subject to a redirect. Other modules
+    // should use hook_action_info_alter() for their triggers.

Lines wrap too early.

+++ modules/system/system.module	7 Apr 2010 13:58:03 -0000
@@ -3054,6 +3057,15 @@ function system_goto_action_submit($form
+ * FormsAPI to issue a redirect after processing is complete.

FormsAPI should be Form API.

121 critical left. Go review some!

For what logical reason on God's green Earth would you want to redirect someone away from node/3 to another URL based, the way the triggers currently work, solely on the fact that hook_node_view() or hook_comment_view() was invoked?

And if you needed to, you can _alter() it back in. But for core to support blindly redirecting away from node or comment view is insane.

We can set it back to a long list of allowed actions, but thought abstracting it made more sense.

The 'any' flag is what seems like a hack in this instance, when you consider that it is the _only_ special case.

#37 had the long list, and was already RTBC before, marked CNW by webchick primarily for that reason. So, we have two competing approaches. webchick didn't like the first one, Dries didn't like the second one. Adding the "Needs committer feedback" tag so we don't keep re-rolling conflicting versions pointlessly.

Also this is stupid, but it's an issue you can only get into with specific config, so not critical IMO. Still needs work for comments. #37 and #50 both still needs work for comments regardless of which one we end up going with.

I ran into the same problem with rules and I solved it by letting the action set a global variable, which then is checked on hook_exit() or before rendering the page, to actually invoke the redirect. Not clean, but it does it.

Check rules_action_drupal_goto() here. It also has the dangerous "immediate" option, but I think it's safer to remove that possibility at all. In d7 we could use hook_page_build().

So instead of working around it, why not fix it?

>For what logical reason on God's green Earth would you want to redirect someone away from node/3 to another URL based, the way the triggers currently work, solely on the fact that hook_node_view() or hook_comment_view() was invoked?

Well with conditions in place (rules) people do that often.

@fago Are current "rules" far from this triggers/actions?

@fago: The current patch tries to solve it as properly as possible. hook_exit() and other hooks run too late for certain triggers, e.g. when submitting a node via node_form(). drupal_redirect_form() will redirect after form submission. This action almost exclusively wants to only get triggered when being invoked non-programmatically via form submissions. So the action needs to intercept and change the redirection target before drupal_goto() is invoked after form submission, and we cannot invoke drupal_goto() in another, dedicated location, as that would potentially break API hooks from running.

Perhaps we can skip the "any" assignment problem by simply renaming the action to clarify that it's only affecting form submissions, i.e. "Redirect after form submission".

Frankly, this is a case where the solution is nearly worse then the initial problem. This 'non-view' stuff strikes me as a one-off, ad-hoc workaround for the problem.

Why not simply:

- set $_GET['redirect'] in system_goto_action(), only if not already set, and set a $GLOBALS['force_redirect'] variable
- add a call to drupal_goto() in drupal_page_footer() after the invocation of hook_exit().

Is there any probability that hook_exit() would skiped like in D6 advanced cache? If so we could not rely on it.

I just fixed the action for rules 7.x-2.x, see http://drupal.org/cvs?commit=352594
I think we could apply the same pattern in core (and re-use it in rules then).

1. set $_GET['destination']
2. make sure drupal_goto() is called via the global + hook_page_build() - or can someone suggest a better hook to use?

@andypost: yes, rules is pretty different, but the problem stays the same.
@#61: there is drupal_exit() now, so modules have to use that.

@sun:
In case another module or the form API calls drupal_goto() we have no problem at all, as we have set the destination parameter which drupal_goto is going to take into account.

@damien: drupal_page_footer() comes too later as the HTML has been already outputted, thus you can't set headers any more.

Maybe best we could add a check for the global after drupal_render_page() was called.

Renamed the action's label and extensively clarified the docs. Hope you get the point.

All my points in #60 still stand. And no, it's not too late to do that in drupal_page_footer(), which is the function that actually outputs the page to the browser.

@Damien: Can you please explain why that would be any better than the suggested solution? I fail to see how you're going to handle form submissions, AJAX requests, and whatnot in that approach. All of that is properly taken into account with this patch. This action simply doesn't make sense to be used outside of form submissions.

@Damien You are wrong - drupal_deliver_html_page() prints output so drupal_page_footer() is too late for redirect!

@sun Renaming seems reasonable but I not seeing any check for "is this request a form submission"

@andypost: *sob* Please re-read #65: drupal_page_footer() is what *actually outputs the page to the browser* (the ob_flush() is there).

@Damien You right about printing but we talk about redirection after submitting form so code execution never reach this place because possible redirect from http://api.drupal.org/api/function/drupal_redirect_form/7

My rant about hook_node_view() is irrelevant in the 'redirect' approach anyway, which only applies to form submissions. A goto in hook_exit() is likely fine, but only if we can account for the AJAX case. Also note that modules will sometimes call hook_node_view() in order to build data for presentation outside of normal contexts, so I still think redirecting when that hook is triggered is a very bad idea.

@Damien The 'non-view' bit was in response to earlier discussion, since the removal of drupal_goto() meant only form submissions would trigger the event, which I still believe is the only desirable behavior.

Ok, why not for the "only redirect form submissions" approach.

But:

• We should probably use path_is_admin($_GET['q']) here, instead of hardcoding "admin" and "batch",
• We should not alter $_GET['destination'] if it is already set in the URL.

@"Redirect after form submission"

But why can I configure it if there is no form in place at all? Why does it apply if no form is submitted at all (but someone else invoked drupal_goto())?

>I fail to see how you're going to handle form submissions, AJAX requests, and whatnot in that approach. All of that is properly taken >into account with this patch. This action simply doesn't make sense to be used outside of form submissions.

Oh it makes a lot of sense to do a path redirect without any form. It's just doesn't make much sense for triggers, because they miss conditions. But actions aren't supposed to be used by triggers only, no?

@#64: If you set the destination that way, you have to urlencode() it.

@damien & drupal_page_footer(). Thanks for pointing that out - I overlooked that too. I really like doing it in drupal_page_footer() as it's the right place for it and it's working regardless to what the page is supposed to be rendered too. And yes, it might not make much sense to invoke the action in case of AJAX requests, but that's not up to the action to decide. *If* it is invoked, it *has* to work.

-> Patch adding that simple check attached. With something like this in place, it's easy to fix up the action.

Whether the action should "force" the redirect and set $_GET['destination'] is another question though. I just can say that having not the possibility to "force" it renders the action useless in situations like a user login issued from the login block. Imo having that configurable makes sense.

@fago drupal_page_footer() does not work for forms with redirects!

EDIT for AJAX requests http://api.drupal.org/api/function/ajax_footer/7

@ajax: oh, ok drupal_page_footer() is only used in drupal_deliver_html_page() so it won't apply to ajax requests.

@forms:
again: overriding existing redirects like form redirects is simple: set $_GET['destination']. Thus doing that + setting the global flag would work for all page requests delivered with drupal_deliver_html_page() or drupal_goto() - what I think is fine.

Shoot, did we get stalled on this one? Let's decide and get it in. IRC discussion? There are other patches with new tests depending on it.

We did. I greatly prefer the form-based solution in #64 to the 'force_redirect' approach.

This needs to be in sync with #733028: SA-CORE-2010-001 - Open redirection, which suggests that we might need the 'force_redirect' parameter, so that we can securly pass 3rd-party URLs through code, but not through a GET request. But perhaps that value can be acted on by the form handler...

Options:

1. Merge both approaches and patches, as we have to support form submission scenarios as well as other scenarios - whereas other scenarios do not really make sense with core's Trigger module (only for Rules and other implementations supporting conditions).
2. Go back to the patch that re-purposes the action to "Redirect after form submission" only. The label is totally clear, nothing else should be expected. Rules and other modules can implement their own advanced action.
3. "Do nothing." Remove the entire action from core. Doesn't really solve the problem though, since contributed modules like Rules will have to work around this limitation in core.

I think 1) is the best way to go. A patch like #72 makes it easy to fix the action like it is now and I could leverage that for Rules too.

ad 2), yes the label makes sense. But still it's available on any trigger whether there is a form or not. Whether it makes sense to make use of non-form redirects or not, it's simpler if you can just use "Page redirect" when you want and you don't have to think about forms.

Next as for 3) Rules would still have to workaround the limitation in core with case 2). Thus even if we go with 2) or 3) I'd like to see #72 get in, but that could be a separate issue too.

Just noticed this was moved to non-critical. It's critical. Drupal should not allow you to break it's core APIs through configuration.

Tagging this with 'priority major' in preparation for that priority, I stick by #56 in that this comes under "stupid but not a release blocker".

Also uploading a patch which just removes the action.

Needed a test change.

ok, here is patch that actually fixes the action for any page + includes a test. It's based upon #64 and #72.

Do we need to commit the user session before bailing out?

No, drupal_goto() takes care of that.

OK, then one last question about this approach: should 'force_redirect' override other forms of redirection, such as $_GET['destination'] or $form_state['redirect']?

+ // Modules may set this global to force redirecting to the given URL. Using
+ // this global ensures the execution flow is not interrupted at stages where
+ // it is not expected, e.g. while performing a save operation.

No, it's forcing a redirect in case none happens else. It's not about overriding other redirects. Makes sense?

It does, but I don't think that's the intention of the action. I would think the action _would_ override those other redirects.

Use case:

-- When editing a node, the user is always taken to node/NID/view, the default behavior.

-- Admin wants them to go to /user instead.

Does this approach allow that to happen?

Oh the action does correctly override, as it does more than setting the flag. With the patch you configure the action and it just will work.

@use-case:
yep.

code style looks ok.

Powered by Dreditor.

@agentrickyard looks like you've done a pretty good review of the approach, does it sound good to you?

how to test this? try what is in the original issue (and #2) and/or #6?
in #75 rfay mentions other tests are waiting to get in on this. which ones? maybe trying those could test this/complete the review?

Well, I think the approach is the best solution to a bad problem. The other option is ripping this feature out of core, which isn't desirable.

The simpletests should have us covered. Otherwise, you can test with the approach defined in #89.

s/Force redirecting/Force redirection/g - this needs to be fixed at least twice.

Instead of putting the global directly in drupal_page_footer(), I don't see why system.module can't implement hook_exit() (with an accompanying comment that it only exists to support this action). Then when actions are hopefully moved to a separate .module in D8 instead of being crammed into system.module, that could move with it, rather than adding hard-to-track clutter to common.inc for what is a minor and fairly ill-thought-out feature in the first place. If we can keep the ugliness relatively self-contained then at this stage it's probably better to fix it than rip it out though.

The patch adds $GLOBALS['force_redirect'] as its intended to be useful generally, such that it could be used in other situations too (e.g. by the rules action). Though then it might make sense to add this as an option to drupal_goto() e.g. $options['postpone'], thus supporting the functionality of issuing page redirects anytime directly?

Usual use cases that redirect upon node-edit break with the action, but it would also break if a module issues drupal goto too. So why not support that use-case on a API level instead of on action level only?

Putting it in system_exit() doesn't make it any less API level than putting it in common.inc, just keeps drupal_page_footer() a bit tidier - we could not move it out of system_exit() if the rest of actions moves eventually. Also nothing stops rules implementing hook_exit() if it has to either.

However, a $postpone option to drupal_goto() sounds better than the global checking, I've seen drupal_goto() in form submits before and it's not fun when you find out that's why nothing worked. If this is an extra optional parameter then it's not really an API change either.

Hello guys,

Seems like I am running into the same problem. However, I am reluctant to go into hacking the core. So, can anyone suggest a more user-friendly way of having content being saved and triggers to work ok.
I have a "Redirect to profile creation page, if users have no profile." rule with "Heartbeat: when user adds a page, log user activity After saving new content" working together. My guess is that the latter seems to override the former, hence the non-saving of data.
Would I be correct to assume so?
Thanks for the thread as it gives me more insight into what's happening.
T

This was my current progress yesterday with adding an $options['delay'] = TRUE to drupal_goto() and checking the redirect. Still working out form redirection clobbering out the delayed redirection.

Reroll to fix that issue. Not crazy about unsetting $_GET['destination'] here. Other option seems to be multiple possible values for the delay static.

@Tafa in #96
This is a Drupal 7 issue, so it has to be fixed in 7 first. Then maybe, maybe after it is fixed it might be back ported to Drupal 6.

You might be able to get a fix in by installing Drupal 7 on a test site, and testing the patch.
See:
http://drupal.org/patch/apply
and
http://drupal.org/patch/review

You can ask in irc for help reviewing, people always like to help new people who are willing to learn how to review and test patches
http://drupal.org/irc

Dave and I discussed this some more. Key is to clobber $_GET['destination'] at the end of the page request.

Fixes bad patch roll. Also returns the disallow of delayed redirects on batch actions, which have their own mechanism for redirection that should not be overruled by this action.

+  // Skip redirection if there is already a delayed redirection is set.
+  if ($redirect = drupal_static('drupal_goto_delay')) {
+    return;
+  }

Well. A drupal_goto() is final (you end the request, so the first drupal_goto() in the code flow wins). Similarly, the first call to drupal_goto(), even delayed, should always win. Could we add logic directly into drupal_goto() to do that, so that this snippet becomes unnecessary?

I don't think so, because here we are talking about a user configurable option that can overrule the behaviors of a form written in code. But maybe we can handle that inside drupal_goto().

[edit]

Tested. With that line removed, the form redirect clobbers the action, which is undesirable.

I suppose we could drupal_static the goto?

+1 for making this a general usable drupal_goto option. I think what damien meant in #104 is that drupal_goto() itself should check for delayed redirects. I've improved the patch to do so, thus the form API needs no separate check and moreover that way the delay option will be taken into account properly regardless whether someone invokes a drupal_goto() later on. Also I ensured that the first delayed drupal_goto will win. Then I changed 'skip_destination' to be another documented drupal_goto option and let the action set it instead of enforcing it for 'delay'. As 'delay' in general doesn't require the use of 'skip_destination', we should make it optional to the caller. I also re-added the check to not invoke a drupal_goto() in the admin area in order to prevent users from locking themselves out.

Patch attached. I guess due to #839172: Cannot redeclare user_update_7010() the bot won't be able to test it. :/

+++ includes/batch.inc
@@ -477,6 +477,8 @@ function _batch_finished() {
+    // Do not allow delayed redirects on batch actions.
+    drupal_static_reset('drupal_goto_delay');

This comment is not sufficient, it only covers the next best conclusion ("What is this reset doing?"), but it does not give reasons for "Why do we want to prevent delayed gotos after finishing a batch?". Not even sure whether this issue sufficiently explains this line, but for overly complex issues like this, we shouldn't rely on CVS annotate either way.

Aside from that, I just wondered: Shouldn't we force all drupal_goto()s during a batch run to be delayed and be effectively ignored?

+++ includes/common.inc
@@ -641,7 +641,10 @@ function drupal_encode_path($path) {
+ *   - delay: If TRUE, this redirect is delayed to the end of the page request.

@@ -656,11 +659,33 @@ function drupal_encode_path($path) {
+    // Record a delayed redirect only in case there is none yet.

The @param key description should already tell that only the first delayed goto wins and all subsequent are going to be ignored...

...and AFAICS, only delayed gotos are going to be ignored. A regular non-delayed goto will be executed, i.e., wins over the already delayed goto.

I can't comment on whether this behavior is correct, but the PHPDoc definitely needs to fully explain the situation.

After tinkering about the Batch API thing above, the current logic does not look entirely natural to me: If anything triggers a delayed goto during a request, then the caller expects all hooks and stuff to run until the page request ends (i.e., the very heart of this issue). To make sure that this desire is actually fulfilled, subsequent gotos would have to be ignored.

At the very least, that would be a clean logic for me. The other way around, disrespecting a previously delayed goto, would make the entire delayed goto very fragile, as you can't be sure whether your delayed redirect will actually happen delayed.

Of course, this would require to prepend/append a "return" to all current invocations of drupal_goto(). (However, that may be good idea to do anyway.)

Lastly, what have been the reasons to implement the FIFO logic and entirely ignore subsequent delayed gotos? While we want to put a high-level info into the function docblock, we need to carefully describe in inline comments _why_ we are doing things that way.

+++ includes/common.inc
@@ -656,11 +659,33 @@ function drupal_encode_path($path) {
+    if (!$delayed_redirect) {
...
+  if ($delayed_redirect) {

1) !isset(), you want to explicitly test against NULL.

2) isset(), it's either NULL or the data we stored.

+++ includes/common.inc
@@ -656,11 +659,33 @@ function drupal_encode_path($path) {
+      // Ensure this routine only runs once.
+      unset($delayed_redirect['options']['delay']);

Took some time to understand this line. Would be a lot easier to understand if we'd just unset($options['delay']) directly after identifying that it's set, instead of afterwards. We're immediately returning either way.

+++ includes/common.inc
@@ -656,11 +659,33 @@ function drupal_encode_path($path) {
+  // A destination in $_GET always overrides the function arguments unless overridden
+  // by a delayed redirect. We do not allow absolute URLs to be passed via $_GET, as
+  // this can be an attack vector.

Exceeds 80 chars.

Powered by Dreditor.

I agree that 'delay' needs better docs, in particular we need to document *when* to use it.

>...and AFAICS, only delayed gotos are going to be ignored. A regular non-delayed goto will be executed, i.e., wins over the already delayed goto.

No, if there is delayed goto it will be applied. The docs say "If TRUE, this redirect is delayed to the end of the page request." and is that way, regardless whether the end of the page request would be another drupal_goto() or not.

>After tinkering about the Batch API thing above, the current logic does not look entirely natural to me: If anything triggers a delayed goto during a request, then the caller expects all hooks and stuff to run until the page request ends (i.e., the very heart of this issue). To make sure that this desire is actually fulfilled, subsequent gotos would have to be ignored.

I don't think so. A regular drupal_goto() already marks the end of the page request.

@FIFO:
The first redirect should win, as it is the natural behavior for redirects - the first applied redirect wins. With FIFO delaying a redirect doesn't change that.

>Aside from that, I just wondered: Shouldn't we force all drupal_goto()s during a batch run to be delayed and be effectively ignored?
Postponing them to after the batch makes sense to me, but that's another issue.

>1) !isset(), you want to explicitly test against NULL.
I know, but my check is valid and equal and looks more readable to me.

ok, I improved the docs and addressed the rest of the comments. I've removed the batch API related fix for drupal_goto() but re-added the batch check to the action. That way the action won't introduce troubles for batches, but still the handling of redirects could be fixed for batches generally in another issue.

There seems to be some regression in the latest patch? Plus some lines coming from another issue.

Unrelated:

- * functionality instead of calling this function. 
+ * functionality instead of calling this function.

Unrelated:

- *   - 'url': URL to redirect to. This will be passed through
+ *   - url: URL to redirect to. This will be passed through

Regression?

+  // Do not redirect during batch processing or on administrative pages.
+  $path = arg(0);
+  if ($path != 'admin' && $path != 'batch') {
+    drupal_goto(token_replace($context['url'], $context), array('delay' => TRUE, 'skip_destination' => TRUE));
+  }

I still think that skip_destination should be set by the delay subroutine, though other functions could also call it.

The patch just removes some whitespace of the end of those lines. They need to go anyway.

'url' -> url is no regression, but makes it conform with our doxygen convention.

>I still think that skip_destination should be set by the delay subroutine, though other functions could also call it.
What if I want to issue a usual drupal_goto() but delay it. Then I don't want to skip the destination parameter.

If you don't skip_destination on a delay, then your goto always gets clobbered when used with a destination param. I find that undesirable.

This is the default behavior of drupal_goto() - why should it change when I change a redirect to be delayed? In case of the action we want it that way, but else in general not.

Because delayed is the mechanism we have for avoiding FIFO in order to make this action secure. Using 'delay' should trump all other means of redirection.

FIFO is not related to the destination parameter, as the parameter exists independent from any redirection ordering. Also delaying doesn't avoid FIFO, contrary it introduces the question of which redirect to apply in the end where "FIFO" is our solution.

With 'delay' as drupal_goto option as implemented in the patch, this action is not the only use-case for delaying. Any module that wants to issue drupal_goto() e.g. in a hook_node_insert() implementation would have to use it too. Therefore I asked in #115 why I should want to have 'skip_destination' suddenly default to TRUE if 'delay' is used by some code *being not the action* as it does not default to TRUE if the code doesn't need to delay the redirect.

Here's a new patch.

I cleaned up the documentation in #111. Also, I think that when the trigger is triggered, it should make sure it's the one goto that is going to take precedence, so I added a static reset to that call.

Other than that, the code is identical and only the doc/variable names have changed.

I would also like to say that I read through this entire thread on this issue today for the first time. This whole thing seems like a good approach to me.

Ooops. Minor tweak (line wrapping and typo).

@fago I'm just not following the logic, but if we plugged the security hole and didn't break the action, I'm ok with the patch as is.

Let me bug Dave to review.

Most recent patch fails, as the path alias of a node is not set properly on save when a redirect trigger is set on node save.

Any idea why?

Premature redirect before hook_node_save() finishes, but I haven't pinpointed why.

[Edit: wait, I may have reversed the dang patch. Need to retest.]

Pay no attention to the guy who can't keep his environment clean! Patch still works, though it applies with fuzz.

+++ includes/common.inc	28 Jun 2010 19:12:29 -0000
@@ -2109,7 +2141,7 @@
@@ -2442,6 +2474,13 @@

@@ -2442,6 +2474,13 @@
 function drupal_page_footer() {
   global $user;
 
+  // If a delayed drupal_goto() was requested at some point in page processing,
+  // process by calling drupal_goto(), which will find the stored request and
+  // process it.
+  if (drupal_static('drupal_goto_delayed')) {
+    drupal_goto();
+  }
+

One potential problem with putting this in drupal_page_footer() is the the redirection will only happen on html returned pages. If a redirection happens in another type of response drupal_page_footer() is not called.

drupal_page_footer is only called by drupal_deliver_html_page which is to "Package and send the result of a page callback to the browser as HTML."

What is called in all page endings (or should be) is drupal_exit(). This is called in drupal_goto() as well, If we consider using this we need to make sure to avoid circular loops.

If the intent is for this to only work on html pages it should be documented as such.

39 critical left. Go review some!

I agree with comment above surely this should happen in drupal_deliver_page that way we can actually operate on the trigger no matter the deliverly mechanism.

Agreed. Here's a patch that moves the processing to drupal_deliver_page() instead of drupal_page_footer().

if ($path != 'admin' ? what happened to path_is_admin() ?

Hmmm.

I took a look at path_is_admin(). It basically returns TRUE for paths that modules put into hook_admin_paths() implementations. For instance:

function node_admin_paths() {
  $paths = array(
    'node/*/edit' => TRUE,
    'node/*/delete' => TRUE,
    'node/*/revisions' => TRUE,
    'node/*/revisions/*/revert' => TRUE,
    'node/*/revisions/*/delete' => TRUE,
    'node/add' => TRUE,
    'node/add/*' => TRUE,
  );
  return $paths;
}

So if we use path_is_admin(), we are going to say that if we're on a node editing page, we won't redirect.

Meaning that this redirect hook wouldn't take effect if someone was editing or adding a node.
That seems wrong. Wouldn't that be a primary use of this hook: "I've just added a node. Redirect me to a thank you page", or some such thing.

Actually, why are we excluding admin and batch paths from redirects anyway in this hook? I think I maybe understand batch, but admin?

The assumption is that batch editing from the Content screen (in the admin path) is being done by a super-user who wants to bypass the redirection rules.

Without that exception, changing, say a group of nodes to 'promote to homepage' may trigger a redirect.

I don't think we can exclude /admin* ... note, that <10 semi-mass-updating of entities happen via form submissions, so drupal_redirect_form() should invoke the actual redirection.

@drupal_deliver_page():
Sounds good to me, in particular as this is now not limited to the action any more, though any delayed redirects issued during page rendering won't be applied. However I do think this is acceptable as usual hooks like node_view() run before anyway and just rendering the resulting $page shouldn't trigger such a logic anyway. I improved the docs to mention that the direct is delayed to that point.

+    // Make this request take precedence over previous drupal_goto() requests.
+    drupal_static_reset('drupal_goto_delayed');

Why that? In case multiple goto actions are executed, the first action should still win. Additionally I'd also say if another module invokes a delayed redirect before the action runs, it still has to win as that way this results in the same redirection as when the module would not delay the redirect. Consequently I do not think fiddling around with drupal_static_reset('drupal_goto_delayed') is a valid use-case.

>The assumption is that batch editing from the Content screen (in the admin path) is being done by a super-user who wants to bypass the redirection rules.

Applying operations to nodes or users should be fine anyway, as this is done with a batch. However this check is necessary to ensure users are not able to lock themselves out of the drupal site. Consider a module providing a trigger that runs on every page load. If one configures the action to to be triggered on that, the site would be unusable and the user won't be able to fix the configuration. Indeed the check for administrative pages seems to be to unspecific, thus I improved it to only care about the trigger pages.

also note that we cannot use hook_exit() for invoking the redirect, as usually the page content has been already sent when it is invoked. drupal_page_footer() doesn't do it that way, for which I've opened #845452: drupal_page_footer() calls hook_exit() to early.

a) Why did you need to remove this from the drupal_goto doc:

+ *     precedence. Call drupal_static_reset('drupal_goto_delayed') if
+ *     you need a later delayed drupal_goto() call to take precedence.

I think that is useful information.

b) Kitten killing warning: there are several things in this latest patch that are completely unrelated (removing spaces from the ends of lines)

ad a): I removed it, as explained above:

In case multiple goto actions are executed, the first action should still win. Additionally I'd also say if another module invokes a delayed redirect before the action runs, it still has to win as that way this results in the same redirection as when the module would not delay the redirect. Consequently I do not think fiddling around with drupal_static_reset('drupal_goto_delayed') is a valid use-case.

As I don't see a use-case for doing so, I don't think it needs to be documented. Moreover I'd consider the static internal to drupal_goto. If a module already issued a drupal_goto() I see no point in removing that. Without delaying the redirect would have been already applied nevertheless - delaying should not change that behaviour but be safe to use. However if there is a use case for it, let's add it.

ad b)
Yes, those whitespaces need to go anyway.

OK on (a).

On (b), please file a separate issue.

ad b) We often scratch whitespaces as part of another patch. Imo creating separate issues for each wrong whitespace would just create unnecessary extra-work, so let's better stay with the simple way.

OK. It really depends on who is committing the patches, but fine.

I'm wondering if there should be a test that actually exercises the problematic behavior that was noted in the original bug report. Namely that if you assign a system goto action to a node save, the node isn't getting published. I would be more comfortable with this patch if we had a test that failed before patching, illustrating the reported behavior, and then passed after patching. As it is, I think you have to test by hand, so it's possible this issue could resurface down the road in Drupal 8, 9, etc.

Agreed with that. The more obscure the bug, the more likely we are to break it again if we don't have test coverage.

I'll take on the project of adding tests. Give me 24 hours (or perhaps less, we'll see)...

OK, here's a new patch. It's the previous patch, with an additional test that adds a redirect on node save, and tries to verify that the redirect happens and the hook_node_insert fires.

I have verified that the test fails without the includes/common.inc and modules/system/system.module patches -- the redirect happens but hook_node_insert is not invoked. With the patches, the test passes. So I think it's a valid test, and it reproduces the original error report. (The original report had to do with some contrib modules not getting their hook_nodeapi() implementations invoked in D6 when creating a new node; this is the equivalent in D7).

Adding 'API change' tag for the additions to drupal_goto(). Doesn't break BC but good to have it in the list for general tracking.

@jhodgdon: thanks for caring about the tests.

+++ modules/trigger/tests/trigger_test.module	7 Jul 2010 22:12:50 -0000
@@ -132,3 +132,12 @@
+function trigger_test_node_insert($node) {
+  $nodes = variable_get('trigger_test_nodes_inserted', array());
+  $nodes[] = $node->nid;
+  variable_set('trigger_test_nodes_inserted', $nodes);
+}

Wouldn't it be simpler to output a message and test for the existence of the message afterwards?

+++ modules/trigger/tests/trigger_test.module	7 Jul 2010 22:12:50 -0000
@@ -132,3 +132,12 @@
\ No newline at end of file

This should not happen.

43 critical left. Go review some!

Sure the test could output a message and look for it afterwards. That would work too. But most of our tests to verify a hook has been invoked follow the pattern of setting a variable.

The newline does need to be fixed...

>Sure the test could output a message and look for it afterwards. That would work too. But most of our tests to verify a hook has been invoked follow the pattern of setting a variable.

ok, I didn't know that. :)

Here's a patch that fixes the newline (hopefully). If someone wants to write a different test that uses a message instead of a variable set, please do. I think the current test is fine...

I reviewed the code and everything looks very good. It's very well documented and fixes an issue that has been bugging me for long. The fact that the tests pass is IMO enough for this to be RTBC, but I wasn't too involved with the issue, and I also didn't try it out myself, so I won't mark it such.

Don't pass.

Well now we know for sure.
Re-uploading #147 now.

Great, imo it's good to go. It fixes the issue, but in a way even modules benefit - now it's possible to redirect safely even in situations as hook_node_* or others (without access to $form_state) using the delay option. Also we have a new parameter that helps to avoid previously necessary trickery with $_GET['destination']. Finally it fixes the action and comes with tests!

No time for a full review just now, but this edge-case is definitely not critical.

+++ modules/system/system.module	8 Jul 2010 15:31:15 -0000
@@ -3069,18 +3069,25 @@
+  if (arg(0) != 'batch' && !(arg(0) == 'admin' && arg(1) == 'structure' && arg(2) == 'trigger')) {

This strikes me as incredibly brittle, and making assumptions that core's Trigger module is the only module that will use this system_goto() action, which is not the case. What does "Better Trigger" module do in contrib? (Or Views Bulk Operations, or Rules, for that matter?)

43 critical left. Go review some!

>This strikes me as incredibly brittle, and making assumptions that core's Trigger module is the only module that will use this system_goto() action, which is not the case. What does "Better Trigger" module do in contrib? (Or Views Bulk Operations, or Rules, for that matter?)

Hm, other modules using this action would have to make sure they don't run it on their configuration page - what usually isn't the case anyway. For modules providing ways to react upon hooks like Rules, this isn't so simple any more. Still this simple check makes sure everything is alright for the trigger module (and so for core in general), while for everything more I see no simple way to determine which pages should be excluded - so this is the best we can do now.
For Rules, I re-implement the action nevertheless as the action doesn't fit to the way Rules works. Anyway that would be easy thanks to the nice drupal_goto() API improvement of this patch - thus I could replace the currently in place workarounds by a proper API function call (++).

Still the main issue we are fixing here, is that the action currently breaks things if used with node_presave|insert|update hooks - what I'd consider rather critical as it renders the action not only rather useless, but dangerous. Thus bumping back to critical.

Lacking a better idea to prevent an epic redirect action via other tools and modules, I'd recommend to fix the critical hook breakage caused by this action now.

Defer perfectionism.

+1 to commit #151
drupal_goto() have a lot of use cases so this should be finally fixed

@webchick

Sadly, this is a critical security issue and there are only two options:

1) Fix it.

2) Drop this action from core.

Labelling it non-critical is not a solution.

I suspect contrib module that do similar things will have to update their code to comply with the approach. Personally, I thinking banning this action on all of /admin is perfectly reasonable.

Back in #56 I marked this not critical, I'm doing so again now in full agreement with webchick. It is a minor feature of core that is horribly broken, there's plenty of those, does not make it a release blocker - to quote sun, "defer perfectionism". Marking this critical is an abuse of that priority that renders it completely meaningless.

On the patch, adding stuff to system module that depends on trigger module is a real step backwards, but don't have the energy to argue on that except if we can't do it properly, we could remove it per #86. If it's, 'useless and dangerous', so why not just cull it? Better than useless, and dangerous, and fragile.

And if this is a critical security issue, and is in D6, then why is it not being handled in a private issue by the security team or tagged as such?

This isn't about perfectionism, this is about not putting in a hack that adds checks for optional core modules (and Trigger module of all the possible things...) into required parts of the system. And yes, I grant that this edge case can render your system buggy and broken, but it is nevertheless an edge case, and one that exists in D6 as well, and will not block release.

We have this $context variable being passed into system_goto_action(). Can the Trigger admin page pass in some sort of generic index to this array that the action could check for, which means "don't actually do the goto"? That way Rules, etc. could do a similar thing without instructing their users to hack this line in system.module.

And bumping back to needs work because this is not committable as-is. If the suggestion in #162 is a no-go, we should probably remove the action from core.

(aka, "What catch said." ;))

This isn't about perfectionism, this is about not putting in a hack that adds checks for optional core modules (and Trigger module of all the possible things...) into required parts of the system. And yes, I grant that this edge case can render your system buggy and broken, but it is nevertheless an edge case, and one that exists in D6 as well, and will not block release.

I think two things get mixed up here. First off there is the topic off this issue, which is about fixing the action that in *many* situations (hook_node(presave|insert|udpate)) breaks things. Whether its critical or not, the patch in #151 solves that.

Then there is another possible issue, what is the check mentioned in #154 is about. In *theory* a module could expose a new trigger for hook_init() - then the action would be configurable for that hook, what would lead to an unusable site. The check is about preventing this theoretical problem.

ok, here is patch without this questionable check. Still this patch fixes this issue fine, as I explained in #152. Let's handle the other, theoretical problem in a separate issue so it doesn't hold up this improvement.

see #851586: Ensure trigger configuration pages stays functional

Changing to major as per tag.

Tag update

#165: goto.patch queued for re-testing.

UGH. UGH. UGH.

Now, now.

Cough, cough.

Where were we before this patch knocked me out? Yes. Kitten killer monster, why do we deal with the drupal_goto action vs $_GET['destination'] in this issue? And, given it's a global pig anyways, why the heck do we put lipstick on this particular pig? Just unset it. If you do not like this approach then reroll without the unset and open another issue for it has nada to do with this one, here.

Now that's out of the way, may I draw attention to the uncanny resemblance of some_function(NULL, TRUE, array(), $booger, 'aardvark') (see the patch review guide) to drupal_goto(token_replace($context['url'], $context), array('delay' => TRUE, 'skip_destination' => TRUE));?

We need no stinking new options and still we are done with less code.

Opsie, minus debug.

Have tested it the new version of the patch is much simpler to understand. Just a reroll will additional doxygen for the new drupal_goto_delayed function. I'm happy to mark this RTBC from a functional point of view however given that this patch adds code comments it needs a second pair of eyes.

Looked over patch, all looks good except:

1. A few unrelated changes, I split these out into #870620: Trailing whitespace in common.inc and #870626: Extraneous spaces in trigger.test., and removed from this issue.

2. Simply checking for the path being 'batch' is insufficient, instead we should be checking if there is a current batch set and if that batch is running, added that check.

We shouldn't be doing anything with destination unless we are actually go to act on the redirect.

Discussion on IRC over batch brought up an issue to my thinking.

Currently in d6 if something needs to override the destination, like batch does, it unsets $_GET['destination'] then issues a drupal_goto; as far as I can tell that is still the case in d7.

This is how batch keeps track of what the destination was:

      if (isset($_GET['destination'])) {
        $batch['destination'] = $_GET['destination'];
        unset($_GET['destination']);
      }

http://api.drupal.org/api/function/batch_process/7

then it issues a drupal_goto to what the batch url is (usually 'batch', but can use a custom url like update.php).

So, assuming above patch, if drupal_goto_delayed was issued, the drupal_goto in batch_process, instead of redirecting to batch url, would exit, and call the shut down, and be redirected elsewhere.

This could also break form submission

    if ($type == 'submit' && ($batch =& batch_get()) && !isset($batch['id'])) {
      // Some previous submit handler has set a batch. To ensure correct
      // execution order, store the call in a special 'control' batch set.
      // See _batch_next_set().
      $batch['sets'][] = array('form_submit' => $function);
      $batch['has_form_submits'] = TRUE;

http://api.drupal.org/api/function/form_execute_handlers/7

In both, if a batch is set and not being process, it stores additional form_submit function in a special set, to be processed after the batch is processed as far as I can tell.

So, if the form has multiple submit handlers, they will never be fired.

Aka, as far as I can tell, this patch is still not solving the original purpose of not breaking core apis?

Additional note: even if batch processed, since the delayed is static, the new destination (ie why the system_goto action was set) would be lost.

If feels to me like the best place would be one place to check if an 'override' to drupal_goto existed (like $_GET['destination']) is and to hace places like batch_process check, and unset, that.

Uhm, great progress. #165 was ready, had documented API improvements, which actually worked + more tests.

#177 won't work in any case, as registered shutdown functions won't help us on regular pages that don't have a redirect yet - you cannot output headers after the HTML has been sent.

>We need no stinking new options and still we are done with less code.

This stinking options are straight forward to use, documented and work regardless of changing internals. Yes, fiddling around with $GET['destination'] does also work, but who knows?
And adding a separate function instead of adding an option (-> delay) is not better either, but just less visible.

Although chx's idea looks interesting, it stinks for the reasons outlined in #179, so I've to agree that #165 was and still is ready.

Just updating 165 with the batch test (as mentioned, batch doesn't have to be /batch for url, like with update.php).
$batch means the batch has been set, $batch['current_set'] means the batch is processing (vs only just being set).

However, this patch still would stop batch from working, as outlined in 178, as it overrides path, so batch_process would have to be updated (and anywhere in contrib if it does similar handling of $_GET['destination'], though I don't know of any). batch_process would need to remove the delayed redirect, then save it somewhere for when it's done.

Personally, the least problematic way seems to misuse _GET['destination'] (if (!empty($_GET['destination']) {), store the destination in destination then a global/static/etc. to say "do drupal_goto before page is rendered", that way places that expect to find the overriden destination in $_GET['destination'] will work as expected. (if their own drupal_goto is done before the page is rendered, etc.), instead of having two places to check. Shrug.

Marked as duplicate #337442: event not saved when a trigger assigned

Just a reroll of #181 - as I said in #176, I removed the unrelated coding standards changes from the patch and split them out into #870620: Trailing whitespace in common.inc and #870626: Extraneous spaces in trigger.test..

This one should apply.

_drupal_bootstrap_page_header has an ob_start and the shutdown sequence shows that shutdown functions are ran before output buffers are flushed. There is an implicit ob_flush in drupal_page_footer but one wonders what's the purpose of that? If we would omit that then my code would work.

#184: system_goto_action_184.patch queued for re-testing.

Seems like we stopped at 99%. Still a major problem. Any chance to resurrect this issue?

Well, there might be multiple ways to implement this, but #184 looks good to me + is ready. So let's let the bot re-test it.

@#178: You cannot break the batch with the action, as it checks for it. But yes, you can break the batch by using a delayed drupal_goto. However, you can do so with an instant drupal_goto too. But note, still the API functions work as expected; it's just not expected that anyone uses them during the batch just as it is not expected that anyone calls an instant drupal_goto() during a node_save() operation.
There are always ways you can break the expected behaviour, in the end we cannot prevent that. So I don't think we have to prevent it for that case either.

As to #143 fixing tag to API addition.

#184: system_goto_action_184.patch queued for re-testing.

Re-adding the security tag.

subscribe

#184: system_goto_action_184.patch queued for re-testing.

Hrm, better separate tags.

suscribe

#181: goto_2.patch queued for re-testing.

Just marked #1227548: Redirect does not work with User Login block as a duplicate. I'm +1 on the 'delayed' and 'skip destination' options. These also make sense for some third party authentication methods where you need to be sure to redirect to another login or logout page regardless of if $_REQUEST['destination'] is set.

#184: system_goto_action_184.patch queued for re-testing.

Could this be the reason why, in D6, if I set a redirect to trigger on new account creation, the redirect occurs but then the user is never logged in?

Thanks in advance,
Julie