EntityResource: Provide comprehensive test coverage for Message entity

Working on this.

Yay!

@Wim Leers given that core does not store Message entities, what do we test here?

I think the purpose here is to be able to send messages as if you were submitting a "contact" form, but via REST, so you can have your own interface.

Which means we need to test only POST, not GET, nor PATCH nor DELETE. We can override those test methods and call self::markTestSkipped();.

To be able to test the POST method, we'll need to modify \Drupal\contact\MessageForm::save(). It currently does this:

  public function save(array $form, FormStateInterface $form_state) {
    $message = $this->entity;
    $user = $this->currentUser();
    // Save the message. In core this is a no-op but should contrib wish to
    // implement message storage, this will make the task of swapping in a real
    // storage controller straight-forward.
    $message->save();
    $this->mailHandler->sendMailMessages($message, $user);
    $contact_form = $message->getContactForm();

    $this->flood->register('contact', $this->config('contact.settings')->get('flood.interval'));
    if ($submission_message = $contact_form->getMessage()) {
      drupal_set_message($submission_message);
    }

    // To avoid false error messages caused by flood control, redirect away from
    // the contact form; either to the contacted user account or the front page.
    if ($message->isPersonal() && $user->hasPermission('access user profiles')) {
      $form_state->setRedirectUrl($message->getPersonalRecipient()->urlInfo());
    }
    else {
      $form_state->setRedirectUrl($contact_form->getRedirectUrl());
    }
  }

The entity type annotation has this:

*     "storage" = "Drupal\Core\Entity\ContentEntityNullStorage",

We need most of the logic in MessageForm::save() to be moved to the storage handler, so that the e-mail is sent from there, and so that the flood control is also handled there. Then only the redirecting and message setting is left in MessageForm::save().

Of course, this will need sign-off from the contact.module maintainers. So please don't start working on it just yet.

We need most of the logic in MessageForm::save() to be moved to the storage handler, so that the e-mail is sent from there, and so that the flood control is also handled there. Then only the redirecting and message setting is left in MessageForm::save().

Do we need a separate issue for this?

There was intend to make mail send optional #2325805: Make email sending optional on a per-contact form basis
So probably mail send better to move out of form submit to pre|post save somehow

Flood control could be moved to entity validation level but looks that too late.
IMO flood control should fire early, so kinda #2431357: Extract a reusable UserAuthFlood service and reuse it in UserLoginForm and BasicAuth and UserAuthenticationController better to decouple it and use at form level & a rest plugin (not sure I understand in which plugin that may live)

So believe it or not, contact storage was formed from a test module in core. So you should be able to turn that on in your test and have message storage.

With regards to email, yeah splitting that out is good. And the flood stuff, validation makes sense, but making that generic by way of the issue andypost linked is probably best

+1 to what @larowlan and @andypost said. We all want contact storage in the core and we are constantly improving that in contrib as well but in this case, I'd agree with @andypost and @larowlan we are better of moving the functionality than introducing storage. This means we need a title update so tagging it.

The issue title is still accurate, the implementations simply changed :)

Note that this blocks REST test coverage and of course functionality from being completed.

btw Since 4.x in action we can decide how properly "Decouple mail sending" into configurable action
It just needs test for allowed tokens and UI

Can one of the contact.module maintainers provide a status update for this?

Hey, the 'contact as an 80% replacement for webform' initiative never got buy in.

Not sure what you need from us (contact module maintainers) here?

Well, if no messages are stored, then there are no \Drupal\contact\Entity\Message entities one can GET via REST either.

Per #18, that will not happen, at least not in the foreseeable future.

Consequently, we should:

I filed #2878513: Forbid view/edit/delete in ContactMessageAccessControlHandler

but create access could be fixed here, maybe re-title the issue again?

but create access could be fixed here, maybe re-title the issue again?

Current create access is correct/not a problem… so I'm not sure what you mean?

Yeah, I'm fine with the plan in comment 6.
Ideally it would be an event to avoid the coupling, but we don't have entity events - so in the storage handler is fine.
Can you add those new features via composition instead of inheritance, because we'll need to do similar in contact_storage.

