Problem/Motivation

We have multiple issues with possible malconfiguration or unmet requirements.

First, we don't want to check everything and have false positive error in system requirements.

All plugins have config based instances.

Problems could be:
- IMAP contains wrong credentials.
- Many unprocessed items in IMAP or any other queue based fetcher.
- Unmet php library dependency, example pgp signing / check library.

Some of the issues can be covered by Monitoring project. Others need new coverage, typically the hook_requirements.

Proposed resolution

Let inmail implement hook_requirements.

Let inmail cycle through all instances of plugins (deliverer, analyser, handler) and ask each handler if it is healthy.
Could be checkRequirements().

For this, we will introduce a new method on the interface.
Checking for a library dependency is easy.
Make sure to not check if an item is disabled. thus if no pgp analyser is enabled, no error if the library is missing.

Manually disabling is a different thing to being unable based on runtime checks.

We can add an argument to checkRequirements, identifying "runtime" for runtime checks and "requirements" for the global hook.
The requirements check can be verbose, even trying to connect to each service configured to warn about wrong credentials setup.
The runtime check will only quickly check the situation to avoid fatal errors. It need to be fast and should be performed prior to interacting with the plugin instance. Not sure where exactle the API should be called, but for sure in check / fetch of IMAP.

May be, we even want to create two different methods?
- For hook_requirements, we need the requirements return value.
- For the runtime check, we might want to throw an exception?

Remaining tasks

User interface changes

API changes

Data model changes

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

miro_dietiker created an issue. See original summary.

miro_dietiker’s picture

Issue summary: View changes

Since we will use Monitoring sensors to make sure there are not too many unprocessed mails. (However we need the sensors. ;-) )
Also, we want to avoid double monitoring and having two errors on our dashboard (requirements + specific sensor)...

I just realised that the count of unprocessed mails is not the best measure.
Instead the best to know would be the age of the oldest unprocessed mail. The sensor message value would be the age (simlar to cron) and the message could still contain the total count and the remaining count. BTW it's one sensor per deliverer instance. Or a sensor per plugin listing the aggregate of its instances.

Explanation: A threshold based on count doesn't fit sites as a high traffic site would soon go critical and a threshold of 0 or 1 for a small site would also result in many wrong warnings / escalations... and higher numbers add no value because you never know if the single mail you just sent is processed or not.

Primsi’s picture

One additional remark: our hook_requirements implementation will have to run in the "runtime" phase, because the other two options would mean that we are checking requirements only on inmail install or update, which is not very useful.

One possible way would be to invoke the method once when submitting the plugin form in Inmail for every plugin and then instead of the argument on the method have a new entry on the plugin annotation, "validate_runtime" or something similar, so that inmail knows which plugins to check runtime and which only on enable. The idea of throwing exceptions on runtime is still applicable, we just catch them in inmail and handle them accordingly for runtime checks.

mbovan’s picture

Status: Active » Needs review
FileSize
9.33 KB

I'm uploading the initial patch as an idea. There are some @todo's added.

IIRC, "runtime" phase of hook_requirements() executes on admin/config and admin/reports/status only.

miro_dietiker’s picture

Status: Needs review » Needs work
  1. +++ b/inmail.install
    @@ -3,6 +3,48 @@
    +    $enabled_instances = $entity_type_manager->getStorage("inmail_$plugin_type")->loadByProperties(['status' => TRUE]);
    

    Don't concatenate here. Define the name in full in the static list.

  2. +++ b/src/PluginRequirementsInterface.php
    @@ -0,0 +1,18 @@
    +  public function checkRequirements();
    

    If this collects things, it should return some more structure than just a string. Requirements are keyed by a name. Possibly even more structure including verbosity... Possibly we even want to pass in the structure by reference?

    At least the return bool|string is a bad idea. We need an interface that tells us more about severity, count, allow us to iterate through individual info and so on.

    Also i think this check should receive information about if it's a simple lightweight check of in full.

miro_dietiker’s picture

@Primsi The proposal to extend checkRequirements would be to allow

