Fixes #1391: CDetailView: callables (including anonymous functions for PHP 5.3+) could be used as value generators of the attributes. #1405

Merged
merged 3 commits into from Jun 6, 2013

Conversation

Projects
None yet
7 participants
@resurtm
Contributor

resurtm commented Sep 14, 2012

Fixes #1391.

Testing code:

<?php $this->widget('zii.widgets.CDetailView',array(
    'data'=>TaskGroup::model()->find(array('offset'=>1)),
    'attributes'=>array(
        'id',
        'title',
        'create_time',
        array(
            'name'=>'update_time',
            'value'=>function($data) {
                return $data->id.', '.$data->title;
            },
        ),
    ),
)); ?>

<br/><br/>

<?php
class TestingClass
{
    public static function testingCallback($data)
    {
        return $data->id.', '.$data->title;
    }
}
?>

<?php $this->widget('zii.widgets.CDetailView',array(
    'data'=>TaskGroup::model()->find(array('offset'=>1)),
    'attributes'=>array(
        'id',
        'title',
        'create_time',
        array(
            'name'=>'update_time',
            'value'=>array('TestingClass','testingCallback'),
        ),
    ),
)); ?>
Fixes #1391: CDetailView: callables (including anonymous functions f…
…or PHP 5.3+) could be used as value generators of the attributes.
@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Sep 14, 2012

Member

I don't understand why we need this. Why not just directly assign the evaluation result to 'value'?

Member

qiangxue commented Sep 14, 2012

I don't understand why we need this. Why not just directly assign the evaluation result to 'value'?

@creocoder

This comment has been minimized.

Show comment
Hide comment
@creocoder

creocoder Sep 17, 2012

Contributor

@resurtm Very good enchancement!

@qiangxue Because sometimes there is some very complex logic to get result. For example:

<?php $this->widget('zii.widgets.CDetailView',array(
    'data'=>TaskGroup::model()->find(array('offset'=>1)),
    'attributes'=>array(
        'id',
        'title',
        'create_time',
        array(
            'name'=>'status',
            'value'=>function($data) {
                switch($data->type)
                {
                    case 'started':
                        return 'Started';
                    break;
                    case 'finished':
                        $time=User::model()->getLastTaskFinishTime();
                        return 'Finished at '.Yii::app()->dateTimeFormatter->formatDateTime($time);
                    default:
                        return "Unknown ($data->type)";
                    break;
                }
            },
        ),
    ),
)); ?>
Contributor

creocoder commented Sep 17, 2012

@resurtm Very good enchancement!

@qiangxue Because sometimes there is some very complex logic to get result. For example:

<?php $this->widget('zii.widgets.CDetailView',array(
    'data'=>TaskGroup::model()->find(array('offset'=>1)),
    'attributes'=>array(
        'id',
        'title',
        'create_time',
        array(
            'name'=>'status',
            'value'=>function($data) {
                switch($data->type)
                {
                    case 'started':
                        return 'Started';
                    break;
                    case 'finished':
                        $time=User::model()->getLastTaskFinishTime();
                        return 'Finished at '.Yii::app()->dateTimeFormatter->formatDateTime($time);
                    default:
                        return "Unknown ($data->type)";
                    break;
                }
            },
        ),
    ),
)); ?>

@ghost ghost assigned cebe Sep 17, 2012

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Sep 17, 2012

Member

I also like it. in one application I created a formatter method for each special value type and placed that complex logic there. But when it is specific to one view only, annonymous function will be very helpful imo.

Member

cebe commented Sep 17, 2012

I also like it. in one application I created a formatter method for each special value type and placed that complex logic there. But when it is specific to one view only, annonymous function will be very helpful imo.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Sep 17, 2012

Member

Based on this argument, then every property of a widget should support anonymous function (even though most of them may not need it most of the time). I don't think this is a good thing.

The similar effect can be achieved without anonymous function in many different ways: reusable formatting methods, a piece of PHP statements locally, eval().

Member

qiangxue commented Sep 17, 2012

Based on this argument, then every property of a widget should support anonymous function (even though most of them may not need it most of the time). I don't think this is a good thing.

The similar effect can be achieved without anonymous function in many different ways: reusable formatting methods, a piece of PHP statements locally, eval().

@creocoder

This comment has been minimized.

Show comment
Hide comment
@creocoder

creocoder Sep 17, 2012

Contributor

@qiangxue For specific view there is no need to do reusable formatting methods. Can you show example how similar effect can be achieved without anonymous function for my example?

Contributor

creocoder commented Sep 17, 2012

