Zend Framework

Fixing formatting...

You are right, personally i think redirector action helper has to be refactored to skip the dispatching look somehow and call the redirect and exit after that, so that it behaves equally in the enviroments.

Maybe it's possible to do something with an exception? That is, instead of exiting, a third return technique is added to the redirect helper (on top of "continue" and "exit) so that in test mode it throws a custom exception from the redirect helper to effect the jump. This exception could be caught somewhere in test harness. It's a nasty fix though...

I am also experiencing this problem. I have a authentification control in preDispatch()
Everything is working fine in production, but when I'm executing the tests, my action is still triggered.
I am currently using ZF 1.10.8

How to easily repeat it :

 class MyController extends Zend_Controller_Action
{
    function preDispatch() {
        return $this->_redirect('/'); // I am using 'return' :)
    }

    function indexAction() {
        var_dump('This should never be executed');
    }
}

 
string(This should never be executed)

Note: using return $this->_redirect('/', array('exit' => true)); is worse, as phpunit is also terminated.

Another vote to have this bug fixed.

I also have problems with Zend_Test and my ACL plugin.

Unfortunately, as almost all of my applications uses preDispatch method, Zend Test becomes very limited as i have to keep checking if a test fail because of this bug or a real bug in my app.

Here's my workaround, but it's not pretty!

In the preDispatch method of my plugin, instead of issuing a redirect, I rewrite the request to specify a custom "plugin-redirector" action, setting the location of the redirect as a param.

$request->setModuleName('default');
$request->setControllerName('index');
$request->setActionName('plugin-redirector');
$request->setParam('location', '/foo');

The action is called which then performs the redirect (note the return):

function pluginRedirectorAction()
{
    $location = $this->_getParam('location');
    return $this->getHelper('Redirector')->gotoUrl($location);
}

As far as I can tell, this exhibits the same behaviour in production and testing.

Having the same issue, any news on this ?

This problem isn't just limited to preDispatch.

I am using a routeShutdown hook, for language detection.

And I redirect to a url witch contains the url.

Like: /nl/controller/action

This isn't detected by zend_test either!

I found out that it works sometimes. But for some reason, when the error controller is used, it isn't detected as a redirect.

So when i go to /abc (this doesn't exists, so it produces a 404 page), it redirects to /nl/abc. But that isn't detected! But when i go to /index, which redirects to /nl/index, it ís detected properly in zend_test.

Hi Guys,

I've developed a patch which seems to fix the behaviour for me (I'm only using redirects in preDispatch methods). We might need to tweak it a bit to work with routeShutdown etc. All I'm doing is 'returning early' in Zend_Controller_Action::dispatch() if the preDispatch method has turned the response into a redirect.

The Zend_Controller_Action & Zend_Test_PHPUnit_ControllerTestCase suites continue to pass as expected. My application's test suite now passes correctly as well.

!trunk_ZF-7496.diff!

Regards,

Rob

Hi Rob,

