Correctly handle `null` values for `DbPanel::$criticalQueryThreshold` and `::$excessiveCallerThreshold`

yiisoft / yii2-debug

Debug Extension for Yii 2

http://www.yiiframework.com

BSD 3-Clause "New" or "Revised" License

202 stars 149 forks source link

Correctly handle `null` values for `DbPanel::$criticalQueryThreshold` and `::$excessiveCallerThreshold` #506

Closed rhertogh closed 1 year ago

rhertogh commented 1 year ago

Q	A
Is bugfix?	✔️
New feature?	❌
Breaks BC?	❌
Fixed issues	#505

what-the-diff[bot] commented 1 year ago

PR Summary

Threshold check added in getExcessiveCallers() The method now checks if the excessive caller threshold is set before proceeding.
Alert icon function for excessive calls A new function in the callers view checks if the number of calls exceeds the limit, and displays an alert icon if it does.
Simplified critical query threshold comparison In the summary view, a new function (isQueryCountCritical) is used to compare with the criticalQueryThreshold, making the comparison process more efficient.

rhertogh commented 1 year ago

@MarkoNV Could you please test this fix.

MarkoNV commented 1 year ago

@MarkoNV Could you please test this fix.

Reported issue is fixed. Adding possibility to null $excessiveCallerThreshold is nice touch and working correctly.

However, while testing I noticed one inconsistency, I not sure if it's related to fix or only became apparent after fix: On /debug/default/index excessive callers are shown according to value of $excessiveCallerThreshold during request execution, but number of queries warning is shown according to current value of $criticalQueryThreshold.

Both, summary and db panel always show according to current values of $excessiveCallerThreshold and $criticalQueryThreshold.

If it's not related to this fix, I can open new issue.

rhertogh commented 1 year ago

You're correct, the index page the number of excessive callers is indeed calculated and stored during the request execution and won't change when $excessiveCallerThreshold is changed. We could calculate it during viewing, but that would be less performant. Since the $excessiveCallerThreshold probably won't change that often I would recommend to keep it as it is. @MarkoNV @samdark Your thoughts on this?

MarkoNV commented 1 year ago

I'm OK with that, eventually, it should be mentioned in docs.

rhertogh commented 1 year ago

Docs are updated with note that changes will only be reflected in new requests and info about the null setting.