@qiangxue For specific view there is no need to do reusable formatting methods. Can you show example how similar effect can be achieved without anonymous function for my example?

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Sep 17, 2012

Member

The only way I see would be something like this, but if you do that for multple columns code quikcly gets unreadable

<?php
    switch($data->type)
    {
         case 'started':
             $value1='Started';
         break;
         case 'finished':
             $time=User::model()->getLastTaskFinishTime();
             $value1='Finished at '.Yii::app()->dateTimeFormatter->formatDateTime($time);
         break;
         default:
              $value1= "Unknown ($data->type)";
    }

 $this->widget('zii.widgets.CDetailView',array(
    'data'=>TaskGroup::model()->find(array('offset'=>1)),
    'attributes'=>array(
        'id',
        'title',
        'create_time',
        array(
            'name'=>'status',
            'value'=>$value1,
        ),
    ),
)); ?>
Member

cebe commented Sep 17, 2012

The only way I see would be something like this, but if you do that for multple columns code quikcly gets unreadable

<?php
    switch($data->type)
    {
         case 'started':
             $value1='Started';
         break;
         case 'finished':
             $time=User::model()->getLastTaskFinishTime();
             $value1='Finished at '.Yii::app()->dateTimeFormatter->formatDateTime($time);
         break;
         default:
              $value1= "Unknown ($data->type)";
    }

 $this->widget('zii.widgets.CDetailView',array(
    'data'=>TaskGroup::model()->find(array('offset'=>1)),
    'attributes'=>array(
        'id',
        'title',
        'create_time',
        array(
            'name'=>'status',
            'value'=>$value1,
        ),
    ),
)); ?>
@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Sep 18, 2012

Member

You can also use eval(), which is almost the same as anonymous function except that it uses global scope.

I care more about code consistency. Why should the 'value' property be so special here? If we change the code so that 'value' can take anonymous function, shouldn't we do the same for similar properties in other widgets?

Member

qiangxue commented Sep 18, 2012

You can also use eval(), which is almost the same as anonymous function except that it uses global scope.

I care more about code consistency. Why should the 'value' property be so special here? If we change the code so that 'value' can take anonymous function, shouldn't we do the same for similar properties in other widgets?

@creocoder

This comment has been minimized.

Show comment
Hide comment
@creocoder

creocoder Sep 18, 2012

Contributor

shouldn't we do the same for similar properties in other widgets?

@qiangxue For what other widgets for example? From a practical point of view, the problem so far only in this widget.

Contributor

creocoder commented Sep 18, 2012

shouldn't we do the same for similar properties in other widgets?

@qiangxue For what other widgets for example? From a practical point of view, the problem so far only in this widget.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Sep 18, 2012

Member

For example, CButtonColumn.template or viewButtonLabel: developers may want to customize them based on some complex logic. In fact, virtually any widget property has similar need, although practically this is not common for most of them.

What I'm really against is the practice that we specially handle a property because it may receive some value obtained via complex value. IMO, this adds extra burden to widget developers and is a misuse of anonymous function.

Member

qiangxue commented Sep 18, 2012

For example, CButtonColumn.template or viewButtonLabel: developers may want to customize them based on some complex logic. In fact, virtually any widget property has similar need, although practically this is not common for most of them.

What I'm really against is the practice that we specially handle a property because it may receive some value obtained via complex value. IMO, this adds extra burden to widget developers and is a misuse of anonymous function.

@creocoder

This comment has been minimized.

Show comment
Hide comment
@creocoder

creocoder Sep 18, 2012

Contributor

@qiangxue I do not think we have any place where anonymouse functions was misused.

Contributor

creocoder commented Sep 18, 2012

@qiangxue I do not think we have any place where anonymouse functions was misused.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Sep 18, 2012

Member

I should use the term "abuse" rather than "misuse".

Member

qiangxue commented Sep 18, 2012

I should use the term "abuse" rather than "misuse".

@creocoder

This comment has been minimized.

Show comment
Hide comment
@creocoder

creocoder Sep 18, 2012

Contributor

@qiangxue I think there is two ways of solve problems:

  1. reusable formatters/widgets + specific Widget for specific view
  2. reusable formatters/widgets + specific Anonymous for specific view

So choose beetween Widget VS Anonymous

Widget

  • faster
  • need file to implement

Anonymous

  • no need file to implement
  • slower
Contributor

creocoder commented Sep 18, 2012

@qiangxue I think there is two ways of solve problems:

  1. reusable formatters/widgets + specific Widget for specific view
  2. reusable formatters/widgets + specific Anonymous for specific view

So choose beetween Widget VS Anonymous

Widget

  • faster
  • need file to implement

Anonymous

  • no need file to implement
  • slower