+++ b/inmail.install
@@ -3,6 +3,48 @@
+    foreach ($enabled_instances as $enabled_instance) {
...
+      $plugins[$key] = $enabled_instance->checkRequirements();

Oh also, now we are triggering checkRequirements() per instance.
If the plugin has a requirement to a library such as PGP and it has two instances, the error would appear twice.

I think we should create two methods, one is static per plugin to checkRequirements and one is per instance to check configuration.

+++ b/inmail.install
@@ -3,6 +3,48 @@
+      $key = $plugin_type . '_' . $enabled_instance->id();

Aha... now i see why you avoided the inmail_ prefix above. Requirement keys are nicer...

+++ b/inmail.install
@@ -3,6 +3,48 @@
+      if ($plugins[$key] === TRUE) {
+        unset($plugins[$key]);

Yeah really ugly. Better conditionally add it, the problem is the variable types of the return statement.
But with my proposal to define a uniform return type in the interface, you can always merge.

Trying to be more specific: hook_requirements expects an associative array:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...

So the key could be calculated by the caller...
IMHO checkRequirements should always return an array with the keys as defined in hook_req like title, value, severity, description. If nothing to report, then return empty array.

mbovan’s picture

Status: Needs work » Needs review
FileSize
13.29 KB
8.92 KB

Providing a patch with checkRequirements() updates and tests (not sure about the naming there).

Re #5.2, #6:

If this collects things, it should return some more structure than just a string.

Oh.. I was returning the array structure, but forgot to update the documentation... Updated the documentation.

Re #6:

Oh also, now we are triggering checkRequirements() per instance.
If the plugin has a requirement to a library such as PGP and it has two instances, the error would appear twice.

I think we should create two methods, one is static per plugin to checkRequirements and one is per instance to check configuration.

We could aslo modify inmail_get_plugin_requirements and filter out duplicates?

miro_dietiker’s picture

Status: Needs review » Needs work
  1. +++ b/src/Entity/PluginConfigEntity.php
    @@ -71,4 +72,25 @@ abstract class PluginConfigEntity extends ConfigEntityBase {
    +    $requirements = [];
    ...
    +    return $requirements;
    

    Thus, you are attaching empty arrays to the hook_requirements for plugins that have no (enabled) instance.

  2. +++ b/src/Entity/PluginConfigEntity.php
    @@ -71,4 +72,25 @@ abstract class PluginConfigEntity extends ConfigEntityBase {
    +    $entity_type_parameters = explode('_', $this->getEntityTypeId());
    +    $plugin_type = end($entity_type_parameters);
    ...
    +    $plugin_manager = \Drupal::service("plugin.manager.inmail.$plugin_type");
    

    This reads so much crazy...
    This should somehow be more explicit, such as setting a property in each subclass constructor.

  3. +++ b/src/MessageProcessor.php
    @@ -115,7 +115,7 @@ class MessageProcessor implements MessageProcessorInterface {
    +      if ($analyzer_config->status() && empty($analyzer_config->checkRequirements())) {
    
    @@ -136,7 +136,7 @@ class MessageProcessor implements MessageProcessorInterface {
    +      if ($handler_config->status() && empty($handler_config->checkRequirements())) {
    

    Hmm... you should add a separate API for this.
    The hook_requirements (check admin reports requirements) can easily also contain a status message that is no error. And that is totally OK... could be queue size or last sync time or whatever...
    So we need some extra TRUE|FALSE API that only stops if misconfigured.
    However, this now leads to silent skipping.
    I guess we should skipping to the analyzer / handler at least on the processing result.

mbovan’s picture

Status: Needs work » Needs review
FileSize
14.51 KB
18.19 KB

I'm uploading an updated patch.

Here is the short summary:
- I added a new method on PluginRequirementsInterface - isAvailable() to get a boolean value of plugin's availability
- Implemented getPluginInstance() on a config entity
- Added plugin type property (deliverer, handler, analyzer)
- Fixed hook_requirements to list requirements by plugin (instantiated plugins only) so we can avoid possible problem of multiple same-message problem.
- Also, I added a new PluginBase which implements PluginRequirementsInterface (didn't find better name) and it's used as a base class for Inmail plugin types. Not sure about this DRY change though...

miro_dietiker’s picture

Status: Needs review » Needs work
+++ b/inmail.install
@@ -5,6 +5,46 @@
+        $key = $plugin_type . '_' . $enabled_instance->getPluginId();
...
+          $plugin_requirements[$key] = $enabled_instance->checkRequirements();

Hmm...

Now we can no more check if entered passwords are OK of a plugin instance!

miro_dietiker’s picture

OK Again, to sum up, we have two categories of things to check in requirements:

1) If plugin dependencies are met, such as
- php-imap module installed, for IMAP fetching (deliverer)
- php-pgp module installed, for crypto signing (anaylzer)

2) If settings are right, such as
- IMAP credentials are right

This issue should IMHO cover both cases, with dealing properly
- No repeating messages, per plugin check
- Allow per-instance checks.

mbovan’s picture

Status: Needs work » Needs review
FileSize
18.49 KB
9.66 KB

Made the changes with 2 new methods: checkPluginRequirements() and checkInstanceRequirements() . Also, updated deliverers code to check isAvailable() before using it.

miro_dietiker’s picture

Status: Needs review » Needs work
  1. +++ b/inmail.install
    @@ -5,6 +5,56 @@
    +    // Check plugin requirements for the current plugin type.
    +    foreach ($plugin_manager->getDefinitions() as $plugin_id => $definition) {
    +      $plugin_instance = $plugin_manager->createInstance($plugin_id);
    +      $requirements[$plugin_type . '_plugin_' . $plugin_id] = $plugin_instance->checkPluginRequirements();
    +    }
    

    This call should be a static plugin call.

    But i would simply first loop over instances and collect the plugin types with instances and then do the static call once.

  2. +++ b/inmail.install
    @@ -5,6 +5,56 @@
    +  $requirements = array_filter($requirements);
    

    That's a bit ugly.. :-)

    How about a cleaner merge before?

  3. +++ b/src/PluginBase.php
    @@ -0,0 +1,33 @@
    +use Drupal\Core\Plugin\PluginBase as CorePluginBase;
    ...
    +abstract class PluginBase extends CorePluginBase implements PluginRequirementsInterface {
    

    Plz avoid name clashes and name it something like InmailPluginBase.

mbovan’s picture

Status: Needs work » Needs review
FileSize
19.11 KB
9.33 KB

Tried to make it cleaner but not sure if it actually is. :)

Re #13.1:
Made checkPluginRequirements() a static method, but had to remove PluginRequirementsInterface from PluginConfig because of non-object context... I could update it with optional parameters but it would be too complex on the plugin side IMHO. So for now, we have isAvailable() and checkInstanceRequirements() methods available on PluginConfig entity too. They serve mostly as shortcuts...

But i would simply first loop over instances and collect the plugin types with instances and then do the static call once.

I'm doing it now in one loop (together with instances). I guess we want to check only those plugins that are already in use.

Re #13.2:
Because of the 1 loop above, I left this as is... I could check !empty() instead of filter, not sure..

Re #13.3:
Fixed.

miro_dietiker’s picture

Hmmmm... ;-)

Berdir’s picture

  1. +++ b/inmail.install
    @@ -5,6 +5,56 @@
    +
    +  // Filter plugins/instances with no requirements.
    +  $requirements = array_filter($requirements);
    

    Not 100% sure about doing it like this (vs. if checks above) or not but I do see a benefit: Only with this approach the logic to avoid repeated calls above makes sense, otherwise you'd have to collect them and do another loop. So fine I guess.

  2. +++ b/src/Entity/AnalyzerConfig.php
    @@ -43,6 +43,20 @@ namespace Drupal\inmail\Entity;
    +  public function __construct(array $values, $entity_type) {
    +    parent::__construct($values, $entity_type);
    +    $this->pluginType = 'analyzer';
    +  }
    

    wouldn't this be much easier if you just set $protected pluginType in each class? much fewer code to copy & paste.

miro_dietiker’s picture

Status: Needs review » Needs work
+++ b/inmail.install
@@ -5,6 +5,56 @@
+        $plugin = $entity->getPluginInstance();
+        $requirements[$plugin_key] = $plugin::checkPluginRequirements();

+++ b/src/Entity/PluginConfigEntity.php
@@ -38,6 +39,20 @@ abstract class PluginConfigEntity extends ConfigEntityBase {
+  protected $pluginInstance;

@@ -71,4 +86,65 @@ abstract class PluginConfigEntity extends ConfigEntityBase {
+    if (empty($this->pluginInstance)) {
+      return \Drupal::service('plugin.manager.inmail.' . $this->pluginType)->createInstance($this->plugin, $this->configuration);
+    }
+
+    return $this->pluginInstance;

the $pluginInstance is unused / never initialised.

Berdir’s picture

Discussed a bit with miro.

I think the one-off checkInstanceRequirements() method on the entity isn't very useful. you need the plugin anyway in your requirements helper function, just get it, then call it directly and then also call the static method on it.

Additionally, the if ($plugin_instance) check is kinda pointless right now. createInstance() doesn't return NULL, it returns an exception, so there is no scenario where that would ever be NULL/FALSE. Instead, wrap it in a try/catch in your function and report the absense of a plugin also as an error.

mbovan’s picture

Status: Needs work » Needs review
FileSize
17.99 KB
7.84 KB

Addressed: #16.2, #17 and #18.

I think the one-off checkInstanceRequirements() method on the entity isn't very useful. you need the plugin anyway in your requirements helper function, just get it, then call it directly and then also call the static method on it.

I left it on the entity mostly because the plugin is not aware of the attached entity. Thus, to solve the problem of repeating messages (#11.2.1) and make a difference between messages, I had to add to add something specific to the entity (ID and label). Also, I can add this in the helper method too, so moved it there...

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine to me now.

miro_dietiker’s picture

Status: Reviewed & tested by the community » Fixed

OK, committing, unlocking the other related / postponed ones. Thx!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.