Replace bounce analyzer result with default one

I just created #2770567: Remove ability to extend results by "topic" now that there are contexts with a very similar purpose to this issue.

It could be worthwile to do both: first convert the bounce analyzers to use DefaultAnalyzerResult::setContext() to try it out and revise the strategy, then possibly remove the concept of multiple analyzer results altogether, as suggested there.

For an initial patch, search for usages of BounceAnalyzerResult::TOPIC. It's used in a few Analyzer and Handler classes (and some tests, but let's ignore that to begin with), like this:

    $result = $processor_result->ensureAnalyzerResult(BounceAnalyzerResult::TOPIC, BounceAnalyzerResult::createFactory());

Then values are set (in analyzers) or accessed (in handlers) using methods defined by BounceAnalyzerResult.

The idea of this issue is to not use BounceAnalyzerResult, but DefaultAnalyzerResult. It doesn't have the same getters and setters, but it has the generic setContext() and getContext() methods that we want to use instead. Note that the value is then wrapped in a Context object. Open question: how can we avoid recreating the same context definitions multiple times?

Open question: how can we avoid recreating the same context definitions multiple times?

Hm.. I would expect that a context is usually originated by one analyzer.
Other analyzers depend on existing contexts and cancel operation if missing, without trying to recover too much.

@Arla There might be different special cases, you have an example of a challenging case?

In the bounce case, the idea is that you can choose which (one or more) you want to use. There's the native StandardDSNAnalyzer, but also the Cfortune and Phpmailerbmh plugins that rely on external libraries. And VerpAnalyzer, which focuses on the original recipient in a bounce message.

On the other hand, StandardDsnAnalyzer is simpler, standards-compliant and generally works better than the external libraries (in my experience), so we could let it define the bounce contexts. Then, if any other bounce analyzer is to be setup, it's required that the standard one is setup first.

For initial patch, I change to use DefaultAnalyzerResult instead of BounceAnalyzerResult. I forgot to remove BounceAnalyzerResult from usages, I will in next patch.

Yeah that's a small step in the right direction. As I suggested in pm, PhpStorm should already be telling you that you are using undefined methods on $result (because $result is now of a different type). So the next step is to switch to setContext() and getContext().

Major architectural refactoring. Thus critical.

Using getters and setters from DefaultAnalyzerResult instead function from BounceAnalyzerResult.
Test fails in nmailMailmuteTest::testBounceCounting when we want to access to sendstate which does not exist.

      $this->assertEqual($this->user->sendstate->plugin_id, 'inmail_counting');
      $this->assertEqual($this->user->sendstate->configuration['count'], $count);

I'm not sure what direction and i see two possible approaches:
- Attach a "Bounce" context that has some variables on it that are bounce specific (recipient, reason, ..)
- Attach the specific variables from the bounce information as context

If we attach specific ones, we also need to add more general context information such as a message type that is switched to bounce - to detect that it's not a regular message and more fun.

Thus, after checking, i think we should:
- Rename BounceAnalyzerResult to "BounceContext" (or some better name)
- Put it to the contextual data
- Use its regular interfaces
- Make sure we can easily

Follow-up: I think the concept of a bounce is that basic, it should be promoted to the ProcessorResult as a method "isBounce()".
But i'm also open for inputs if you know better approaches. :-)

I am hopping that I am going in right direction..:D