i.e. if you extend ContentEntityNullStorage and add the functionality via a trait, we can do the same in contact_storage, where we extend SqlContentEntityStorage.

Thanks

#22: thanks for the confirmation! It sounds like the contact module maintainers are not interested in taking this on? (Which is fine! Just checking.)

I'm interested, but that shouldn't stop anyone else grabbing it - there's only so much time in the day :)

Ok! :)

Taking a first stab at this. This is one of the last few blockers of #2824572: Write EntityResourceTestBase subclasses for every other entity type..

I had to take some special measures because Message entities have special limitations:

1. GET/PATCH/DELETE could not be tested — this we already discussed, and should be no surprise. So I overrode their test methods and replaced their bodies with just this: $this->markTestSkipped()
2. Because Message entities are not stored, I had to make 3 changes to EntityResourceTestBase:
   1. it's impossible to reload the stored entity to get the new field and then set default values for it (for REST test-only fields)
   2. the GET response body's field values cannot be compared with the stored entity's field values
   3. there's no need to DELETE the first entity created to be able to perform a second successful POST request
3. getExpectedNormalizedEntity() does not need to be implemented
4. Most tricky: because there is no label entity key, entity validation didn't work as expected: it'd fail to validate the subject field, of which there must evidently only be one

This should be green :) Next step: expanding this to all other authentication mechanisms, and to also test HAL+JSON.

Next step: expanding this to all other authentication mechanisms, and to also test HAL+JSON.

Done.

Rather than allowing GET, PATCH and DELETE routes to be created for Message entities, and those routes then not being able to work at all, we should remove those routes.

I think the title can actually now be just this. The scope in #6 was broader: it also aimed to move the "send e-mail" logic. But that's something that's really pretty much out of scope. The patches above are testing the REST aspect. Whether e-mail is sent or not, that's something that should be fixed and tested within the Contact module.

1. +++ b/core/modules/contact/contact.module
   @@ -235,3 +235,12 @@ function contact_form_user_admin_settings_submit($form, FormStateInterface $form
   +  $definitions['entity:contact_message']['class'] = '\Drupal\contact\Plugin\rest\resource\ContactMessageResource';
   

   Nit, ContactMessageResource::class

2. +++ b/core/modules/contact/src/Entity/Message.php
   @@ -26,6 +26,7 @@
   + *     "label" = "subject",
   

   why that needed?

The label thing is a good question. Note that this might not be visible here because there is no storage, but adding entity keys changes the schema (makes fields required), which would be a problem for contact_storage in two ways:

a) It would require an update function (which core could do, even though default storage is null, it would just do nothing then),
b) hiding the subject field, which is a very common thing to do, would no longer work as it would result in an error.

Just one nit and rest of it looking really solid. Thanks, @Wim Leers reviewing this was fun. Let's address #32 and this one nit before RTBC.

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -881,8 +890,10 @@ public function testPost() {
   +    if (get_class($this->entityStorage) !== ContentEntityNullStorage::class) {
   

   Let's add a comment here as well.

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Message/MessageResourceTestBase.php
   @@ -0,0 +1,154 @@
   +      'message' => 'Llamas are awesome!',
   

   Indeed!

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Message/MessageResourceTestBase.php
   @@ -0,0 +1,154 @@
   +          'value' => 'Dramallama',
   

   lol

#31

1. Great catch, fixed!
2. See #28.4:
   

   Most tricky: because there is no label entity key, entity validation didn't work as expected: it'd fail to validate the subject field, of which there must evidently only be one

#33: Hm, so what's the conclusion? Does this patch need to do something extra? It sounds like only contact_storage would need to be updated. Would that be acceptable? Do you want me to create an issue?

#34:

1. Done.
2. :)
3. :P

Thanks for addressing the feedback. Can we also update the IS please so that we can remove the tag? I think we can RTBC it whenever @Berdir is ready.

I don't understand the validation thing, why wouldn't it be validated if the entity label isn't there? user name is also not defined as entity label, how is that different?

What I mentioned as b) is a blocker. We can not make label an entity key because label can not be required as it can be hidden in the UI. This is the same for node, but it was like that since 8.0, this would break tons of existing contact forms.

