Problem/Motivation

Opening this for discussion. Inmail is missing the standard analyzer result object... It would allow each analyzer to update/extend the (same) result object. In Mailhandler, we created a MailhandlerAnalyzerResult (#2743921: Create Mailhandler analyzer) which has set of properties (footer, processed body, subject, sender) and the same result object is updated with different analyzers - MailhandlerAnalyzer, FooterAnalyzer etc...

Proposed resolution

We could add InmailAnalyzerResult as a default result object...

Comments

mbovan created an issue. See original summary.

miro_dietiker’s picture

Priority: Normal » Critical

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.

mbovan’s picture

one analyser dominates the process and creates pretty limited result object that can not be extended by others.

Not sure why it cannot be extended by others? Analyzers share the same (analyzer result) object and each has an option to set new data or update the data that's already set by other analyzers. We could add a default result object with (standard) properties like: sender, user, processed body, footer, signature...

The problem can be that some analyzers would need to set their results into context (a property on stanard Inmail analyzer result) or work with interfaces as @miro_dietiker mentioned.

miro_dietiker’s picture

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.

mbovan’s picture

Status: Active » Needs review
Issue tags: +gsoc2016
FileSize
10.61 KB

Adding InmaiilAnalyzerResult based on MailhandlerAnalyzerResult extended with context data.

Primsi’s picture

Status: Needs review » Needs work
  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.

Berdir’s picture

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.

mbovan’s picture

Thank you for the feedback, Sascha!

Fixed the points from #6 and implemented suggested methods in #7.

\Drupal\node\Plugin\Condition\NodeType has "node" context key in annotations. Does it mean it requires the context with "node" name to be present at time when this plugin runs?

mbovan’s picture

Primsi’s picture

Status: Needs review » Needs work
  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....

Primsi’s picture

mbovan’s picture

Addressed the points from #10 and added getContextsWithType() method.

Is there a nomenclature for naming contexts?

Hmm, no? Any suggestions on this? Maybe we can use a module name as a prefix, e.g. mailhandler_entity_type.

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....

Yes. It's overridden in case of the same name.

miro_dietiker’s picture

  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.

miro_dietiker’s picture

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?

mbovan’s picture

Applied suggestions from #13.

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.

Added Inmail application in this patch and relevant tests. Also, there is an implemented suggestion to ensure/create InmailAnalyzerResult by default. It would allow us to prefill some properties (AnonymousUser as the default account) that can be overridden by analyzers later. Btw, to pre-set the user we would need "user" dependency or to check if user module is enabled. Not sure what is the proposed way in this sort of situations.

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?

Added a test analyzer context example. Hm, didn't we describe context "plugins" as parts of analyzers?

Status: Needs review » Needs work

The last submitted patch, 15: lack_of_standard_result-2754253-15.patch, failed testing.

miro_dietiker’s picture

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...

miro_dietiker’s picture

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?

mbovan’s picture

Re #18.1: Fixed.
Re #18.2: Created a followup #2769061: Replace bounce analyzer result with default one.
Re #18.3: Yes, I was thinking that we'll have the situation e.g.getAccount()->hasPermission()
Re #18.4: Ah, I've just updated VERP as an example where we can merge properties with default result object. Reverted the changes.

AnalyzerTest passes locally, not sure why it's failing on testbot.

Status: Needs review » Needs work

The last submitted patch, 19: lack_of_standard_result-2754253-19.patch, failed testing.

miro_dietiker’s picture

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.

mbovan’s picture

Berdir’s picture

+++ 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)

miro_dietiker’s picture

Status: Needs review » Fixed

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

  • miro_dietiker committed 52f0b30 on 8.x-1.x authored by mbovan
    Issue #2754253 by mbovan, miro_dietiker, Primsi: Lack of standard result...
Arla’s picture

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()).

Status: Fixed » Closed (fixed)

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