Redirector Controller index()

[alex-dna]

Hi,

Having the $request in the params of the index() method on SilverStripe\CMS\Model\RedirectorPageController is creating challenges with other modules, including CWP search recipe (CwpSearchPageController).

The issue occurs when on a custom PageController, the index() is also defined without the $request param. We then get a warning: Warning: Declaration of SilverStripe\CMS\Model\RedirectorPageController::index(SilverStripe\Control\HTTPRequest $request) should be compatible with PageController::index() in /Users/alexandre.saelens/Sites/cifamstart/vendor/silverstripe/cms/code/Model/RedirectorPageController.php on line 39

In general, the index function doesn't expect any params to be passed, as being on the controller, the $request can be retrieve with $this->getRequest();

Would it be possible to use this instead of the param to avoid further incompatibilities?

Thanks. Alex

[maxime-rainville]

Can your clarify the scenario in which you got a warning thrown at you?

While I agree that our controller action methods are somewhat inconsistant in explicitly requiring a $request, I don't see how we could drop passing it as an argument without causing a lot of headaches for people who are expecting it to be there.

I'm not sure what the original rational for this approach is. My guess is probably that it was meant to be flexible for developers when defining controller action. Basically, it's not uncommon to reply to a request without interaction with the actual request object, so you get the option to omit that parameter when defining your action method's signature.

[djmattski]

@maxime-rainville The issue is that the index function declared in the RedirectorPageController class (which extends PageController) defines a $request parameter.

So if in custom code on the PageController class we define an index function without the parameter - we get the error above that @alex-dna has mentioned. It then becomes a rock and a hard place scenario as if you do define it in PageController then you have the opposite issue with the CwpSearchPageController class that does not define it in its index function.

So the lack of consistency means an issue either way.

[maxime-rainville]

The problem is that we have some Page Controller sub class that defines an index action with a $request parameter and one without a $request parameter.

These two implementations can co-exist as long as the project's PageController class doesn't implement an index method. If PageController ends up implementing an index method, then you'll get a PHP warning because one of the PageController subclass doesn't match the definition of its parent.

The following controllers expect the $request parameter.

• RedirectorPageController
• BlogController
• TaxonomyDirectoryController

The CwpSearchPageController is the only one that doesn't expect a $request parameter.

Which ever way we fix this, we need to have a semver breakage somewhere. My instinct would be to update CwpSearchPageController to expect a SilverStripe\Control\HTTPRequest $request. If someone, has extended that class and overridden the index action, they'll get a PHP warning.

On top of that, we could update the installer so the default PageController class ships with an index action with the expected signature. This could be a way to "nudge" new projects in using the correct signature for index action.

Alternatively, if we want to be a bit more aggressive, we could define an index(HTTPRequest $request) on ContentController. That would force anyone that has created a subclass of PageController with a parameterless index action to update their controller to expect a $request parameter. This is a semver breakage although one that's pretty easy to remedy.