Lack of standard result in collaboration of analyzers

The problem here is that
- the analyzers should collaborate with each other, but
- one analyser dominates the process and creates pretty limited result object that can not be extended by others.

this is an architectural conflict with the current setup and asks if we should have a standard result that offers some collaborative interfaces such as adding contextual data...

Promoting to critical since it's significantly shaping the API.

Trying to better explain my argumentation above.

In the current coding pattern, the first processor who create a result is the Winner.
All other processors can not create a different result with a different more specific interface to them. They need to collaborate with prior analysers through extending the result. What means, the result is fixed in type.
(As soon a more specific analyser result is wanted, we risk dropping data of earlier analyser when the result is replaced.)

This means, as soon as we have a default result object, basically no analyser ever has the opportunity to create an other more specific result.
It also means that the API of the default result should be feature complete and ready for all cases.

1. +++ b/src/InmailAnalyzerResult.php
   @@ -0,0 +1,343 @@
   +   * The user
   

   Missing ending dot.

2. +++ b/src/InmailAnalyzerResult.php
   @@ -0,0 +1,343 @@
   +  protected $entity_type;
   

   This might not be relevant for all message types. Move to a context?

3. +++ b/src/InmailAnalyzerResult.php
   @@ -0,0 +1,343 @@
   +  protected $bundle;
   

   Same as above.

Discussed this with Miro quickly.

My suggestion was to use the Context classes of the plugin system. They have a type, a definition and a value and are integrated with typed data (make sure you use the one in Drupal\Core, not component).

Then you are just a few steps away from Plugins that can define what contexts they need (See \Drupal\node\Plugin\Condition\NodeType as an example) and I think rules also uses that or at least builds on top of it.

So you'd have a method like addContext($name, ContextInterface $context), hasContext(), getContexts(), maybe also something like getContextsWithType() or similar helper methods.

1. +++ b/src/InmailAnalyzerResult.php
   @@ -0,0 +1,220 @@
   +   * @var array
   

   Souldn't is be ContextInterface[] ?

