Add getParams and setParams to Magento\Framework\App\RequestInterface

[ashsmith]

I have noticed in multiple places that getParams() is expected to be set on the request object, despite the interface not requiring the getParams() method. My understanding is that we're using DI and specifying the Interface, it means we shouldn't rely on methods not in the implementation interface.

Adding getParams() to Magento\Framework\App\RequestInterface seems like the right thing to do, and is a common method used by not only the core of Magento, but by extension developers too. I wouldn't feel comfortable using this method knowing that I can't rely on it existence.

More than happy to fork, and submit a pull request but it requires a fair bit of work - so before I commit to that I'd want approval :)

By searching for $this->getRequest()->getParams() I was able to find 20 occurrences, 18 of which happen with app/code/Magento/*. This is just for controllers, I'm sure there are instances in helpers and models where getParams from the request interface is used.

Thanks,
Ash Smith

[antonmakarenko]

Makes sense.

I believe this interface is meant to be an abstraction layer to Zend_Controller_Request_Abstract (which doesn't happen to have an interface). It has the getParams() and a setParam() methods -- I'd recommend to add both, because they are used in the client code and because there are similar counterparts in this interface (getModuleName()/setModuleName(), getActionName()/setActionName())

[benmarks]

👍

[kandy]

I believe that is no reason to add a method to interface when it used in some places, because it contradicts the Interface segregation principle.

Better to define dependencies on real interfaces and never use method not from interface. So better to define new interface Magento\Framework\Request with getParameter and extend Magento\Framework\App\RequestInterface from this.

Also I propose to split interfaces on mutable and readonly part. For example PSR proposal for http message do not have setters for parameters.

[punkstar]

@ashsmith 👍

@kandy 👍 for specialised interfaces. IMO the getParams() method would be a sensible addition to base request interface as it would be useful for absolutely every implementation of a request. Any interface that has a getParam($name) should have a getParams(), so that I may discover available parameters if I don't know a $name.

[vpelipenko]

Internal ticket: MAGETWO-32340

[TexanHogman]

@ashsmith After a review of your proposal a pull request would be accepted. Having getParams() missing is a mistake causing us to go around the defined interface. Thanks for the suggestion and your help is appreciated.
Let's limit scope to adding these two methods to the interface and address implementation classes. While I agree with @kandy on splitting interface across get and sets I am concerned to the amount of change this will create with a PR so let's hold off on that change.

[ashsmith]

@TexanHogman good stuff, I'll submit the PR tomorrow then! :)

[vpelipenko]

Closed. PR is merged.