Won't fix \XF\ControllerPlugin\Error::actionNoPermission required parameter

[Lukas W.]

Affected version

2.2.8 PL 1

\XF\ControllerPlugin\Error::actionNoPermission labels $message as required parameter, however the code flow supports the parameter to be set as false or null and will fallback to a default message in those cases. Strictly speaking not necessarily a bug I guess, but would be nice to declare the $message parameter as null by default to avoid having to explicitly pass a parameter.

 

[RisteDimitrievski]

public function actionNoPermission($message){}

Change to:

public function actionNoPermission($message = NULL)

As far as i can see in the function itself it's support if message is not specified but forgot to put in function parameter to set as optional.

if (!\XF::visitor()->user_id && $this->app instanceof \XF\Pub\App)
        {
            return $this->actionRegistrationRequired();
        }
        else
        {
            if (!$message)
            {
                $message = \XF::phrase('do_not_have_permission');
            }

            return $this->error($message, 403);
        }

 

[Chris D]

On further consideration, I don't feel it's worth blowing up backwards compatibility for this, not even for a major release like 2.3. Just pass null, false or an empty string if you need to force it.

Worth noting too that in all cases as you should be in a Controller context already to call a controller plugin, the cleanest way to do what you want is simply to call:

return $this->noPermission();

or

throw $this->exception($this->noPermission());

This is a method on the Abstract main controller class so is available to all type-specific controllers and doesn't have a required message parameter.

 

[Lukas W.]

I can't speak to the use I had for this a few months ago anymore, but using methods directly in the controller very likely wasn't available for a reason since otherwise I wouldn't have stumbled over this.

I'm not sure I follow the backwards compatibility argument. The signature would change, but any existing method calls would be perfectly compatible still. I'd take a wild guess that extensions to XF\ControllerPlugin\Error are fairly rare if any, so this doesn't strike me as major breaking change.