New system check diagnostic for max_execution_time (#11050) #12576
Conversation
|
|
||
| namespace Piwik\Plugins\Diagnostics\Diagnostic; | ||
|
|
||
| class PhpSettingsCheckService |
There was a problem hiding this comment.
Given this class is meant to be constructed for each required setting, it's not really a 'service class' (who are meant to be stateless or at least immutable). This is more like the https://github.com/matomo-org/matomo/blob/3.x-dev/core/Settings/Plugin/UserSetting.php or https://github.com/matomo-org/matomo/blob/3.x-dev/core/Settings/Plugin/SystemSetting.php class. I think a better name would be RequiredPhpSetting.
| $checks[] = $requiredValue['operator'] . ' ' . $requiredValue['requiredValue']; | ||
| } | ||
|
|
||
| return $this->setting . ' ' . implode(' || ', $checks); |
There was a problem hiding this comment.
Given this is displayed to the user, I think it would be better to use OR here instead of ||.
There was a problem hiding this comment.
Swapped. Also, I've changed isOk to isValid.
|
|
||
| $maxExecutionTime = new RequiredPhpSetting('max_execution_time', 0); | ||
| $maxExecutionTime->addRequiredValue(30, '>='); | ||
| $requiredSettings[] = $maxExecutionTime; |
There was a problem hiding this comment.
Tested locally and I noticed that since required settings are added as an ERROR status, this will block installation if max_execution_time is not > 30 (eg, if it's set to 10 on some shared hosting provider, you wouldn't be able to install matomo).
@mattab do you think this is ok? If not, maybe we can make it a warning.
There was a problem hiding this comment.
@diosmosis +1 to make it a warning.
|
@simivar do you have some time to adjust this PR so the check for |
|
@sgiehl Yes, I'll do that on Wednesday / Thursday. |
|
hi @simivar, will you have time to apply the requested changes this week? |
|
hi @diosmosis, sorry for such late reply but I was away on vacation and just came back today. I'll apply them this week. |
|
@mattab @diosmosis Updated, sorry that it took so long. |
|
fyi @mattab merging this in 3.6.0 since it's done & working |
|
@simivar thanks for the awesome contribution! |
…matomo-org#12576) * New system check diagnostic for max_execution_time (matomo-org#11050) * PhpSettingsCheck: getRequiredSettings() returns array instead building it in property * Rename PhpSettingsCheckService to RequiredPhpSetting and swap || with OR in string * PhpSettingsCheck: method to set whether setting should raise an error or warning
New system check diagnostic for max_execution_time (#11050)
I don't know if I didn't overdid it. If so and you don't like the new
PhpSettingsCheckServiceclass feel free to close that PR and I'll make new PR with different approach.If
PhpSettingsCheckServiceis fine should I move it to DI likeTranslator?