1. +++ b/inmail_cfortune/src/Plugin/inmail/Analyzer/CfortuneAnalyzer.php
   @@ -33,9 +35,10 @@ class CfortuneAnalyzer extends AnalyzerBase {
   +    /** @var \Drupal\inmail\BounceContext $bounce_context */
   +    $bounce_context = BounceContext::createFactory();
   

   Here you want new BounceContext().

   (The createFactory() method is a slightly strange concept that we use to make it a bit easier to call ensureAnalyzerResult().)

2. +++ b/inmail_cfortune/src/Plugin/inmail/Analyzer/CfortuneAnalyzer.php
   @@ -45,11 +48,12 @@ class CfortuneAnalyzer extends AnalyzerBase {
   +      $bounce_context->setRecipient(trim($handler->recipient));
   +      $result->setContext('recipient', new BounceContext(new ContextDefinition('string'), $bounce_context->getRecipient()));
   ...
   +      $result->setContext('handle_status', new Context(new ContextDefinition('string'), $bounce_context->setStatusCode(DSNStatus::parse($handler->status))));
   

   The $bounce_context usage here is correct, but it should replace the setContext() calls. Instead of $result->setContext('handle_status', ...) do $bounce_context->setStatus(...) Only in the end of the method we want something like $result->setContext('bounce', $bounce_context); Same for other analyzers.

3. +++ b/inmail_mailmute/src/Plugin/inmail/Handler/MailmuteHandler.php
   @@ -65,8 +65,8 @@ class MailmuteHandler extends HandlerBase implements ContainerFactoryPluginInter
   +    $result = $processor_result->getAnalyzerResult(BounceContext::TOPIC);
   

   This is where you want $result = $processor_result->getContext('bounce). (and probably rename $result to $bounce_context or so.) Same for other handlers.

4. +++ b/src/BounceContext.php
   @@ -10,7 +11,7 @@ namespace Drupal\inmail;
   +class BounceContext implements AnalyzerResultInterface, ContextInterface {
   

   Extend the Context class, and you will have implementations for many of the methods for free.

I think we can also remove createFactory(), summarize() and label() from the BounceContext class, those are analyzer result methods.

I did changes from #16.

As discussed with @toncic92, I think we are (I am) interpreting the context API a bit wrong. Extending from Context or ContextDefinition is probably not an option.

What we will need in the end is
- BounceDataDefinition extends ComplexDataDefinitionBase
- - getPropertyDefinitions() defines status code recipient and reason
- BounceData extends Map
- - has a @DataType annotation, similar to Mailbox
- - id is inmail_bounce
- - definition_class is BounceDataDefinition
- Analyzers and Handlers will have to check hasContext('bounce') (and handlers will have to call setContext() if false)

For later

As said on the Plugin Contexts documentation page, a plugin that uses a context should declare this in the plugin annotation. For example, MailmuteHandler can have context = @ContextDefinition("inmail_bounce", label = @Translation("Bounce analysis")).

HandlerManager would have to set that context when instantiating the plugin. How can it know what context to use? How can Analyzers best provide or modify available contexts?

I tried to implement staff from #19.

1. +++ b/inmail_phpmailerbmh/src/Plugin/inmail/Analyzer/PHPMailerBMHAnalyzer.php
   @@ -68,8 +70,18 @@ class PHPMailerBMHAnalyzer extends AnalyzerBase {
   +    /** @var \Drupal\inmail\DefaultAnalyzerResult $result */
   +    $result = $processor_result->ensureAnalyzerResult(DefaultAnalyzerResult::TOPIC, DefaultAnalyzerResult::createFactory());
   +    /** @var \Drupal\inmail\BounceDataDefinition $bounce_context */
   +    if ($result->hasContext('bounce')) {
   +      $bounce_context = $result->getContext('bounce');
   +    }
   +    else {
   +      $bounce_context = new Context(new ContextDefinition("inmail_bounce"));
   +      $result->setContext('bounce', $bounce_context);
   +    }
   +    /** @var \Drupal\inmail\BounceData $bounce_data */
   +    $bounce_data = $bounce_context->getContextData();
   

   Now this is the pattern that should be in all Analyzers and Handlers. But all except VerpAnalyzer and StandardDsnAnalyzer should just abort in the else block (probably after logging to $processor_result).

   That's a lot of added lines of code unfortunately. But I think they will get reduced in #2770567: Remove ability to extend results by "topic" now that there are contexts, which should be done immediately after this.

2. +++ b/src/BounceData.php
   @@ -88,26 +57,13 @@ class BounceAnalyzerResult implements AnalyzerResultInterface {
   -  public function getRecipient() {
   -    return $this->recipient;
   +  public function getReason() {
   +    return $this->get('reason');
   

   I think the patch gets a little extra dirty because the methods were moved around. I would prefer if we can make sure they are in the same order as before the change, just to get a cleaner patch.

3. +++ b/src/BounceData.php
   @@ -117,17 +73,18 @@ class BounceAnalyzerResult implements AnalyzerResultInterface {
   +    return $this->get('statusCode');
   
   +++ b/src/BounceDataDefinition.php
   @@ -0,0 +1,59 @@
   +      'status_code' => DataDefinition::create('string'),
   

   You define the property as status_code so that has to be used when calling get() and set() (not statusCode).

4. +++ b/src/BounceDataDefinition.php
   @@ -0,0 +1,59 @@
   +  const TOPIC = 'bounce';
   ...
   +  protected $statusCode;
   ...
   +  protected $recipient;
   ...
   +  protected $reason;
   

   These can all go away from BounceDataDefinition.

5. +++ b/src/Plugin/inmail/Deliverer/ImapFetcher.php
   @@ -283,30 +283,4 @@ class ImapFetcher extends FetcherBase implements ContainerFactoryPluginInterface
   -  /**
   -   * {@inheritdoc}
   -   */
   -  public function isAvailable() {
   

   This looks like you're creating the diff from another issue? Make sure to diff relative to the 8.x-1.x branch.

6. +++ b/tests/modules/inmail_test/src/Plugin/inmail/Analyzer/UnavailableAnalyzer.php
   @@ -23,7 +23,6 @@ class UnavailableAnalyzer extends AnalyzerBase {
        $default_result = $processor_result->getAnalyzerResult(DefaultAnalyzerResult::TOPIC);
   -
        // Do the fake body update. This should not be executed as we only execute
   

   Unrelated change.

Implementing comment #22.

Getting further one step at a time :) Now that the general structure is correct, please make sure to run relevant (or all) tests before uploading patches, to catch errors early.

1. +++ b/inmail_cfortune/src/Plugin/inmail/Analyzer/CfortuneAnalyzer.php
   @@ -33,8 +35,16 @@ class CfortuneAnalyzer extends AnalyzerBase {
   +    /** @var \Drupal\inmail\BounceDataDefinition $bounce_context */
   +    $bounce_context = $result->getContext('bounce');
   +    $bounce_data = $bounce_context->getContextData();
   

   $bounce_context doesn't need a typehint, it's a normal Context object. $bounce_data could use a typehint though, for BounceData. Same in other classes.

2. +++ b/inmail_mailmute/src/Plugin/inmail/Handler/MailmuteHandler.php
   @@ -65,21 +68,30 @@ class MailmuteHandler extends HandlerBase implements ContainerFactoryPluginInter
   +    if (!$result instanceof BounceDataDefinition) {
          return;
        }
   

   $result is definitely not an instance of BounceDataDefinition. You want if (!$result->hasContext('bounce')) here.

3. +++ b/inmail_phpmailerbmh/src/Plugin/inmail/Analyzer/PHPMailerBMHAnalyzer.php
   rename from src/BounceAnalyzerResult.php
   rename to src/BounceData.php
   

   The first fail in the Drupal CI report is caused by referring to this data type ("inmail_bounce") and it's not defined. The responsible plugin manager, TypedDataManager, declares that these plugins should be in the Plugin/DataType folder. Thus, this file must be moved to src/Plugin/DataType.

4. +++ b/src/BounceData.php
   @@ -107,7 +64,7 @@ class BounceAnalyzerResult implements AnalyzerResultInterface {
      public function getRecipient() {
   -    return $this->recipient;
   +    return $this->get('recipient');
      }
   

   This looks correct, but the setters must be altered in the same pattern (using set()).

5. +++ b/tests/src/Kernel/AnalyzerTest.php
   @@ -83,12 +81,16 @@ EOF;
   +    if ($result->hasContext('bounce')) {
   +      $bounce_context = $result->getContext('bounce');
   +    }
   
   +++ b/tests/src/Kernel/VerpTest.php
   @@ -61,9 +61,13 @@ class VerpTest extends KernelTestBase {
   +    if ($result->hasContext('bounce')) {
   +      $bounce_context = $result->getContext('bounce');
   +    }
   

   This is in tests, so the if() doesn't really make sense. We can rely on the context being present. (Same in other classes.)

6. +++ b/tests/src/Kernel/AnalyzerTest.php
   @@ -112,7 +114,7 @@ EOF;
       * Tests BounceAnalyzerResult by instantiation of the object and calls a method.
       */
      public function testBounce() {
   -    $bouncetest = new BounceAnalyzerResult();
   +    $bouncetest = new BounceContext();
        $bouncetest->summarize();
      }
   

   This test method doesn't make sense now that we removed summarize(), so it should be removed.

7. +++ b/tests/src/Unit/Plugin/inmail/Analyzer/StandardDSNReasonAnalyzerTest.php
   @@ -30,13 +30,18 @@ class StandardDSNReasonAnalyzerTest extends InmailUnitTestBase {
   -      $this->assertNull($result);
   +      $this->assertNull($bounce_context);
   

   $this->assertFalse($result->hasContext('bounce')) would look nicer.

Fixing failed testing and implementing comm #26.

1. +++ b/inmail_cfortune/src/Plugin/inmail/Analyzer/CfortuneAnalyzer.php
   @@ -33,8 +33,16 @@ class CfortuneAnalyzer extends AnalyzerBase {
   +    if (!$result->hasContext('bounce')) {
   +      return;
   

   This early exit didn't exist before. I guess it's only needed on error - you need to add a message to the log.

2. +++ b/inmail_mailmute/src/Plugin/inmail/Handler/MailmuteHandler.php
   @@ -65,21 +65,26 @@ class MailmuteHandler extends HandlerBase implements ContainerFactoryPluginInter
   +    $status_code = $bounce_data->getStatusCode();
   
   +++ b/src/BounceDataDefinition.php
   @@ -0,0 +1,29 @@
   +      'status_code' => DataDefinition::create('string'),
   
   +++ b/src/Plugin/DataType/BounceData.php
   @@ -107,17 +65,21 @@ class BounceAnalyzerResult implements AnalyzerResultInterface {
      public function getStatusCode() {
   ...
   +      return DSNStatus::parse($this->get('status_code')->getValue());
   

   This is no more a string, please declare properly.

3. +++ b/inmail_mailmute/tests/src/Kernel/InmailMailmuteTest.php
   @@ -114,9 +114,19 @@ class InmailMailmuteTest extends KernelTestBase {
   +      if (!$result->hasContext('bounce')) {
   +        return;
   

   This is a test and this return changes what the test previously tested. Plz remove return.

4. +++ b/src/Plugin/DataType/BounceData.php
   @@ -154,24 +116,4 @@ class BounceAnalyzerResult implements AnalyzerResultInterface {
   -  public function summarize() {
   ...
   -      $summary['recipient'] = $this->getRecipient();
   ...
   -      $summary['code'] = $this->getStatusCode()->getCode();
   

   This creates a regression. Previously, the summary of a bounce could display the recipient and the status code. With dropping this, the DefaultAnalyzerResult only lists the context keys (which is 'bounce').

   I guess we will need to introduce a summaryInterface and implement it in BounceData.
   OK, could be a follow-up.

5. +++ b/src/Plugin/inmail/Analyzer/StandardDSNAnalyzer.php
   @@ -40,8 +45,20 @@ class StandardDSNAnalyzer extends AnalyzerBase {
   +    $result = $processor_result->ensureAnalyzerResult(DefaultAnalyzerResult::TOPIC, DefaultAnalyzerResult::createFactory());
   +    if ($result->hasContext('bounce')) {
   ...
   +    else {
   ...
   +      $typed_data_manager =\Drupal::service('typed_data_manager');
   

   Previously, this complexity was not needed that's why ensureXYZ exists - it guarantees the BounceResult was created.
   What previously was a result is now a context. The DefaultAnalyzerResult always exists.

   You should convert
   $result = $processor_result->ensureAnalyzerResult(BounceAnalyzerResult::TOPIC, BounceAnalyzerResult::createFactors);
   to

   $result = $processor_result->getAnalyzerResult(DefaultAnalyzerResult::TOPIIC);
   $bounce_data = $result->ensureContext('bounce', BounceData::createFactory());

   So we need to convert the ensureAnalyzerResult to a construct to ensure that the context exists.

   As a result, all those early returns in Analyzers are also not needed.

6. +++ b/tests/src/Kernel/AnalyzerTest.php
   index 0000000..69b6456
   --- /dev/null
   
   --- /dev/null
   +++ b/tests/src/Kernel/StandardDSNAnalyzerTest.php
   
   +++ b/tests/src/Kernel/VerpTest.php
   index 863ce6b..0000000
   --- a/tests/src/Unit/Plugin/inmail/Analyzer/StandardDSNAnalyzerTest.php
   
   --- a/tests/src/Unit/Plugin/inmail/Analyzer/StandardDSNAnalyzerTest.php
   +++ /dev/null
   

   Make sure renames are properly diffed.

7. +++ b/tests/src/Kernel/StandardDSNAnalyzerTest.php
   @@ -0,0 +1,128 @@
   +    if (!$result->hasContext('bounce')) {
   +      return;
   
   +++ b/tests/src/Kernel/VerpTest.php
   @@ -61,9 +61,16 @@ class VerpTest extends KernelTestBase {
   +    if (!$result->hasContext('bounce')) {
   +      return;
   
   +++ b/tests/src/Unit/Plugin/inmail/Analyzer/StandardDSNReasonAnalyzerTest.php
   @@ -30,13 +30,21 @@ class StandardDSNReasonAnalyzerTest extends InmailUnitTestBase {
   +    if (!$result->hasContext('bounce')) {
   +      return;
   

   Again, a test. You know what you expect. Do not early exit.

#2 . I think that this $this->get('status_code') return the TypedDataInterface object and the getValue() return string.

I didn't now how to implement $bounce_data = $result->ensureContext('bounce', BounceData::createFactory()); and ask @Arla, he show me to do like ensureContext($name, $data_type).

Test was failing here $this->assertNull($result); and I change to use $this->assertFalse(is_null($bounce_context));.

And why are u dropping the StandardDSNAnalyzerTest?

+++ b/src/Plugin/DataType/BounceData.php
@@ -74,16 +32,16 @@ class BounceAnalyzerResult implements AnalyzerResultInterface {
   public function setStatusCode(DSNStatus $code) {

@@ -107,17 +65,21 @@ class BounceAnalyzerResult implements AnalyzerResultInterface {
   public function getStatusCode() {
...
+      return DSNStatus::parse($this->get('status_code')->getValue());

But you are NOT returning a string, so the data type needs to be properly defined - however then DSNStatus is not defined as data type.
Don't know yet what direction we want to go here. :-)

BounceData stores status_code as a string (StringData) internally, you can alway get it with get('status_code'). BounceData just adds pretty API methods, and there (getStatusCode, setStatusCode) we do conversion from/to a DSNStatus object. So the current usage seems correct to me.

OK will check again, the data type definition looks fine then indeed.

OK... We're almost there.

1. +++ b/src/BounceDataDefinition.php
   @@ -0,0 +1,29 @@
   + * Contains analyzer results.
   

   I think, we should say that it contains bounce information from the analyzers.

2. +++ b/src/DefaultAnalyzerResult.php
   @@ -290,4 +292,31 @@ class DefaultAnalyzerResult implements AnalyzerResultInterface {
   +  public function ensureContext($name, $data_type) {
   ...
   +    if ($this->hasContext($name)) {
   +      $bounce_data = $this->getContext($name)->getContextData();
   

   Follow-Up: We should add a check if the context has the expected datatype and throw an exception otherwise. This could help avoid with strange problems of type mismatch and incompatible declarations.

3. +++ b/src/Plugin/DataType/BounceData.php
   @@ -154,24 +116,4 @@ class BounceAnalyzerResult implements AnalyzerResultInterface {
   -  public function summarize() {
   

   So yeah, this remains for follow-ups.

4. +++ b/src/ProcessorResult.php
   @@ -32,6 +32,11 @@ class ProcessorResult implements ProcessorResultInterface {
   +    $this->analyzerResults[DefaultAnalyzerResult::TOPIC] = new DefaultAnalyzerResult();
   

   This is a duplicate initialisation. We are initialising this here:

     public function process($raw, DelivererConfig $deliverer) {
   ...
         $default_result = $result->ensureAnalyzerResult(DefaultAnalyzerResult::TOPIC, DefaultAnalyzerResult::createFactory());
   

And please check git settings so that moving a file is reported as move in the diff and not as deleting a file and creating a different file. Reviews are otherwise horrible.

#4. If I remove initialization in constructor test fails because the DefaultAnalyzerResult is not existing.

Added ensureAnalyzerResult in StandardDSNAnalyzer and StandardDSNReasonAnalyzer.

+++ b/src/ProcessorResult.php
@@ -31,7 +31,7 @@ class ProcessorResult implements ProcessorResultInterface {
+  ¶

You added a trailing whitespace on this line! Plz check your phpstorm settings to avoid this kind of problems.
I fixed it locally prior to commit, but...

i see on the console output of CI:
10:30:41 PHP Fatal error: Class 'Drupal\Tests\token\Kernel\KernelTestBase' not found in /var/www/html/modules/inmail/tests/src/Kernel/StandardDSNReasonAnalyzerTest.php on line 19
10:30:41
10:30:41 Fatal error: Class 'Drupal\Tests\token\Kernel\KernelTestBase' not found in /var/www/html/modules/inmail/tests/src/Kernel/StandardDSNReasonAnalyzerTest.php on line 19

So it seems this is a CI error indeed? Passes locally...

Thus committed. We will see if this the CI gets fine with it when testing HEAD...

Hm, now HEAD fails with those messages.. Do we have a dependency problem? :-)
Oops:
* @deprecated in Drupal 8.0.x, will be removed before Drupal 8.2.x. Use
* \Drupal\KernelTests\KernelTestBase instead.
*
So it tests with latest Drupal 8.3 and it is removed already.
Let's fix this in a follow-up.

--- a/tests/src/Unit/Plugin/inmail/Analyzer/StandardDSNReasonAnalyzerTest.php
+++ b/tests/src/Kernel/StandardDSNReasonAnalyzerTest.php
...
-use Drupal\Tests\inmail\Unit\InmailUnitTestBase;
+use Drupal\Tests\token\Kernel\KernelTestBase;

You accidentally used a base class from the Token module. Change this to the standard KernelTestBase and post the patch here, I don't think this needs a separate follow-up.

True, I didn't see it. That should fix the problem.

I change that, but since Miro commit this, I get very different patch.

That's exactly the patch I wanted :) Committed.