Request/Router does not check required arguments for action.

[imunhatep]

Should throw an Exception. Check in Request::execute could be like that:

if(($action->getNumberOfRequiredParameters() > count($this->method_params)) or ($action->getNumberOfParameters() < count($this->method_params))){ throw new \HttpNotFoundException(); }

[frankdejonge]

This solution would require more than this. It also needs to take into account how many of those arguments are required.

[imunhatep]

Emm isn't this part of if statement solves that?

$action->getNumberOfRequiredParameters() > count($this->method_params)

[frankdejonge]

Ah, looked over that. I'll see what I can do :+1:

[WanWizard]

I don't think it's the task of the Request class to validate input. It's the task of the controller action to validate them, and handle validation errors (which in most of the cases is a custom error message or a redirect, and not a HttpNotFoundException).

[imunhatep]

It's up to you to decide where is better to implement such check in Controller/Request/Router/Route classes. I suggested most simple way to implement this check currently.

The problem is that Controller action won't even be called, as MethodReflection::invokeArgs will throw PhpErrorException. In fact there is no try-catch of this Exception.

Besides, it's pretty incorrect to route uri like "localhost/car/delete" to the Controller with action Car::action_delete($id). Technically, there is no such uri like "localhost/car/delete", there is "localhost/car/delete/123" uri, and such requests must be handled correctly by Router/Request i.e. with HttpNotFoundException.

Another question is whether this check brakes some installation.. well probably on first step fuelphp should write warnings to logs...

[WanWizard]

That depends on your application design. In most cases, you want to give the user a proper error message instead of NotFoundException. Which you can only do in your controller, which in turn makes required arguments a bad idea.

[imunhatep]

Ghmm, that is why they are called required. If Controller does not have action, request class throws httpnotfound. By your analogy such error should be resolved by controller either, but it's not. I don't see any reason why request class on missing controller action required parameters should behave differently.

[WanWizard]

I don't think I said I was against it (otherwise I would have closed this), I'm only stating that it's not very user friendly to handle these errors like this, and therefore most people use $argument = null, and handle the error in the controller.

The way you want it is never going to work, since every controller that extends a base controller will have a router() method, which means the number of arguments will always be 2, and they will always be present.

[imunhatep]

Indeed.. Sorry, you are correct, default method router was introduced since 1.6, in that case suggested check won't work. I use slightly modified version of fuel (1.6.dev), so I missed that.

Since router method does not use Reflection, check on required arguments count is harder to implement.. But router method approach gives possibility to catch HttpNotFound exception by controller calss.

Hmm, in current situation, probably, I would try to inject router method in controller instance in case it's not defined.. But need to think more on that, so it won't break anything...

[WanWizard]

Only for Controller and Controller_Template through. Controller_Rest (and it's derivative Hybrid) require a router() method.