@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Sep 18, 2012

Member

First, we agree the problem here: we have some specific formatting code that is used only once, and we want it to be organized well.

My opinion is that asking the widget to modify the code to support anonymous function is the wrong direction, even though it does solve the problem. This should not be the burden on the widget developer.

As an alternative solution, why not consider using eval()? is it because it is evil? :)

Also, using anonymous function, you would have trouble if you need to access variables other than $data.

Member

qiangxue commented Sep 18, 2012

First, we agree the problem here: we have some specific formatting code that is used only once, and we want it to be organized well.

My opinion is that asking the widget to modify the code to support anonymous function is the wrong direction, even though it does solve the problem. This should not be the burden on the widget developer.

As an alternative solution, why not consider using eval()? is it because it is evil? :)

Also, using anonymous function, you would have trouble if you need to access variables other than $data.

@creocoder

This comment has been minimized.

Show comment
Hide comment
@creocoder

creocoder Sep 18, 2012

Contributor

@qiangxue

As an alternative solution, why not consider using eval()?

eval()

  • no need file to implement
  • make widget developer life easier
  • slower than anonymous

Also, using anonymous function, you would have trouble if you need to access variables other than $data.

We can use use keyword: function($data) use ($any)

is it because it is evil? :)

I think eval() is evil anyway :) Moreover i hope in Yii 2 and PHP5.3 we can totally forget this function, except places where anonymous cant be serialized,

Contributor

creocoder commented Sep 18, 2012

@qiangxue

As an alternative solution, why not consider using eval()?

eval()

  • no need file to implement
  • make widget developer life easier
  • slower than anonymous

Also, using anonymous function, you would have trouble if you need to access variables other than $data.

We can use use keyword: function($data) use ($any)

is it because it is evil? :)

I think eval() is evil anyway :) Moreover i hope in Yii 2 and PHP5.3 we can totally forget this function, except places where anonymous cant be serialized,

@creocoder

This comment has been minimized.

Show comment
Hide comment
@creocoder

creocoder Sep 18, 2012

Contributor

But i agree that problem solve with anonymous is wrong way. I think problem is deeper than we think. For example in some other non-php frameworks which use MVC approach there is special View-Model (model of view) layer. Perhaps the problem should be solved in more detail improvements in View layer.

Contributor

creocoder commented Sep 18, 2012

But i agree that problem solve with anonymous is wrong way. I think problem is deeper than we think. For example in some other non-php frameworks which use MVC approach there is special View-Model (model of view) layer. Perhaps the problem should be solved in more detail improvements in View layer.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Sep 18, 2012

Member

I think problem is deeper than we think.

I agree. We should think about a better and more systematic solution.

Member

qiangxue commented Sep 18, 2012

I think problem is deeper than we think.

I agree. We should think about a better and more systematic solution.

@creocoder

This comment has been minimized.

Show comment
Hide comment
@creocoder

creocoder Sep 28, 2012

Contributor

Anyway i vote to add support of anonymous function to CDetailView widget, because its total unusable without it for now. Because of this, I can not use this widget for a year or more. Why ? I want consistent code in my views. I use anonymous in CGridView and i do not want half-solutions for CDetailView. Will ask the question another way:

