Add EntityInterface::validate()

Made some progress, not sure whether in good direction... Patch contains implementation of validate function as well test support for it. Would be nice if some 'detailed' directives are provided...

Some feedbacks :

1. +++ b/src/MIME/EntityInterface.php
   @@ -83,4 +83,12 @@ interface EntityInterface {
   +   * Check that message contains Header fields such as Subject, To, From, ID.
   ...
   +   *   Returns TRUE if message is valid, otherwise FALSE.
   

   Your documentation here should be generic to all classes implementing this interface, not specific to Message objects.

2. +++ b/src/MIME/Message.php
   @@ -51,4 +51,16 @@ class Message extends Entity implements MessageInterface {
   +  public function validate() {
   

   You can move your documentation specific to Message's inside here.

3. +++ b/src/MIME/Message.php
   @@ -51,4 +51,16 @@ class Message extends Entity implements MessageInterface {
   +        if (is_null($this->getHeader()->getFieldBody($key))) {
   

   You just want to know if the field exists. You can use hasField() on the Header object to avoid the overhead of returning and trimming the body.

New patch that focuses on issues stated in comments #2 and #3

Here are some feedbacks. In my humble opinion, returning an array of arbitrary strings from validate() adds too much complexity for what it's worth. Maybe we should put in some more thoughts?

1. +++ b/src/MIME/Header.php
   @@ -213,4 +213,36 @@ class Header {
   +   * Returns the bodies of the field with the given name.
   

   A field has a single body, but you return the body of multiple fields now. "Returns the body of the fields with the given name.".

2. +++ b/src/MIME/Header.php
   @@ -213,4 +213,36 @@ class Header {
   +    $field_bodies = array();
   

   Please use the short array syntax, always.

3. +++ b/src/MIME/Header.php
   @@ -213,4 +213,36 @@ class Header {
   +    if ($field_bodies === FALSE) {
   

   This will never evaluate to TRUE, since you're doing a strict (type) comparison and you initialize the variable as an empty array. if (!$field_bodies) {...} will be sufficient.

4. +++ b/src/MIME/Header.php
   @@ -213,4 +213,36 @@ class Header {
   +    foreach ($field_bodies as $key => $field) {
   +      $body[] = trim($field_bodies[$key]['body']);
   +    }
   

   Can't you just assign the 'body' array element and trim it directly in the first loop?

5. +++ b/src/MIME/Message.php
   @@ -51,4 +51,37 @@ class Message extends Entity implements MessageInterface {
   +    // Date and From header fields are only required fields in message by IS.
   

   Please mention to which issue you are referring to by "IS" or, better, mention directly the source (RFC 5322 format).

6. +++ b/src/MessageProcessor.php
   @@ -89,6 +89,12 @@ class MessageProcessor implements MessageProcessorInterface {
   +        $this->loggerChannel->info('Message Validation failed with message "%message" ',['%message' => implode(', ', $validate_message['value'])]);
   

   Discussion : Should we exit/throw here?

7. +++ b/tests/src/Unit/MIME/MessageTest.php
   @@ -72,4 +72,31 @@ class MessageTest extends UnitTestCase {
   +
   +
   +
   

   Unnecessary spaces. I also think that it would be interesting to test for the case where you have all necessary fields, but duplicates. Right now you're testing the case where you are missing a field + duplicate (should be separate) and when it's fine.

+++ b/src/MIME/EntityInterface.php
@@ -83,4 +83,12 @@ interface EntityInterface {
+   * @return bool
...
+  public function validate();

+++ b/src/MIME/Message.php
@@ -51,4 +51,38 @@ class Message extends Entity implements MessageInterface {
+   * @return array
...
+  public function validate() {

aha... :-)

Whatever validate returns should allow us to know what message is related to what field.
Thus the messages should always be keyed..

And since this stuff is hierarchical, we need to maintain key hierarchy merging with a separator.

However, i also think we should check how core does these kind of things. We are now defining something on our own, right?

Rerolling patch since it failed to apply

Regarding comment #10... There are two variations of validate functions how in core it is done. In Symfony\Component\Console\Inputymfony\Component\Console\Input::validate() function only throws an exception, while in Drupal\Core\Entity\FieldableEntityInterface validate function returns list of constraint violations. So, would be nice to make agreement on which way to go further (throw only an exception, or return error messages)...

Fixed some small issues regarding comments, tests. Still needs agreement and change logic of function validate whether to return just boolean value or associative array...

Regarding validate(), I think that it has already been decided to just return a boolean :

In this case, I suggest that validate() should just return at boolean result, because it's the simplest API and we cannot really motivate anything more complex imo.

Moving back to needs work..

Hm hm. What's the value of refurning true/false if u do all the cases checking.

If something fails, i need either info inthe api about what was the problem or if it is processing related, i would want to have a message on the logger that explains what the validation problem was.

So what is the goal of the new validate method?

Changed the logic of validate function to return only boolean value and making getter method for fetching error messages. Modified a little comments to satisfy changes as well as tests.

Bug fix due to automated test failure

Looks good overall. Here are some feedbacks :

1. +++ b/src/MIME/Entity.php
   @@ -131,4 +140,26 @@ class Entity implements EntityInterface {
   +    // Empty the error message storage.
   

   Just as a fyi, the word "storage" in this context might lead to confusion.

2. +++ b/src/MIME/Header.php
   @@ -213,4 +213,32 @@ class Header {
   +    if (!$body) {
   +      return [];
   +    }
   

   The original getFieldBody() implementation would return a string or NULL, if no match was found, which is why we had that check. Makes sense to return an empty array, but then we don't really need that check anymore, since we initialized the variable as an empty array. However, we don't really want the overhead of filtering the empty array. What about :

   return ($filter && $body) ? preg_replace('/\([^)]*\)/', '', $body) : $body;

3. +++ b/src/MIME/Message.php
   @@ -51,4 +51,36 @@ class Message extends Entity implements MessageInterface {
   +    $counter = count($this->getHeader()->getFieldBodies('Received'));
   +    if ($counter > 1) {
   +      $result = FALSE;
   +      $this->error_messages['Received'] = $counter . ' Received fields';
   +    }
   +    $counter = count($this->getHeader()->getFieldBodies('From'));
   +    if ($counter > 1) {
   +      $result = FALSE;
   +      $this->error_messages['From'] = $counter . ' From fields';
   +    }
   

   Might be more elegant to loop over the fields here. Also, we're doing these checks even if we know that the field is absent. Maybe we could move this into the foreach loop above as an else case?

4. +++ b/src/MessageProcessor.php
   @@ -97,6 +97,15 @@ class MessageProcessor implements MessageProcessorInterface {
   +        $this->loggerChannel->info('Message Validation failed with message %message',['%message' => implode(', ', $message->getValidationErrors())]);
   

   Missing space after comma ;).

5. +++ b/src/MessageProcessor.php
   @@ -97,6 +97,15 @@ class MessageProcessor implements MessageProcessorInterface {
   +          $event->setSeverity(RfcLogLevel::ERROR);
   

   We are not including Drupal\Core\Logger\RfcLogLevel. I think this will explode when we reach that line. Do we have a test to cover that case?

6. +++ b/tests/src/Unit/MIME/MessageTest.php
   @@ -72,4 +72,40 @@ class MessageTest extends UnitTestCase {
   +    $this->assertFalse(empty($message->getValidationErrors()));
   ...
   +    $this->assertFalse(empty($message->getValidationErrors()));
   

   Maybe we should assert the actual validation errors?

Some small changes considering comment #22

Adding a related bug-fix issue.