Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow-up to #2754253: Lack of standard result in collaboration of analyzers
Problem/Motivation
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.
Proposed resolution
Discuss the possibility to merge the result objects...
Comment | File | Size | Author |
---|---|---|---|
#46 | replace_bounce_analyzer-2769061-46.patch | 593 bytes | toncic |
| |||
#38 | interdiff-2769061-35-38.txt | 1.69 KB | toncic |
#38 | replace_bounce_analyzer-2769061-38.patch | 33.82 KB | toncic |
| |||
#35 | interdiff-2769061-29-35.txt | 1.91 KB | toncic |
#35 | replace_bounce_analyzer-2769061-35.patch | 33.49 KB | toncic |
|
Comments
Comment #2
ArlaI 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.
Comment #3
ArlaFor 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:
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?
Comment #4
miro_dietikerHm.. 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?
Comment #5
ArlaIn 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.
Comment #6
toncic CreditAttribution: toncic at MD Systems GmbH commentedFor initial patch, I change to use DefaultAnalyzerResult instead of BounceAnalyzerResult. I forgot to remove BounceAnalyzerResult from usages, I will in next patch.
Comment #8
ArlaYeah 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().
Comment #9
miro_dietikerMajor architectural refactoring. Thus critical.
Comment #10
toncic CreditAttribution: toncic at MD Systems GmbH commentedUsing getters and setters from
DefaultAnalyzerResult
instead function fromBounceAnalyzerResult
.Test fails in
nmailMailmuteTest::testBounceCounting
when we want to access tosendstate
which does not exist.Comment #12
miro_dietikerI'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. :-)
Comment #13
toncic CreditAttribution: toncic at MD Systems GmbH commentedI am hopping that I am going in right direction..:D
Comment #14
toncic CreditAttribution: toncic at MD Systems GmbH commentedComment #16
ArlaHere you want
new BounceContext()
.(The createFactory() method is a slightly strange concept that we use to make it a bit easier to call ensureAnalyzerResult().)
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.This is where you want
$result = $processor_result->getContext('bounce)
. (and probably rename $result to $bounce_context or so.) Same for other handlers.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.
Comment #17
toncic CreditAttribution: toncic at MD Systems GmbH commentedI did changes from #16.
Comment #19
ArlaAs 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?
Comment #20
toncic CreditAttribution: toncic at MD Systems GmbH commentedI tried to implement staff from #19.
Comment #22
ArlaNow 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.
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.
You define the property as status_code so that has to be used when calling get() and set() (not statusCode).
These can all go away from BounceDataDefinition.
This looks like you're creating the diff from another issue? Make sure to diff relative to the 8.x-1.x branch.
Unrelated change.
Comment #23
toncic CreditAttribution: toncic at MD Systems GmbH commentedImplementing comment #22.
Comment #25
toncic CreditAttribution: toncic at MD Systems GmbH commentedComment #26
ArlaGetting 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.
$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.
$result is definitely not an instance of BounceDataDefinition. You want
if (!$result->hasContext('bounce'))
here.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.
This looks correct, but the setters must be altered in the same pattern (using set()).
This is in tests, so the if() doesn't really make sense. We can rely on the context being present. (Same in other classes.)
This test method doesn't make sense now that we removed summarize(), so it should be removed.
$this->assertFalse($result->hasContext('bounce'))
would look nicer.Comment #27
toncic CreditAttribution: toncic at MD Systems GmbH commentedFixing failed testing and implementing comm #26.
Comment #28
miro_dietikerThis early exit didn't exist before. I guess it's only needed on error - you need to add a message to the log.
This is no more a string, please declare properly.
This is a test and this return changes what the test previously tested. Plz remove return.
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.
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
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.
Make sure renames are properly diffed.
Again, a test. You know what you expect. Do not early exit.
Comment #29
toncic CreditAttribution: toncic at MD Systems GmbH commented#2 . I think that this
$this->get('status_code')
return theTypedDataInterface
object and thegetValue()
return string.I didn't now how to implement
$bounce_data = $result->ensureContext('bounce', BounceData::createFactory());
and ask @Arla, he show me to do likeensureContext($name, $data_type)
.Test was failing here
$this->assertNull($result);
and I change to use$this->assertFalse(is_null($bounce_context));
.Comment #31
miro_dietikerAnd why are u dropping the StandardDSNAnalyzerTest?
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. :-)
Comment #32
ArlaBounceData 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.
Comment #33
miro_dietikerOK will check again, the data type definition looks fine then indeed.
Comment #34
miro_dietikerOK... We're almost there.
I think, we should say that it contains bounce information from the analyzers.
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.
So yeah, this remains for follow-ups.
This is a duplicate initialisation. We are initialising this here:
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.
Comment #35
toncic CreditAttribution: toncic at MD Systems GmbH commented#4. If I remove initialization in constructor test fails because the DefaultAnalyzerResult is not existing.
Comment #38
toncic CreditAttribution: toncic at MD Systems GmbH commentedAdded ensureAnalyzerResult in StandardDSNAnalyzer and StandardDSNReasonAnalyzer.
Comment #41
miro_dietikerYou 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...
Comment #43
miro_dietikerHm, 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.
Comment #44
ArlaYou 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.
Comment #45
johnchqueTrue, I didn't see it. That should fix the problem.
Comment #46
toncic CreditAttribution: toncic at MD Systems GmbH commentedI change that, but since Miro commit this, I get very different patch.
Comment #48
ArlaThat's exactly the patch I wanted :) Committed.