2. +++ b/src/InmailAnalyzerResult.php
   @@ -0,0 +1,220 @@
   +  public function setUser($user) {
   

   Type hint?

3. +++ b/src/InmailAnalyzerResult.php
   @@ -0,0 +1,220 @@
   +    if ($this->getSender()) {
   +      $summary['sender'] = $this->getSender();
   +    }
   +    if ($this->getSubject()) {
   +      $summary['subject'] = $this->getSubject();
   +    }
   

   Just sender and subject? Should we at least mention that other contexts are attached?

4. +++ b/src/InmailAnalyzerResult.php
   @@ -0,0 +1,220 @@
   +    return $this->contexts;
   

   Has no default value.

5. +++ b/src/InmailAnalyzerResult.php
   @@ -0,0 +1,220 @@
   +  public function hasContext($name) {
   

   So the subsequent parts of the system need to know the name? Should we support getting contexts by type? Is there a nomenclature for naming contexts?

Also: what happens if two analysers want to add the same context type. The last one overrides all? Hm... is there even a use case for it....

1. +++ b/src/InmailAnalyzerResult.php
   @@ -0,0 +1,233 @@
   +  protected $user;
   

   Unsure if this should be Account or user?

2. +++ b/src/InmailAnalyzerResult.php
   @@ -0,0 +1,233 @@
   +    return t('Inmail Analyzer Result');
   ...
   +  /**
   

   How about "Default Result"?

3. +++ b/src/InmailAnalyzerResultInterface.php
   @@ -0,0 +1,154 @@
   +  public function getContexts();
   

   getAllContexts() or so?
   I don't like that +s to fetch all. Core also adds the +Multiple.

4. +++ b/src/InmailAnalyzerResultInterface.php
   @@ -0,0 +1,154 @@
   +   * @return \Drupal\Core\Plugin\Context\ContextInterface|null $context
   +   *   Requested context object or NULL if not found.
   ...
   +  public function getContext($name);
   

   IMHO this should throw an exception when the context with $name is not existing, expecially since we have a hasContext($name)

This issue neither uses the result in inmail nor tests it.
I think we need this and can not simply commit the result for later use.

Also i'm missing the application of the context definition.
Through the definition it should be possibly to enumerate all existing contexts accross all context providers.
We need a discovery / plugin manager for that?

One question would be, will we simply put account/user into a named context and use the same API?

Anonymous is the default. There is no reason to load an anonymous user and attach it to the user context. Attaching NULL or not attaching anything at all needs to have the same effect. We should really only attach an account if we should switch to a specific account from anonymous.

Not sure about your other question...

Some thoughts from a quick review.

1. +++ b/src/InmailAnalyzerResult.php
   @@ -0,0 +1,229 @@
   +class InmailAnalyzerResult implements AnalyzerResultInterface, InmailAnalyzerResultInterface {
   

   Why not name it DefaultAnalyzerResult?

2. +++ b/src/MessageProcessor.php
   @@ -95,6 +105,15 @@ class MessageProcessor implements MessageProcessorInterface {
   +    $default_result = $result->ensureAnalyzerResult(InmailAnalyzerResult::TOPIC, InmailAnalyzerResult::createFactory());
   

   Hm yeah, we kindof have double extensibility here with result topics.

   The whole bounce case could also be handled with a simple single result and some context data instead of a bounce topic and a separate result. We need to carefully think about this situation and decide if we will drop the multi result concept as a whole if we have contextual typed data on the result. Not yet sure.

   Happy to discuss and separate this problem into a follow-up. For now i would just introduce the standard result and offer the new methods and avoid overlaps with the bounce cases as much as possible.

3. +++ b/src/MessageProcessor.php
   @@ -95,6 +105,15 @@ class MessageProcessor implements MessageProcessorInterface {
   +      $default_result->setAccount(User::getAnonymousUser());
   

   Unsure here. My idea was to only set this if it's different than anonymous. But i see, we could also simply initialise it to anonymous.

4. +++ b/src/Plugin/inmail/Analyzer/VERPAnalyzer.php
   @@ -65,7 +68,9 @@ class VerpAnalyzer extends AnalyzerBase {
   +      $address = $matches[1] . '@' . $matches[2];
   +      $result->setRecipient($address);
   +      $default_result->setSender($address);
   
   +++ b/tests/src/Kernel/VerpTest.php
   @@ -63,8 +64,11 @@ class VerpTest extends KernelTestBase {
    
   

   Why do we need to set the default result sender here? Why overwrite it in Verp?

Also another follow-up: Even the user context (and many others) should then be exposed through the same context definition interface and internally use those. So the same system can be used to build a UI.

1. +++ b/src/DefaultAnalyzerResult.php
   @@ -0,0 +1,229 @@
   +  public function addContext($name, ContextInterface $context) {
   +    $this->contexts[$name] = $context;
   

   What if the context already exists? We should also throw exception here.

2. +++ b/src/DefaultAnalyzerResult.php
   @@ -0,0 +1,229 @@
   +  public function getContext($name) {
   +    return $this->contexts[$name];
   

   I think this should throw an exception if undefined.

3. +++ b/src/MessageProcessor.php
   @@ -95,6 +105,15 @@ class MessageProcessor implements MessageProcessorInterface {
   +    // @todo: Add dependency to "user" module?
   +    if ($this->moduleHandler->moduleExists('user')) {
   

   No need to check. In real installations, user module always exists. (Except kernel tests)

Test fails: Plz check core and contrib module versions of testbot to reproduce the same condition locally.

Another Follow-up will be to declare the potential analyzer contexts through annotation so we can build a UI.

+++ b/tests/modules/inmail_test/src/Plugin/inmail/Analyzer/TestAnalyzer.php
@@ -0,0 +1,44 @@
+  private function addContext($default_result) {

protected

Also remove the @todo.

(miro will do that on commit)

Thx, committed. :-)

And created a follow-up about converting the internals of the standard result to context data too...
#2770553: Expose defaultAnalyzerResult properties as contexts

Oh no, I'm too slow :) Commenting anyway, for discussion's sake (here or in followups).

IS is not very clear about the relation between this issue and #2743921: Create Mailhandler analyzer. I suppose that it was simply the second analyzer result class created, and therefore triggered the need of this issue?

We need to carefully think about this situation and decide if we will drop the multi result concept as a whole if we have contextual typed data on the result. Not yet sure. (#18)

I'm looking forward to that followup!

Even the user context (and many others) should then be exposed through the same context definition interface and internally use those. (#21)

Indeed, I thought the point was to only use context already in this issue. That is, no sender/account/body/footer/subject properties in DefaultAnalyzerResult, only contexts.

+++ b/src/DefaultAnalyzerResult.php
@@ -0,0 +1,235 @@
+class DefaultAnalyzerResult implements AnalyzerResultInterface, DefaultAnalyzerResultInterface {

Is there really a point in a new interface for the new result class? If yes, why doesn't it extend AnalyzerResultInterface?

+++ b/src/DefaultAnalyzerResult.php
@@ -0,0 +1,235 @@
+  public function addContext($name, ContextInterface $context) {
+    if (isset($this->contexts[$name])) {
+      throw new \InvalidArgumentException('Context "' . $name . '" already exists.');
+    }
+    $this->contexts[$name] = $context;
+  }

AFAIK it's customary to use setX() when the structure is associative (and addX() when it's a list). IMHO, that would also make the exception unnecessary, because "set" implies that overwriting may occur. (If the caller wants to avoid overwriting, it should first check the result of hasContext()).

Created followups based on my last comment:
#2770567: Remove ability to extend results by "topic" now that there are contexts (highly related to, almost a duplicate of #2769061: Replace bounce analyzer result with default one)
#2770667: Remove DefaultAnalyzerResultInterface
#2770679: Rename addContext() to setContext()