Thanks for the patch to contribute to ZF. However, before contributing code you should sign the cla (http://framework.zend.com/wiki/display/ZFPROP/Contributor+License+Agreement). After then you can attach files.

Oh I see,

I tweeted Matthew asking, but he hasn't replied yet.

I'll go sign the CLA.

Regards,

Rob

Rob - Have you considered that it may be bad practice to do redirecting from preDispatch() or init() etc.? See what I have commented in ZF-5619. Thoughts?

Rob - Have you considered that it may be bad practice to do redirecting from preDispatch() or init() etc.? See what I have commented in ZF-5619. Thoughts?

Hi Kim,

No I haven't. Why is it bad practice? For example if I wanted to check if a user is not 'logged-in' then deny them from the rest of the controller actions, what should I be doing? Ideally I don't want to repeat code in every controller action to check whether the user is logged in or not. It would be preferable to place this logic in one location and automatically deny access to all of the controller actions.

Rails offers a similar feature in their 'before_filters'. You can create a filter with redirects if required. Please refer to: http://guides.rubyonrails.org/action_controller_overview.html#filters.

Regards,

Rob

Okay,

I've signed the CLA, please find my patch attached.

Regards,

Rob

Rob,

I'm refering Matthew (ZF-5619)

The primary issue I'm seeing right now is actually in your code. When you call _forward() or one of the _redirect() or redirector methods, you should always return immediately; if you do not do so, the action continues to execute, which can lead to the problems you're describing.

and Olek (ZF-7455)

If I redirect from an action helper in init or preDispatch, I want the action controller not to even call the action. Unfortunately, this is not the case, and the regular action logic like rendering happens as well.

So have you tested that if you are calling redirector in preDispatch() and return there immediately, does it prevent action to be executed?

So have you tested that if you are calling redirector in preDispatch() and return there immediately, does it prevent action to be executed?

Yes I have. The problem is this works in production, but if you are using Zend_Test_PHPUnit_ControllerTestCase to unit-test your controllers it won't work. This is because the setExit method on Zend_Controller_Action_Helper_Redirector is set to false, so performing a redirect won't call PHP's exit function - hence Zend_Controller_Action::dispatch() will call the controller action.

Does that make sense?

Regards,

Rob

Ok then the patch looks good to me. Nice work, thanks.

Ok then the patch looks good to me. Nice work, thanks.

Kim,

Keep in mind we might need to play with it a bit to cater for some of the other issues the guys have mentioned above. e.g. I'm not sure if it fixes redirects in the routeShutdown hook for example as I didn't test for this.

It should be trivial to add another test though. I'll do it if I get some time.

Or alternatively maybe we should open separate issues for these hooks? What are your thoughts?

Regards,

Rob

I'm not sure if it fixes redirects in the routeShutdown hook for example as I didn't test for this.

But that's the plugin method and thus beyound the issue addressed here? Though I could also test this in next week.

edit:

Or alternatively maybe we should open separate issues for these hooks? What are your thoughts?

Yes.

Oh Cool,

I think that's it from me then on this issue.

Regards,

Rob

I've attached a modified version of Rob Morgan's fix and additional unit tests to support (ZF-7496.patch).

The modification I made was to allow the post-dispatch hooks to still run even if a redirect was invoked in the pre-dispatch hooks.

The provided unit tests show that the provided fix works for all pre-dispatch hooks (init, routeStartup, routeShutdown, dispatchLoopStartup and preDispatch)

Adam - tested and working as expected from controller plugin hooks and from controller action hooks.

I've run the patch through QA for our sites which use redirector in the affected locations and no adverse effects were observed.

Hey Adam,

I've successfully run your patch and the updated unit-tests without any problems.

I've also run the patch against one of my sites and it works as intended.

How do we go about getting this patch merged into ZF trunk?

Regards,

Rob

The consensus seems to be that the fix works as expected, so i've committed it to trunk and merged into release.

Fixed in trunk r24252
Merged to release-1.11 in r24253

Not sure how this fits in the scenario, so i'll explain and let you judge.

I have a test that does the following actions:

1. Build dependencies
2. Calls a dispatch on url X to setup dependencies (this url does a header-location redirect)
3. Dispatches url Y
4. Checks result.

Since this commit was done this case is now failing and fails to ever execute the action in Url Y. I might be doing something wrong, i should probably clear the dispatcher after the first call but i'm looking into how to do this.

I just wanted to share the scenario so you guys can look at another usercase.

Cheers

@Rafael

Could you provide a short code sample illustrating your use case? The test code plus the code in the action method for "url X" (specifically the redirect) should be sufficient to allow me to get a good handle on how to correct the issue.

One of the changes introduced for this issue's fix is that action methods are not executed after a redirect has been invoked, and this is where you're problem lies. We may want to revisit that decision, as it appears to break the pre-existing functionality you outlined

Reopened due to comments

@Rafael

I am interested to see your test code as well.

Regards,

Rob

Hey guys, i actually figured i needed to call a resetResponse between the dispatches to clear avoid the issue.

If you still want i can go fetch the code, but it was as i suspected my fault.

@Rafael
Could you still show the code where you build your dependencies and then dispatch to the final action? I will update the ZF manual to include a note advising users that they must use resetResponse in that situation, but I would like to make sure I understand the use case properly first. Thanks!

There's actually a note in the examples already (http://framework.zend.com/manual/en/zend.test.phpunit.html#zend.test.phpunit.examples), though we may want to pull that out as a separate, more explicit note prior to the examples.

@Adam

My case is very close to the example Matthew pointed to. We have a few code policies here which prevent me from sharing the whole code, but its nature is the same.

One call to log the user in, one call to test something with a logged user. So the response stays dirty from one to another, doing a reset solves the issue.

@Rafael, @Matthew
The fix for the issue has changed that facet of framework's behavior: you now have to resetResponse between redirects, which (AFAIK) you didn't need to do before. Would this be considered a BC break, or is it OK?