Sensor to verify Performance settings

1. +++ b/config/schema/monitoring.schema.yml
   @@ -269,6 +269,10 @@ monitoring.settings.system_load:
          type: string
    
   +monitoring.settings.performance_settings:
   +  type: monitoring.settings_base
   

   wrong key here.

2. +++ b/src/Plugin/monitoring/SensorPlugin/PerformanceSettingsSensorPlugin.php
   @@ -0,0 +1,51 @@
   +    $bins = Cache::getBins();
   +    $null_cache_bins = FALSE;
   +    foreach ($bins as $bin) {
   +      if (!$bin) {
   +        $null_cache_bins = TRUE;
   

   the bin isn't not defined, it's an instance of the null backend.

I think the verbose table should be label: value, this isn't going to scale well as we add more checks. Have a look at the requirements sensor for example.

Maybe we could icons, experiment with something like the new status page?

Also, you're not yet implementing the most important part yet, runSensor() and the even moster important part: tests ;)

You might want to define a helper function that returns an aray which contains status true/false, label and value, so that you can also easily loop over it and list all non-ok things in status messages in runSensor()

1. +++ b/config/install/monitoring.sensor_config.monitoring_perfomance_settings.yml
   @@ -0,0 +1,15 @@
   +id: performance_settings
   
   +++ b/src/Plugin/monitoring/SensorPlugin/PerformanceSettingsSensorPlugin.php
   @@ -0,0 +1,231 @@
   + *   id = "monitoring_performance_settings",
   

   for the update sensors button to work, the id needs to be the same.

2. +++ b/config/install/monitoring.sensor_config.monitoring_perfomance_settings.yml
   @@ -0,0 +1,15 @@
   +caching_time: 86400
   

   there is no reason to cache this for a long time, it s a very fast sensor. set it to 15min or so.

3. +++ b/src/Plugin/monitoring/SensorPlugin/PerformanceSettingsSensorPlugin.php
   @@ -0,0 +1,231 @@
   +
   +    if ($value !== 0) {
   +      $sensor_result->setStatus(SensorResult::STATUS_WARNING);
   +    }
   

   you can simply set the status to warning, it doesn't matter if you set it multiple times an then the code gets simpler.

4. +++ b/src/Plugin/monitoring/SensorPlugin/PerformanceSettingsSensorPlugin.php
   @@ -0,0 +1,231 @@
   +    if ($this->sensorConfig->getSetting('css_aggregation')) {
   +      $css_enabled = $performance_config->get('css.preprocess');
   +      $performance_settings[] = [
   +        'label' => 'CSS Aggregation',
   +        'value' => $css_enabled ? 'Enabled' : 'Disabled',
   +        'status' => $css_enabled,
   +      ];
   +    }
   

   instead of making it conditional, I would add another key, 'enabled'. Then you don't need to duplicate the checks, which is why we added this helper, and instead can loop over it and build the table. If you need more info in verbose, just add more keys.

5. +++ b/tests/src/Kernel/MonitoringPerformanceSettingsPluginTest.php
   @@ -0,0 +1,60 @@
   +   * Tests the perfomance settings sensor plugin.
   +   */
   +  public function testPerfomanceSettingsSensorPlugin() {
   +
   

   to test null cache backend, use setSetting(), set the same value as you have in your settings.local.php

1. +++ b/src/Plugin/monitoring/SensorPlugin/PerformanceSettingsSensorPlugin.php
   @@ -0,0 +1,232 @@
   +        $sensor_result->addStatusMessage($this->t('@label : @value', ['@label' => $performance_setting['label'], '@value' => $performance_setting['value']]));
   

   We usually don't use t() in status messages, below you also don't, so just use string concatenation here.

2. +++ b/src/Plugin/monitoring/SensorPlugin/PerformanceSettingsSensorPlugin.php
   @@ -0,0 +1,232 @@
   +    $css_ignored = $this->sensorConfig->getSetting('css_aggregation') ? '' : ' (Ignored)';
   +    $rows[] = [
   +      'label' => 'CSS Aggregation' . $css_ignored,
   +      'status' => $css_enabled ? 'Enabled' : 'Disabled'
   +    ];
   

   I don't understand why you're not building the verbose table from the data in the helper?

3. +++ b/src/Plugin/monitoring/SensorPlugin/PerformanceSettingsSensorPlugin.php
   @@ -0,0 +1,232 @@
   +    $performance_settings = [];
   +    $performance_config = \Drupal::config('system.performance');
   

   we should use dependency injection for all servics.

4. +++ b/src/Plugin/monitoring/SensorPlugin/PerformanceSettingsSensorPlugin.php
   @@ -0,0 +1,232 @@
   +    // Check page caching.
   +    $moduleHandler = \Drupal::service('module_handler');
   +    $page_caching_enabled = $moduleHandler->moduleExists('page_cache');
   +    $performance_settings[] = [
   +      'label' => 'Page caching',
   +      'value' => $page_caching_enabled ? 'Enabled' : 'Disabled',
   +      'status' => $page_caching_enabled,
   +      'enabled' => $this->sensorConfig->getSetting('page_caching'),
   +    ];
   

   We probably want to check for dynamic page cache too, as a separate key. Wondering about big pipe as well, but lets leave that out for now.

5. +++ b/src/Plugin/monitoring/SensorPlugin/PerformanceSettingsSensorPlugin.php
   @@ -0,0 +1,232 @@
   +    // Check if there is a non zero max age.
   +    $cacheable_metadata = new CacheableMetadata();
   +    $non_zero_max_age = $cacheable_metadata->getCacheMaxAge() === 0 ?: 1;
   +    $performance_settings[] = [
   +      'label' => 'Non-zero max age',
   +      'value' => $non_zero_max_age ? 'Yes' : 'No',
   +      'status' => $non_zero_max_age === FALSE,
   +      'enabled' => $this->sensorConfig->getSetting('non_zero_max_setting'),
   +    ];
   

   good luck trying to write a test for this ;)

   PS: This doesn't make sense and is always true. That's not what this check is about, have a look at the performance settings page and how that is stored.

6. +++ b/src/Plugin/monitoring/SensorPlugin/PerformanceSettingsSensorPlugin.php
   @@ -0,0 +1,232 @@
   +      $backend = $cache_factory->get($bin);
   +      if ($backend instanceof NullBackend) {
   +        $null_cache_bins = TRUE;
   +      }
   

   I'd suggest to collect the null bins and list them in the output.

7. +++ b/tests/src/Kernel/MonitoringPerformanceSettingsPluginTest.php
   @@ -0,0 +1,69 @@
   +
   +    // Enable page caching.
   +    $this->enableModules(['page_cache']);
   +
   

   looks like you're not doing negative tests on the module, you should once test when it is not enabled. Just like with the settings and cache bin.. do one run with optimal settings, then set everything wrong and test again. Then set some settings and check again, like you already do.

8. +++ b/tests/src/Kernel/MonitoringPerformanceSettingsPluginTest.php
   @@ -0,0 +1,69 @@
   +    // Create a NullBackend cache bin.
   +    $cache_backend_null = [
   +      'bins' => [
   +        'render' => 'cache.backend.null',
   +        'dynamic_page_cache' => 'cache.backend.null',
   +      ]
   +    ];
   

   We're not creating a cache bin, we're setting the cache backend configuratoin for cache bins.

9. +++ b/tests/src/Kernel/MonitoringPerformanceSettingsPluginTest.php
   @@ -0,0 +1,69 @@
   +    $sensor_manager = monitoring_sensor_manager();
   +    $sensor_config = $sensor_manager->getSensorConfigByName('monitoring_performance_settings');
   +    $sensor_config->set('status', TRUE);
   +    $sensor_config->save();
   

   the sensor is enabled by default, so that shouldn't be needed?

One more thing to check.

If php version > 7.0, check the that http://php.net/manual/en/ini.core.php#ini.zend.assertions is set to -1.