Zend Framework

This is a more serious bug than perhaps it has appeared, as it prohibits the use of ActionStack in many cases. It's a trivial fix (it seems) and would be good to solve.

Another (possibly simpler) solution would be to replace the ActionStack::forward($next) method with
public function forward(Zend_Controller_Request_Abstract $next)
{
$this->setRequest($next);
}

Follow the comments of the issue http://www.nabble.com/Menu-in-ZF%3A-Controller-Plugin-with-postDispatch-method-td23306086.html#a23315051
The problem described there seems to be related.

Added code tags.

Please don't change the behaviour of _forward() ! There are some use cases where it is explicitly needed that you have the parameters still available!

But I would like to see something like this:

/**
 * @param string $url
 * @param bool $reset      | reset the params in Front Controller
 */
protected function _forward($url, $reset = false)

This would not break BC!

I would like to submit a patch for this issue; not sure the best place to discuss it, please let me know if these comments are not the appropriate place.

I think that BC should be broken here. The ActionStack::forward method should use the exact request object passed to it when it was created, not some frankenstein object of all the user params set in previous actionstack actions. If an actionstack action2 relies on parameters set in a previous actionstack action1, then action1 should add action2 to the actionstack.

Current behaviour is:

public function indexAction()
    {
        $request = clone $this->getRequest();
                $request->setActionName('test2');
                $this->_helper->actionStack($request);

        $request = clone $this->getRequest();
                $request->setActionName('test1')
                        ->setParams(array('bar' => 'baz'));
                $this->_helper->actionStack($request);
    }

    public function test1Action()
    {
        $this->getRequest()->setParam('fromTest1','shouldNotBeInTest2');
        Zend_Debug::dump($this->getRequest()->getParams());
//outputs  ["bar"] => string(3) "baz", ["fromTest1"] => string(3) "shouldNotBeInTest2"
    }

    public function test2Action()
    {
        Zend_Debug::dump($this->getRequest()->getParams());
//outputs  ["bar"] => string(3) "baz", ["fromTest1"] => string(3) "shouldNotBeInTest2"
    }

I believe the rational & desired behaviour is for test2 to not have any parameters set when it is called like this. If the above is the desired behaviour, then it should be coded like:

public function indexAction()
    {
        $request = clone $this->getRequest();
                $request->setActionName('test1')
                        ->setParams(array('bar' => 'baz'));
                $this->_helper->actionStack($request);
    }

    public function test1Action()
    {
        $this->getRequest()->setParam('fromTest1','shouldNotBeInTest2');
        Zend_Debug::dump($this->getRequest()->getParams());
//outputs  ["bar"] => string(3) "baz", ["fromTest1"] => string(3) "shouldNotBeInTest2"


          $request = clone $this->getRequest();
          $request->setActionName('test2');
          $this->_helper->actionStack($request);
    }

    public function test2Action()
    {
        Zend_Debug::dump($this->getRequest()->getParams());
//outputs  ["bar"] => string(3) "baz", ["fromTest1"] => string(3) "shouldNotBeInTest2"
    }

What are your thoughts on breaking BC here? I have a patch for this ready if it's acceptable. I don't believe that this optional reset parameter suggested by Dennis is viable, as _forward is not a user-callable method.

A long time has passed since I add a comment to this issue.

After re-reading everything, it seems like I have not seen the "clone" statement in the description. I'm not sure if there exists a reset() method for the request-object, but I still would prefer another way.

There isn't a reset() method for the request object, but this isn't the main problem. The problem is that ActionStack::forward does not call any putative reset() method; it adds parameters to the request object using setParams() (which doesn't actually set the parameters - it adds them to existing parameters). Changing this breaks BC, but as far as I can see, it's the only way to prevent unwanted parameters being set in later ActionStack actions.

What is your use-case for building up parameters through successive ActionStack actions?

It is possible, that you set a value per setParam() which will be used by more than one Action in the later use.

My main missunderstanding is the context, where you want to change that. I thought about the Action-Controller available _forward() method and as I can see, I was wrong here. The main reason why I'm still against a reset of the values is, that no one would expect that by cloning an object! For me a clone must be an exact copy, not a half copy. You never know who uses this behaviour at the moment. So I think a reset() or resetParams() method seems to be a better way instead of implementing a functionality no one would expect when using it.

I'm glad to see this issue is still alive. I'm still 'patching' the ZF with my 'solution' in the original post of this issue. As I see it now, it is clear that the request object needs to have 2 separate methods to modify or add parameters:

modify (reset) parameter(s):
Zend_Controller_Request_Abstract::setParam()
Zend_Controller_Request_Abstract::setParams()

add parameter(s):
Zend_Controller_Request_Abstract::addParam()
Zend_Controller_Request_Abstract::addParams()

Dennis: absolutely. No-one's suggesting that the clone does anything other than clone the request object; only that, given 2 actions in the actionstack, each with their own request object, action1 should not magically change the parameters for action2's request.

Stijn: I agree this is by far the best solution. However, this seems like a big BC break; any input from the ZF team on this?

This is a big BC break, as the parameter munging is a specific use case for which ActionStack was designed.

I think we could introduce a flag to ActionStack to indicate that the request object user parameters should be cleared, but this is really the only concession I'd like to make at this time.

Fair enough.

Out of interest, what is the use case for using ActionStack to build up parameters which isn't better served by the code I outline above, where test1Action adds test2Action to the ActionStack?

Patch & test done.

I've added a clearParams() method to the request class, and a setClearRequestParams() method to the ActionStack class, which controls this behaviour. It defaults to false, so bc is not broken - I think this is confusing, and that bc should be broken here.

I'd still really like to see a robust defence of the current behaviour, this solution seems sub-optimal to me.

I'd also be interested to know why the request setParams() actually adds parameters rather than setting the parameters, and why it's not implemented as Stijn suggested, which is the root cause of this confusion.

One reason that the user params are not cleared on _forward/ActionStack calls is due to how other components, such as ContextSwitch, do their work. Imagine a scenario where ContextSwitch is in play, and a JSON context is detected because of the "format" parameter found in the user params (i.e., set by the router); if you were to clear out the params, then additional actions invoked would no longer know the current context, and would be appending HTML to what should be a JSON response.

I am reviewing the patch for inclusion; as long as the behavior is controlled by a flag and that the default state of the flag is the current behavior, we should be able to include it.

Patch applied in trunk and 1.9 release branch