I don't understand the validation thing, why wouldn't it be validated if the entity label isn't there?

Attached is a patch that reverts this change.

If this passes, great, then we don't need that change. Would love to be wrong :)

Assigning to @Berdir for feedback.

At least the first test fail has nothing to do with validation that's a problem with your test setup.

You already had that once with user entities and added a workaround:
$label_field = $this->entity->getEntityType()->hasKey('label') ? $this->entity->getEntityType()->getKey('label') : static::$labelFieldName;

But static::$labelFieldName is empty, so you end up sending a structure with an empty array key and \Drupal\serialization\Normalizer\FieldableEntityNormalizerTrait::denormalizeFieldData() explodes on that.

Beside setting that, I'd suggest two additional things:

* Add a hasField() check in \Drupal\serialization\Normalizer\FieldableEntityNormalizerTrait::denormalizeFieldData() to avoid an exception or at least provide a more meaningful exception or maybe ignore that data and leave it to custom normalizer subclasses to deal with it?

* Wrap that getKey() code in a helper method, it exists a few times and throw an exception when you end up with an empty label key. Then you could also directly override that method instead of the property in theory.

After that it fails with a different error:

1) Drupal\Tests\hal\Functional\EntityResource\Message\MessageHalJsonAnonTest::testPost
Failed asserting that 201 is identical to 422.

/core/modules/rest/tests/src/Functional/ResourceTestBase.php:353
core/modules/rest/tests/src/Functional/ResourceTestBase.php:372
/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:801

I manually verified that a Message object with more than one subject values fails validation, so possibly it is another test setup problem?

$message = Message::create(['contact_form' => 'personal', 'subject' => ['a', 'b'], 'message' => 'foo']); 
$violations = $message->validate()                                                                                                                                                                    foreach ($violations as $violation) {                                                                                                                                                                     
echo $violation->getPropertyPath() . ': ' . $violation->getMessage();                                                                                                                                              
}

subject: <em class="placeholder">Subject</em>: this field cannot hold more than 1 values.

Also, we actually have a test storage backend for message, and it might actually make sense to have at least optionally a version of that test that runs with that module enabled?

Thanks for the super fast feedback! 👏

I'll look at this ASAP, might be a while because DrupalCon.

Once again, Berdir++ Can't believe I missed that.

Also, one \n too many, caused a coding standard fail.

Beside setting that, I'd suggest two additional things:
[…]
After that it fails with a different error:

No more failures after that single tiny change for me. IMHO this patch is now completely ready: it's 99% test coverage, 1% code addition (to ensure Message entities can only be POSTed). 0 API changes, 0 data structure changes.

This seems safe to RTBC to me.

Thanks, for addressing the feedback @Wim Leers.

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Message/MessageResourceTestBase.php
@@ -0,0 +1,159 @@
+      $this->assertTrue(FALSE);
...
+      $this->assertTrue(FALSE);
...
+      $this->assertTrue(FALSE);

Should we use $this->fail() here - it's more explicit

or alternatively $this->setExpectedException()

https://thephp.cc/news/2016/02/questioning-phpunit-best-practices

❤️

PHPStorm--

I thought there was something like that, but PHPStorm's autocomplete is apparently unable to find ->fail()… Much better!

#48 was already stupid of me, but I can't believe I didn't even think about $this->setExpectedException() is … kind of inexplicable/unforgivable. That's of course by far the better solution!

RTBC +1

Adding review credits

Committed as 4d43652 and pushed to 8.5.x. Thanks!

Hurray! One less blocker for #2824572: Write EntityResourceTestBase subclasses for every other entity type., #2868035: Test that all core content+config entity types have functional REST test coverage and #2910883: Move all entity type REST tests to the providing modules.