search

yiisoft / yii2-debug

Debug Extension for Yii 2
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
202 stars 150 forks source link

Simplify isEnabled() check for AssetPanel and avoid throwing errors #364

Closed machour closed 5 years ago

machour commented 5 years ago
Q A
Is bugfix? yes
New feature? no
Breaks BC? no
Tests pass? yes
Fixed issues #275
samdark commented 5 years ago

That's controversial change. While panel should work with weird code flawlessly, silently disabling it in case of any error may be confusing for end user not knowing why it was disabled...

rob006 commented 5 years ago

@machour Any reason why you're catching everything instead of simple isset(Yii::$app->view) check?

BTW: Are you sure it will work? AFAIK you will get Error here, not Exception.

machour commented 5 years ago

@rob006 I reproduced the faulty setup and developed this PR based on it, so yes it works. I'll change it for a isset() instead 👍

@samdark this disables it for an unwanted init() call performed by external code. The panel is still displayed when initialized through the debug module bootstrap.

machour commented 5 years ago

@rob006 finally changed to check Yii::$app->assetManager directly, instead of ->view->. Seems to be working as expected for both normal & #275's setup.

rob006 commented 5 years ago

@machour Does it work correctly if you set View::$assetManager to custom instance (in this case assets will not be registered by Yii::$app->assetsManager)?

samdark commented 5 years ago

For more info on why it is like that currently see #252, #234, #220, #242.

machour commented 5 years ago

Nice catch, thank you @rob006.

Due to Yii __isset() usage, the following still produces an error:

isset(Yii::$app->view) && Yii::$app->view->assetsManager; 
// Undefined property: yii\web\Application::$view

Used isset(Yii::$app->view->assetsManager) instead, that fixes #275

samdark commented 5 years ago

Merged. Thank you!