With #2495613: Sensor for php notices, we started limiting down the configurable elements but we also have a number of existing sensors that need the same, e.g. 404 and image style derivates, no point in showing configurable conditions and verbose output there since they have a custom output anyway.
This also contains verbose output improvements for those two sensors.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | configurable-configuration-verbose-output-2543886-10-interdiff.txt | 3.83 KB | berdir |
| #10 | configurable-configuration-verbose-output-2543886-10.patch | 14.89 KB | berdir |
Comments
Comment #1
berdirComment #5
miro_dietikerNice move.
I think the main problem with the code is that you never check for the value of forcedTable or forcedTimeintervalField when excuting the sensors. Instead the settings are always used.
If we go into this direction, we should possibly add a wrapper for all the forcable settings similar to getTimeintervalField()...
Missing is at least getTable()... And check the forced XYZ setting. Then we have a consistent pattern.
Comment #8
berdirFixed the phpnotice sensor, not too happy yet with that specific check about verbose fields there.
Always checking the force flags makes sense, although we should still enforce that config has the correct values and then it doesn't really matter. This is mostly for preventing that the settings can be changed in the UI. Not 100% sure if being able to change them in config is a bug or a feature.. :) For example, we need to make sure that the config dependency on e.g. the table still works, but that can also go throgh the helper.
Comment #9
miro_dietikerFragments from our discussions...
Comment #10
berdirYep, this makes more sense I guess.
Comment #12
miro_dietikerYeah this looks much clean now.
But there is nothing about default values. So with disabled configuration, you fully rely on persisted default values on persisted sensors. This seems fine for updates it looks like the settings are not dropped.
The bigger problem here is creating instances through the UI. If a sensor has configurable set to FALSE and is addable through UI, the value will always end up empty. And in most cases this is wrong since disabling configurability is more something to force a setting to a specific value such as a table.
Comment #13
berdirThere doesn't need to be, default configuration is a different problem. None of the sensors touched here can be added, so it is irrelevant to those.
Comment #14
miro_dietikerYay! Committed.
Tests didn't pass because some dependencies did not yet chase with head.
Comment #18
giancarlosotelo commented