Separate text fields per verbose extra field

Here is a start, no text area anymore, validation has moved to the validateConfigurationForm() function, renamed all 'keys' instances to 'fields'. Adding and deleting fields is possible, fields are validated before saving, so requesting a non-existing field won't break the whole sensor.

Here is a screenshot:

1. Back to keys it is.

2. I went over all comments and checked if they make sense/are still copy&paste.

3. Removed this health check as well as the equivalent for the conditions.

4. Changed the button accordingly for keys and conditions.

5. $database->schema()->fieldExists($table, $key_name) is way easier than configuring a sensor and running it. Also added validation for the table (because I use that for later validation).

Rebased, and this time the issue and the patch belong together.

New version, switched to verbose_fields, re-added try/catch block which runs a query on that table with the field (and it's a simple query now instead of running a modified sensor).

Corrected the schema, should have run a test locally first : /.

Not finished, just uploading a patch so I can continue from home :).

Edit: Some feedback for the validation would be nice though :). (reverting the fields is unnecessary, as the form won't continue until the form_state is error free)

Removed the $settings array and the usage where possible (I use it once to save the submitted settings), removed the unused delta, made another query for the table in case the schema says the table doesn't exist, fixed the fields query.

Removed the array_pop (the whole line was rather pointless) and cleaned up the validation/saving of new fields.

Fixed the test fails which are related to this issue in MonitroingCoreWebTest. Couldn't reproduce the payment test fail though.

1. +++ b/src/Plugin/monitoring/SensorPlugin/DatabaseAggregatorSensorPlugin.php
   @@ -127,10 +120,10 @@ class DatabaseAggregatorSensorPlugin extends DatabaseAggregatorSensorPluginBase
   +    $fields = $this->sensorConfig->getSetting('verbose_fields');
   +    if (!empty($fields)) {
   +      foreach ($this->sensorConfig->getSetting('verbose_fields') as $field) {
   +        $query->addField($this->sensorConfig->getSetting('table'), $field);
          }
        }
   

   Why not use $fields in foreach?

2. +++ b/src/Plugin/monitoring/SensorPlugin/DatabaseAggregatorSensorPlugin.php
   @@ -361,6 +331,57 @@ class DatabaseAggregatorSensorPlugin extends DatabaseAggregatorSensorPluginBase
   +    $form['output_table'] = array(
   ...
   +      '#tree' => FALSE,
   +    );
   

   Is it #tree false by default?

3. +++ b/src/Plugin/monitoring/SensorPlugin/DatabaseAggregatorSensorPlugin.php
   @@ -361,6 +331,57 @@ class DatabaseAggregatorSensorPlugin extends DatabaseAggregatorSensorPluginBase
   +      '#empty' => t(
   +        'Add keys to display in the verbose output.'
   +      ),
   

   One line?

4. +++ b/src/Plugin/monitoring/SensorPlugin/DatabaseAggregatorSensorPlugin.php
   @@ -398,37 +419,24 @@ class DatabaseAggregatorSensorPlugin extends DatabaseAggregatorSensorPluginBase
   -    $settings['conditions'] = [];
   +    $conditions = [];
   ...
   -        $settings['conditions'][] = $condition;
   +        $conditions[] = $condition;
   ...
   +    $settings['conditions'] = $conditions;
   

   Do we need this change? If we do like before then we don't need $settings['conditions'] = $conditions; line.

5. +++ b/src/Plugin/monitoring/SensorPlugin/DatabaseAggregatorSensorPlugin.php
   @@ -398,37 +419,24 @@ class DatabaseAggregatorSensorPlugin extends DatabaseAggregatorSensorPluginBase
   +    $verbose_fields = [];
   +    foreach ($form_state->getValue('verbose_fields') as $field) {
   +      if (!empty($field['field'])) {
   +        $verbose_fields[] = $field['field'];
   +      }
        }
   ...
   +    $settings['verbose_fields'] = $verbose_fields;
   

   Same here, you can use $settings['verbose_fields'][] instead of $verbose_fields[].

6. +++ b/src/Plugin/monitoring/SensorPlugin/DatabaseAggregatorSensorPlugin.php
   @@ -477,10 +526,26 @@ class DatabaseAggregatorSensorPlugin extends DatabaseAggregatorSensorPluginBase
   -          t('The specified time interval field %name does not exist.', array('%name' => $field_name)));
   +          t('The specified time interval field %name does not exist in table %table.', array('%name' => $field_name, $table)));
   

   %table array key is missing.

1. Done

2. It's not, removing it breaks saving of fields.

3. Done

4. & 5. Done

6. Done

Re #27 (3): You changed different empty message. That's good, but this one is still here. :)

+++ b/src/Plugin/monitoring/SensorPlugin/DatabaseAggregatorSensorPlugin.php
@@ -361,6 +329,57 @@ class DatabaseAggregatorSensorPlugin extends DatabaseAggregatorSensorPluginBase
+      '#empty' => t(
+        'Add keys to display in the verbose output.'
+      ),

Thanks! Fixed the second #empty message.

Readded the comment, removed one empty line in a docblock and changed the header for the verbose fields and conditions table slightly.

The 'field' nesting is necessary because every verbose field is a row in a table with only one column (the 'field' column). I tried helping alleviate the confusion by adding a comment and renaming the column field to field_key (so the key for a field is the content of that column). I also changed the header for the conditions to also say 'Field key', so both are consistent.

The tests did need some updating with the new column key, couldn't reproduce in the beginning because I was in the completly wrong branch... :D

Here is the patch with the updated tests.

When i create a new sensor, i still only get "id" and "label" is missing as verbose field.

Stupid mistake, fixed.

Also the conceptual fields like id / label / ... are not on the field list, making me confused as a user.
We should list them separately.

I put the synonymous key in brackets after the field now, see screenshot.

I tried to add a "path" field to a node sensor. Column was empty and when executing, the sensor has thrown an error.

Finally managed to fix that according to berdir's proposal, I check if there is a field default_formatter with the FieldTypePluginManager. If not, I don't try and 'view()' the field but keep the $value as set previously ($entity->$field->$property).

Here is the interdiff, thought I clicked on upload, apparently not.

1. +++ b/src/Plugin/monitoring/SensorPlugin/EntityAggregatorSensorPlugin.php
   @@ -243,8 +245,15 @@ class EntityAggregatorSensorPlugin extends DatabaseAggregatorSensorPluginBase im
   +            $ui_definitions = $field_type_manager->getUiDefinitions();
   +            $value = $entity->$field->$property;
   +            if (!empty($ui_definitions[$field_type])) {
   +              if (isset($ui_definitions[$field_type]['default_formatter'])) {
   

   I don't think you need UiDefinitions() here ? Those are already filtered, so path will never even be in there.

   Instead, you could just write if (isset($field_type_manager->getDefinition($field_type)['default_formatter']

2. +++ b/src/Plugin/monitoring/SensorPlugin/EntityAggregatorSensorPlugin.php
   @@ -414,16 +423,24 @@ class EntityAggregatorSensorPlugin extends DatabaseAggregatorSensorPluginBase im
   +    $keys = array_flip($entity_type->getKeys());
   +    foreach ($available_fields as $delta => $field) {
   +      if (isset($keys[$field])) {
   +        if ($field != $keys[$field]) {
   +          $available_fields[$delta] = $field . ' (' . $keys[$field] . ')';
   +        }
   +      }
   +    }
   

   Do we really support all the keys her? Stuff like langcode and status?

   I'd just hardcode those that you explicitly support.

1. Done

2. Except for Changed which caused some problems (I could add a case for changed and handle it correctly though) all fields seemed fined. I switched to getting the keys from the entity_type directly. Less keys are displayed now (but more are supported).

There is another problem though with some entities: File for example has for key label the field filename, the field label has a special case for rendering a link, the File doesn't have a canonical Url though and getUrlInfo() does not work. I fixed it now by switching the from field key to the field itself in case that is possible (as it is with File), if there is no field set for the key label, I use the case 'label': instead of the default case which renders the linked label to the entity.

Here is the screenshot I was talking about up there, it's from the latest version with less keys.

Postponed and waiting for #2534498: Modify EntityAggregatorSensor to only aggregate content entities.

Not so sure about the direction of the last patches, we lost all the real fields.

I dropped the keys stuff almost completely. Instead, just hardcoding id and label as suggested above. We have a small overlap but that makes defaults a lot easier. Also simplified the label case a bit and made sure it doesn't fail when we don't have a canonical link template.

We have quite some checks and special cases now. And not that much test coverage for them.

I'm going to commit this when it is green. And I'll create a follow-up to test creating sensors for nodes and files with all available fields and make sure the output is correct and doesn't fail anywhere.

Fixed that test.

Committed.