Yii 2 began its life long before PHP 7’s scalar type. Since 7.1, I like to declare type on all method parameters in application code. It’s a habit. But when you declare scalar type on an action method, something nasty happens. I’d like to discuss what we want to do about it in Yii 3.
If controller class Foo has strict type and public function actionBar(int $id) then request /foo/bar?id=baz returns 500 server error. That’s wrong. What should happen instead?
Perhaps the framework should help. It could reflect on the action method before calling it, check the input string to see if it is an int. If so, convert it, otherwise return a 400 with a suitable error message. This is functionally a validation and it makes me uncomfortable.
Validation belongs business logic but this would be validation implied by the type of params of methods with a name starting with “action” in classes extending Controller. The implicit validation runs after filters but before the action. If the app wants something different from this validation the framework provides, e.g. a more specific error message, it can:
catch the exception in its own bindActionParams() method, which becomes a real mess,
drop the type declaration,
not use action param binding.
I think an app should always be explicit in specifying its validation. Using a type declaration to specify a default validation that the framework provides is dangerous. We ask too much of programmers to understand and remember the special meaning of a type declaration in action method params. Such implicit validation is a foot gun for people a habit of declaring type.
I think the best way forwards might be to drop action param binding altogether from web cntrollers in Yii 3. It’s a bit too magical and it encourages sloppy programming. Otoh, it’s real handy in console controllers.
Am I right that the only thing you feel wrong right now is that you cannot customize error code and error message for the case when parameter type doesn’t match?
No. I also don’t like when a framework overrides the meaning of a token in the programming language, but only in certain contexts, and does something that would normally belong to business logic. For some users, this will be a surprising, unexpected change in the meaning of a PHP type declaration.
I think that’s a foot gun. It’s likely that some users will discover the framework’s hidden validation functionality for the first time in production.
To express the same concern in different terms, this runs against our preference to be explicit. The validation is hidden away in an unusual location, bad enough, and it is triggered by a PHP token that has nothing to do with validation.
I don’t agree. Type hinting is, in my opinion not input validation. Just as mapping a URL to a route is not validation.
Regardless of scalar type hinting / coercion additional validation will always be needed.
For me input validation is something I do when the user enters information. A link containing a numeric ID is generated by the system, and while it is verified it is not user input. The correct response in such cases is something like a 404 / 400 depending on the alteration made by the user.
As a developer type hinting in the action just means that I can rule out certain edge cases where type juggling normally would require extra work.
In my opinion we should support scalar type hinting and return 400 in case the type coercion fails.
When you have done a typo on a URL, for example typed a wrong URL http://my.host.com/some_action/ in place of http://my.host.com/some-action/, you won’t call this 404 error as a “validation error”. It is just the same for the case of /some-action?id=foo and /some-action?id=1. And virtually these errors are caused always by the developers, not by the end users. They are coding errors in fact. IMO it’s not appropriate to discuss this kind of error in terms of “validation” and/or “business logic”.
On the other hand, the fact that the same method signature can have a different meaning only in action methods can be a problem as @thefsb says, although it’s the same for the console app.
Personally I want the action parameter binding feature to be kept included in Yii 3, because it’s handy.
The difference is manual input (form fields) vs generated input (clicking on a link).
Even without type hint, with your reasoning, checking the existence of a param is input validation…
I think we should define the terms more clearly:
Request validation: check basic validity of the request (route, method, param types).
Input validation: check validity of data supplied directly by the user.
Here, request validation is not part of the business logic. Yii can do this in it’s routing, before executing the action. Input validation is then done in the action / model.
I would call that status 404 because no resource at /some_action was found.
I don’t think that’s the same at all. The resource was found but the query parameter has an invalid format. That’s a bad request and hence 400 status.
I think you are looking at this from the point of view of how the server software works whereas I am looking at it from the point of view of the client using a protocol.
Again I disagree. If id=1 is valid and id=foo is not valid then the client must change the request not the developer fix a bug. So the server should send 400 with a message explaining what’s wrong so the client can fix it.
The cause of a request with an invalid query param (e.g. bad link or buggy API client code) is beside the point here. The server should in any case observe the protocol spec.
I have a lot of experience with buggy client apps using my APIs. A recent case led to the issue Samdark linked above. The API client sometimes sends "undefined" to an endpoint that expects an int for that param, \yii\web\Controller::bindActionParams() calls an action method, PHP raises a fatal type error and the error handler sends 500 to the client, a critical to Rollbar and logs an error.
That’s clearly the wrong response the request. So how do we fix this bug? Danil felt the right solution was to fix it in \yii\web\Controller::bindActionParams(). Use reflection to get the type declaration and if it’s an int check the input string format, if invalid, send a 400 with an error message.
I don’t like it. The fix checks input strings for certain formats and sends an error to the client if the format is wrong. That’s validation. And it’s not the job of the framework.
Danil and I considered another way forwards. Keep action parameter binding but require the controller dev to specify behavior if type is declared on a param.
The user would do this by overriding \yii\web\Controller::actionParamRules(): array. The method returns something like ['type' => [\Closure testFunction]]. The test function returns a message string in case of an error.
If an action declares a parameter type then actionParamRules() looks for a suitable rule. If there is one and it returns an error message then actionParamRules() throws a bad request exception. Otherwise it calls the action in a try block, catching a \TypeError and throwing some new Yii action param type exception that ends up as a 500.
In this way, Yii
communicates to devs that there is a complication with parameter types on action methods
provides a way for the app to take control of error cases
I’m not really happy with this approach because it adds so much complexity. It’s a clunky way to put control back into the app’s domain.
If nobody had heard of action param binding before and somebody clever were to propose it for Yii 3 then this complication with type declaration would be enough to reject the idea. Action param binding is a shortcut that nobody really needs. It can be nice but our code is more explicit when we don’t use it.