$this->widget('zii.widgets.grid.CGridView',array(
    'columns'=>array(
        array(
           'name'=>'title',
           'value'=>function($data)
           {
               //some complex presentation logic here;
           },
        )
    )
));
 $this->widget('zii.widgets.CDetailView',array(
    'data'=>TaskGroup::model()->find(array('offset'=>1)),
    'attributes'=>array(
        'id',
        'title',
        'create_time',
        array(
            'name'=>'status',
            'value'=>..., // eval(), prepared vars, renderPartial() maybe,
                   // or maybe generate html in some fat model method o_O??? no thanks i pass.
        ),
    ),
Contributor

creocoder commented Sep 28, 2012

Anyway i vote to add support of anonymous function to CDetailView widget, because its total unusable without it for now. Because of this, I can not use this widget for a year or more. Why ? I want consistent code in my views. I use anonymous in CGridView and i do not want half-solutions for CDetailView. Will ask the question another way:

$this->widget('zii.widgets.grid.CGridView',array(
    'columns'=>array(
        array(
           'name'=>'title',
           'value'=>function($data)
           {
               //some complex presentation logic here;
           },
        )
    )
));
 $this->widget('zii.widgets.CDetailView',array(
    'data'=>TaskGroup::model()->find(array('offset'=>1)),
    'attributes'=>array(
        'id',
        'title',
        'create_time',
        array(
            'name'=>'status',
            'value'=>..., // eval(), prepared vars, renderPartial() maybe,
                   // or maybe generate html in some fat model method o_O??? no thanks i pass.
        ),
    ),
@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe May 27, 2013

Member

I think for yii 1.1 we can allow annonymous function here. We should merge it or close it. I'm for merge as it is a small change that does not hurt anyone and also makes some people happy :)

Member

cebe commented May 27, 2013

I think for yii 1.1 we can allow annonymous function here. We should merge it or close it. I'm for merge as it is a small change that does not hurt anyone and also makes some people happy :)

@klimov-paul

View changes

framework/zii/widgets/CDetailView.php
@@ -206,7 +208,7 @@ public function run()
if(!isset($attribute['type']))
$attribute['type']='text';
if(isset($attribute['value']))
- $value=$attribute['value'];
+ $value=is_string($attribute['value']) ? $attribute['value'] : call_user_func($attribute['value'],$this->data);

This comment has been minimized.

@klimov-paul

klimov-paul May 27, 2013

Member

Check "is_string" can be unreliable.
Better to change the condition to be "is_callable()".

@klimov-paul

klimov-paul May 27, 2013

Member

Check "is_string" can be unreliable.
Better to change the condition to be "is_callable()".

This comment has been minimized.

@resurtm

resurtm Jun 6, 2013

Contributor

Sure, done.

@resurtm

resurtm Jun 6, 2013

Contributor

Sure, done.

@resurtm

This comment has been minimized.

Show comment
Hide comment
@resurtm

resurtm Jun 6, 2013

Contributor

Everyone agree to merge this?

Contributor

resurtm commented Jun 6, 2013

Everyone agree to merge this?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Jun 6, 2013

Member

OK for me.

Member

samdark commented Jun 6, 2013

OK for me.

resurtm added a commit that referenced this pull request Jun 6, 2013

Merge pull request #1405 from resurtm/1391-cdetailview-value-closure
Fixes #1391: CDetailView: callables (including anonymous functions for PHP 5.3+) could be used as value generators of the attributes.

@resurtm resurtm merged commit a26844f into yiisoft:master Jun 6, 2013

@robertelza

This comment has been minimized.

Show comment
Hide comment
@robertelza

robertelza Nov 2, 2013

Sorry for bringing it up again but in my recent project I had a little problem with this new feature. When I put a simple string in the value field (like 'virtual' or 'date' in the following code), PHP gives me this warning: "virtual() expects parameter 1 to be string, object given"

$this->widget('zii.widgets.CDetailView', array(
    'data' => $model,
    'attributes' => array(
        array(
            'name' => 'myName',            
            'value' => 'virtual'
        ),        
    ),
));

However when I replace that string with an anonymous function, the warning is gone:

$this->widget('zii.widgets.CDetailView', array(
    'data' => $model,
    'attributes' => array(
        array(
            'name' => 'myName',            
            'value' =>  function() {
                return 'virtual';
            },
        ),        
    ),
));

so my question is: Should we always use anonymous functions to avoid such situations? or maybe there is a better way?!
I'm using PHP 5.3.8

Sorry for bringing it up again but in my recent project I had a little problem with this new feature. When I put a simple string in the value field (like 'virtual' or 'date' in the following code), PHP gives me this warning: "virtual() expects parameter 1 to be string, object given"

$this->widget('zii.widgets.CDetailView', array(
    'data' => $model,
    'attributes' => array(
        array(
            'name' => 'myName',            
            'value' => 'virtual'
        ),        
    ),
));

However when I replace that string with an anonymous function, the warning is gone:

$this->widget('zii.widgets.CDetailView', array(
    'data' => $model,
    'attributes' => array(
        array(
            'name' => 'myName',            
            'value' =>  function() {
                return 'virtual';
            },
        ),        
    ),
));

so my question is: Should we always use anonymous functions to avoid such situations? or maybe there is a better way?!
I'm using PHP 5.3.8

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Nov 2, 2013

Member

I think we should change is_callable to instanceof Closure to allow only annonymous functions.

Member

cebe commented Nov 2, 2013

I think we should change is_callable to instanceof Closure to allow only annonymous functions.

@robertelza

This comment has been minimized.

Show comment
Hide comment
@robertelza

robertelza Nov 3, 2013

So I guess the answer is yes in the next release of Yii ;)

So I guess the answer is yes in the next release of Yii ;)

@resurtm

This comment has been minimized.

Show comment
Hide comment
@resurtm

resurtm Nov 4, 2013

Contributor

Lets continue discussion in #3010.

Contributor

resurtm commented Nov 4, 2013

Lets continue discussion in #3010.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment