Design of new action parameter feature

This new feature is to be available in 1.1.4. More details are described in http://code.google.com/p/yii/source/browse/trunk/docs/guide/basics.controller.txt#146

phpdoc approach

As described in the above tutorial, we are relying on method phpdoc to determine the type of the action parameters. For example, the following code will say that $category is integer and $language is string.




    /**

     * Creates a post.

     * @param integer $category the category ID of the new post

     * @param string $language the language code of the new post

     */

    public function actionCreate($category, $language='en') {...}



Pros: the parameter types are clearly specified and easy to read and tell.

Cons: It’s not DRY. Some accelerators may strip off comments, making it not working;

default value approach

We are thinking another alternative approach of specifying parameter types, i.e., using the type of the default value of the parameter. Using this approach, the above code can be rewritten as:




    public function actionCreate($category=-1, $language='en') {...}



Pros: It is very dry.

Cons: It is a bit automagic. Sometimes we probably don’t want to set a default value.

Note that PHP doesn’t support simple type hinting like the following:




    public function actionCreate(int $category, string $language='en') {...}



We are not quite sure which approach we should choose. Each approach has its pros and cons. Please help us make a decision by casting your vote. Thank you!

why not:




   public function actionCreate($category, $language='en') {

      $category= (int) $category;

      $language = (string) $language;

      //...

}



??

PoL… how would you know that $category should be int?

the default value is not good IMO, what with complex types like objects, arrays?

@PoL: the type checking is enforced by Yii. If you are willing to manually do type checking, then there is no problem.

We only support simple types, including int, float, boolean and string. Note that these parameters are from $_GET.

PHP does allow complex type hinting, even though we don’t need it here.

How about something different?




public function actionCreate(array(

    'category' => array('type' => 'integer'),

    'language' => array('type' => 'string', 'default' => 'en'))

{

   ...

}



The above definition is clear, explicit and yii can automatically create the local variables $category and $language using the dereferencing expression ($$var).

I see more problems than benefit with this feature. Here i’ve described an alternative:

http://www.yiiframework.com/forum/index.php?/topic/10720-model-binders/page__view__findpost__p__56049

EDIT:

I’d like to know more about the rationale behind this feature: What is the great benefit? As i’ve stated this feature relies on Reflection. Something that should be avoided by any means unless there’s a real necessity. I don’t see this necessity in this case.

Yeah, I agree with you mike. A bunch of "smart" helpers would solve the problem.

I especially don’t like the phpdoc approach, it tries to simulate a standard code annotation approach (which isn’t?). Also isn’t the danger that some code optimizers discard the comments etc…

I’d rather prefer something explicit as I proposed or as Mike proposed. But if in the end it has to be with real parameters in action method declaration then I’d rather go with “the default value approach”.

One more approach is to remove type conversion at all and leave just parameters binding and default values since parameters are mostly used with AR and AR internally already works fine with strings. This way we’ll totally avoid PHPDoc annotation losing not very useful part of the feature (type conversion) and leaving all the good parts.

Well it looks like most people commenting here are against phpdoc. But votes are overwhelmingly for phpdoc…

More and more I think about using phpdoc it feels like a hack to me.

More cons:

  • You can’t minify the source code as much as before anymore as comments always have to stay

  • Facebooks HipHop is probably locked out with this feature

Changed my vote to “I don’t care (I won’t use this feature)”. Should be read as “I’m against type casting”. Too many cons for me.

I don’t see the real benefit and prefer handle type checking myself.

(I agree with Mike).

8)

Raoul

Why voting for "the default value approach" then?

@Mike: That’s why I propose the “default value” approach because the comment could be stripped off and the approach is not DRY.

Anyway, this action parameter (including type checking) is an optional feature.

Regarding reflection, we already used in CWidget and CWebService. My preliminary benchmark didn’t find any obvious performance impact when using reflection to implement this action parameter feature. If you could do more test, it would be better.

Just some more notes thrown into the discussion:

In my understanding Yii tries to utilize PHP’s built-in features where it can. I admit i never used it anywhere, but since 5.2.0 the Data Filtering extension is by default installed with PHP. It provides helpers for validation and sanitization and filtering external input. So, maybe another alternative would be to provide a lean wrapper component around these helpers.

Another idea of how to declare input types could be a method like Controller::inputs() to return the mapping of actions to their input types.

Finally i found one more problem with having defined action parameters:

If i understand right, the signature of input parameters would be static for an action, right? So you could not have an action, that requires different sets of input parameters in $_GET anymore. This is something i use a lot in my applications. Even though it might not be the cleanest approach, it sometimes results in less code.

In your last problem, you can simply define the method as you currently do without any parameters.

Using Controller::inputs() makes the code hard to read because now code relevant to actions are scattered in two places.

Data filtering is a different topic…

Initially, I was all for this feature, but now I’m asking myself: “What problem does this solve?”

I saw PHP’s Data Filtering related because it also provides helpers for obtaining input parameters and validating their type. Now i see that CInlineAction actually does not validate the type but only automatically type juggles. So right, not really related.

Even though i see the convenience of having action parameters automacitally available inside the action, i think the automatic type juggling goes a bit too far. It only saves 1 single term (like "(int)" or "(bool)") one can easily put before a variable manually. So not worth the effort of trying to guess the right type.

Besides that i think CInlineAction should not access $_GET directly. I’d not search for that here. Why not provide an easy to use input helper in CController that can be used by both: The automatism fans and the developer who wants full control over parameters himself?


public function actionList()

{

    // Would throw an exception if $_GET['category'] is not set:

    $category= (int) $this->input('GET','category',null,true);


    $articles=Arcticle::model->findByAttributes(array('category'=>$category));

}



So again, how about a controller method like this:


/**

 * Fetch an input parameter from GET, POST or COOKIE.

 *

 * If $isRequired is true the parameter is mandatory and an exception will be thrown if

 * if it's not supplied. By default $isRequired is false. If the parameter is absent

 * and not mandatory, $default will be returned.

 *

 * @param string $type either GET, POST or COOKIE

 * @param string $name Name of input parameter

 * @param mixed $default the default value if isRequired is false and parameter is absent

 * @param bool $isRequired  wether this parameter is required

 * @access public

 * @return string the value of the input parameter

 */

public function input($type='GET',$name,$default=null,$isRequired=false) {}



Because I’m not dogmatic

Actually, this is my favourite, it’s clean and explicit.

The default value approach is decent but I think it’s not so clean and it’s too much automagic.

The phpdoc approach is the worst, it would be nice but we shouldn’t rely on comments.