yiisoft / view

Yii view rendering library
https://www.yiiframework.com/
BSD 3-Clause "New" or "Revised" License
56 stars 44 forks source link

yii\behaviors\CacheableWidgetBehavior cache widget problem #22

Closed rockman84 closed 3 years ago

rockman84 commented 6 years ago

What steps will reproduce the problem?

widget cache not working properly when your widget have registerJs or registerCss in view coz cache just keep for HTML result not for JS and CSS.

What is the expected result?

when widget run in cache condition will registerJs and registerCss

What do you get instead?

disable widget cache or manual cache for registerJs or registerCss

Additional info

Q A
Yii version 2.0.14
PHP version 5.6
Operating system mac
rugabarbo commented 6 years ago

The same problem with assets.

For example, for cacheable widget views I use:

$this->renderDynamic('\namespace\of\MyWidgetAsset::register($this);');

instead of:

MyWidgetAsset::register($this);

But this is trick...

rockman84 commented 6 years ago

for your case why not do in init widget?

public function init() {
   parent::init();
   MyWidgetAsset::register($this->view);
}

coz cacheable will run beforeRun event. but if using registerJs or registerCss will look messy code in widget class

public function init() {
   parent::init();
   $this->view->registerJs(<<<JS
// js code here
JS
);
   $this->view->registerCss(<<<CSS
// css code here
CSS
);
}
rugabarbo commented 6 years ago

for your case why not do in init widget?

Because of I want keep logic of assets registering into views. It's more intuitive.

samdark commented 6 years ago

That's expected. Won't be fixed. Workarounds do exist.

rugabarbo commented 6 years ago

That's expected.

Disagree.

I don't expect that widget can't register css/js resources because of cache. Register calls must be dynamic in any case (cacheable or not).

samdark commented 6 years ago

OK. We may run everything like that through renderDynamic() in 2.1.

rugabarbo commented 6 years ago

I attached a sample of fixes for the registerJs/Css methods. I'm worried about this part of the implementation: https://github.com/rugabarbo/yii2/blob/97bb652cc64300e5fe7c8e4406bd34d1f6ee51e9/framework/web/View.php#L409-L420

        if ($this->context instanceof Widget) {
            foreach ($this->context->getBehaviors() as $behavior) {
                if ($behavior instanceof CacheableWidgetBehavior) {
                    $css = var_export($css, true);
                    $options = var_export($options, true);
                    $key = var_export($key, true);
                    $this->renderDynamic("\$this->registerCss({$css}, {$options}, {$key});");

                    break;
                }
            }
        }

It looks like a hack, but I did not find other ways to fix it. What do you think about this?

If this approach is OK, I can correct other methods to completely fix the problems with the registration of resources by cacheable widgets.

rugabarbo commented 6 years ago

https://github.com/rugabarbo/yii2/commit/998b040784eaae5a1ffab35c9c5e25340cd3bb43 contains other example of possible fix. Just one postRegister() method will be used in the end of each register-method: registerCss, registerJs, registerLinkTag and so on. But this approach looks like even more hack :D

samdark commented 6 years ago

Both look quite hacky. @sergeymakinen do you have any ideas?

rockman84 commented 6 years ago

what i'm do now to fix it

class Widget extends Widget {

  public $assets = [
    '\namespace\of\MyWidgetAsset'
  ];

  public function beforeRun($event) {
     parent::beforeRun($event);
     foreach ($this->assets as $asset) {
        $asset::register($this->view);
     }
  }
}
rugabarbo commented 6 years ago

@rockman84 there are many other cases of resource/tag registration:

I think they should work fine for cacheable widgets out of the box.

@samdark

BTW, registerCsrfMetaTags() already uses my approach for tag registration:

https://github.com/yiisoft/yii2/blob/da9cb57eaca79444ffd4fb246263793371d99cbe/framework/web/View.php#L344-L363

This is not much different from my approach: https://github.com/rugabarbo/yii2/commit/97bb652cc64300e5fe7c8e4406bd34d1f6ee51e9 I just have to export arguments of registration functions to renderDynamic() call.

samdark commented 6 years ago

Yes. I think that's OK.

sergeymakinen commented 6 years ago

@samdark looks hacky but it's ok in this case I guess. In fact it's about "caching" a PHP code. Since it's predefined, it's ok for now.

Offtopic I didn't like inserting a raw PHP into strings, so I have written a [php file based cache adapter](https://github.com/sergeymakinen/yii2-php-file-cache). It's a mostly cache, but it also contains a dead simple [closure-to-raw-php extractor](https://github.com/sergeymakinen/yii2-php-file-cache/blob/master/src/Cache.php#L88-L158). Just an imagination around `renderDynamic()`'s possible future.
rockman84 commented 6 years ago

i agree with @sergeymakinen code looks hacky but since there is no another way for now. its ok

mikehaertl commented 6 years ago

Just for the record I came across this problem today when I used fragment caching in a view file. I was quite surprised to learn here that you get problems when caching any widget that registers js. I'd assume that quite a lot of widgets to that.

Even if the hackish solution is not integrated I'd say there should be big warning signs added in the docs for fragment caching here: https://www.yiiframework.com/doc/guide/2.0/en/caching-fragment