Convert contact message entities to the new Entity Field API

Here's an initial patch, mostly trivial except we might need to deal with the user special case and make that a locked category or so so that we have a bundle. EntityNG doesn't like it if you define a bundle but don't specify it.

Eh.

EntityNG doesn't like it if you define a bundle but don't specify it.

Yep, not knowing the (complete) type of an entity makes things complicated. However entity_create() already documents that the bundle is required anyway. If we add support for optional bundles, I think this state should be valid through the overall life-cycle of an entity, i.e. also allow a loaded $entity to be of type 'contact_message' or 'contact_message.category'.

In this case, it's not about not knowing the bundle. It means it doesn't have one. Which could be a specific locked bundle.

Here's an initial attempt at making personal a special contact category. See also #1831928: Support a 'locked' property on ConfigEntities.

This actually allows you to attach fields to personal contact forms as well. But when I wanted to test it I noticed that the field UI seems to be broken currently, so we're missing tests there I guess? Not a problem of this issue AFAICT.

Patch looks good. Suppose "personal" better to leave in it's own issue #1856556: Convert user contact form into a contact category/bundle

It's a blocker for this issue (EntityNG always requires a bundle for entities with multiple bundles), so we either have to postpone this issue on that or integrate it.

Re-rolled on top of #1856556: Convert user contact form into a contact category/bundle.

Re-roll after the personal bundle issue went in. Let's see if test are in a better shape now.

#12: contact-message-ng-1983548-12.patch queued for re-testing.

Patch is really straight-forward. Here a few remarks:

+++ b/core/modules/contact/lib/Drupal/contact/MessageStorageController.php
@@ -0,0 +1,61 @@
+      'label' => t('Category'),

label and description does not really match. Usually we call it ID always, maybe we want to revisit that as part of http://drupal.org/node/1233394 - the way we do it usually for fields is that we don't have ID in the name.

Also the category is required right? So it should denote:
required => TRUE

+++ b/core/modules/contact/lib/Drupal/contact/MessageStorageController.php
@@ -0,0 +1,61 @@
+      'label' => t("The message subject"),

Should use single quotes.

+++ b/core/modules/contact/lib/Drupal/contact/MessageStorageController.php
@@ -0,0 +1,61 @@
+      'label' => t("The message text"),

Should use single quotes.

+++ b/core/modules/contact/lib/Drupal/contact/MessageStorageController.php
@@ -0,0 +1,61 @@
+      'label' => t("Copy"),

Should use single quotes.

Hm. This started of as a simple re-roll and then I started testing and noticed oooooh so many broken things. And most of them are now fixed, with various things covered by tests now.

Here's a list of changes and bugfixes
- Fixed the things pointed out by @fago above
- Also removed the translatable TRUE for recipient that I forgot in there
- Made email an email_field, that I had to copy to Drupal\Core\Entity
- Made copy a boolean_field
- Added methods to the MessageInterface, using them and removed the public properties init() hack, similar to the files conversion.

(Now we're getting to the fun part)
- Manage fields/display were missing completely, added the necessary entity type annotation stuff, Had to trick a bit with the edit form, as that somehow broke as a result of that.
- Noticed a @todo in contact_field_extra_fields() that we forgot to update when making personal a bundle, we can do that very nicely now (only showing the recipient name as extra field for the personal bundle)
- personal messages did *not* usage the user_mail/copy message keys, they used the page_* ones, including an empty !category in subject. The tests didn't see this because we didn't assert the mail key nor the full subject, just the string put in by the user. Added/updated test assertions
- The user keys did not yet use the render controller.
- $params['recipient'] was missing in the mail params, this was used by user_mail, but as we had the wrong key, so we never got to that part of the code.

A lot of those fixes aren't really related to this conversion but you can't properly test it without most of those being fixed. Like actually adding a field and verifying that it's added to the mail. For which we have zero test coverage, as far as I can see.

Starting to use that tag for things we need to do to complete conversion, remove BC layer and fully convert field API to Entity Field API/Typed data.

Nice clean-up

Good improvements. However, once we add methods it should be complete, thus include setter methods. Else, patch looks good to me as well.

+++ b/core/modules/contact/lib/Drupal/contact/MessageInterface.php
@@ -15,6 +15,54 @@
+  public function copy();

That sounds like it makes a copy - so I don't think it's a good name. Maybe just getCopy() or getCopyFlag() ?

Yes, I wasn't sure about copy(), but getCopy() sounds like it returns a copy.

Not sure about settters, there are no cases where they are used in core, we just build the entity from the form data and send it. Not sure what kind of use cases could use methods?

Agreed with Berdir. Not having setters communicates how this entity is used.

Renamed to copySender().

I did come up with one use case, and that would be altering the message in a prepare_view or something like that. But I think that's not blocking this and is a api addition, so we can add that any time later on.

Not having those setter methods would indicate that we cannot change its entity fields - but that's not the case. While we support changing the fields, the entity is read-only storage wise though - so I opened the related issue #2019031: Allow entity types to be flagged as read-only. Please comment there.

However note, it's even already written to the fields, the fields are changed e.g. through the form or during creation. Yes, this works with the general setter, but *we do support setting* - so let's do not pretend we don't and add that methods.

Added the setter funcitons.
I'm not sure if/where they should be used now...

#24: contact-message-ng-1983548-24.patch queued for re-testing.

#24: contact-message-ng-1983548-24.patch queued for re-testing.

Added tests for message entity.

+++ b/core/modules/contact/lib/Drupal/contact/MessageInterface.phpundefined
@@ -63,6 +95,14 @@ public function getMessage();
+   * Set if the sender should become a copy of this e-mail or not.

should be sent or should receive I think, instead become, that sounds... german :)

+++ b/core/modules/contact/lib/Drupal/contact/Tests/MessageEntityTest.phpundefined
@@ -0,0 +1,68 @@
+ * Contains Drupal\contact\Tests\MessageEntityTest.

Missing leading \, Contains \Drupal\...

+++ b/core/modules/contact/lib/Drupal/contact/Tests/MessageEntityTest.phpundefined
@@ -0,0 +1,68 @@
+namespace Drupal\contact\Tests;
+
+
+use Drupal\contact\Plugin\Core\Entity\Message;

Should only be a single empty line.

+++ b/core/modules/contact/lib/Drupal/contact/Tests/MessageEntityTest.phpundefined
@@ -0,0 +1,68 @@
+  }
+}
\ No newline at end of file

Should have an empty line after the last function and before closing } of the class.

Missing newline at the end.

Implemented changes as requested

+++ b/core/modules/contact/lib/Drupal/contact/MessageInterface.php
@@ -26,11 +26,19 @@ public function getCategory();
+   * Set the name of the message sender.

Documentation summary should start with a 3rd person verb, i.e. "Sets the name.." instead of "Set the". This applies to all setters.

+++ b/core/modules/contact/lib/Drupal/contact/MessageInterface.php
@@ -26,11 +26,19 @@ public function getCategory();
+   * @param String $senderName

Type-hint should start lower-case.

+++ b/core/modules/contact/lib/Drupal/contact/Plugin/Core/Entity/Message.php
@@ -98,6 +126,13 @@ public function copySender() {
+    $this->set('copy', (bool)$inform);

Missing space after (bool)

Ups, should have catched that. Also changed camel case method arguments, we don't do that.

This will conflict quite a bit with #2010290: Editing a config entity from a listing page results in a 'page not found' and I'm fixing multiple from there already too for contact categories, wouldn't be sad to see this get in first :)

Patch looks good now, however the other one got committed.

#34: contact-message-ng-1983548-34.patch queued for re-testing.

Re-roll and merged in the test coverage for fields and also extended it a bit from the other issue. Re-add the manage field links that were incorrectly removed over there.

#38: contact-message-ng-1983548-38.patch queued for re-testing.

Failed to write configuration... We need to figure this one out, seeing a lot of them.

Great, this is ready then.

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.phpundefined
@@ -209,16 +210,56 @@ function testSiteWideContact() {
+    $i = 0;
+    foreach($this->xpath('//table/tbody/tr') as $row) {
+      if (((string)$row->td[0]) == $label) {
+        break;
+      }
+      $i++;
+    }
+
+    $this->clickLink(t('Manage fields'), $i);

Nice! /me puts that one in memory bank

Some very minor nits... would not have posted the first if I hadn't spotted the second...

+++ b/core/modules/contact/lib/Drupal/contact/CategoryListController.phpundefined
@@ -19,6 +19,22 @@ class CategoryListController extends ConfigEntityListController {
+    $uri = $entity->uri();
+    if (module_exists('field_ui')) {

$uri can be assigned inside the if ()...

+++ b/core/modules/contact/lib/Drupal/contact/Plugin/Core/Entity/Message.phpundefined
@@ -10,6 +10,7 @@
 use Drupal\Core\Entity\Entity;

This can be removed

Fixed that.

#44: contact-message-ng-1983548-44.patch queued for re-testing.

+++ b/core/modules/contact/lib/Drupal/contact/CategoryListController.phpundefined
@@ -19,6 +19,22 @@ class CategoryListController extends ConfigEntityListController {
+    if (module_exists('field_ui')) {

The CategoryListController has the moduleHandler injected as $this->moduleHandler so we can use this...

Committed 4756994 and pushed to 8.x. Thanks!

I think we might need to update the original contact to config entity change notice.

#1588422: Convert contact categories to configuration system never got a change notice. I'll create one for that and this issue combined... tomorrow.

Created https://drupal.org/node/2023711

Not sure about code examples, can't really think of anything useful...

The notice just enough

Removing sprint and change notice tag.