Various fixes to contact types, interaction with contacts, and contact type forms:

The main issue was primary fields were not being saved to config, and field config was not being fetched from Drupal API correctly.

Also available from GH dpi/crm_core/contact-type-primary-fields

Notes:

  • Changed schema since its current usage assumed sequence was a KV, instead the keys were being destroyed on save.
  • Fixed missing contact type interface extend.
  • Added a getPrimaryField method to ContactType entity.
  • Converted getPrimaryField on Contact entity to use getPrimaryField method instead of raw class properties.
  • Changed primary field container to 'details', since thats what a lot of Drupal is using. Also the description mentions 'the fields below' except the description was being rendered below the fields. Changing to details fixes this.
  • Fixed call to getFieldDefinitions. Looks like the Drupal API changed since this was written as its usage was completely nonsensical.
  • Changed empty_option label to remove 'Please'. We don't add emotion to text in Drupal.
  • Primary fields values are now saved to the config.
CommentFileSizeAuthor
#6 2711935-6-contact-type-primary-fields.patch7.56 KBgrahl
contact-type-primary-fields.patch6.21 KBdpi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dpi created an issue. See original summary.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi as a volunteer commented

Patch is required for https://github.com/dpi/courier_crm_core_contact

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented 
  1. +++ b/modules/crm_core_contact/config/schema/crm_core_contact.schema.yml
@@ -17,8 +17,12 @@ crm_core_contact.type.*:
     primary_fields:
-      type: sequence
+      type: mapping
       label: 'The fields the contact uses primarily'
-      sequence:
-        type: string
-        label: 'Field'
+      mapping:
+        email:
+          type: string
+        address:
+          type: string
+        phone:
+          type: string

    Sure about this? These are the only possible primary fields?

    The documentation doesn't seem to indicate this, a sequence for any list of field seems more likely?

    Also, given that we plan to add multi-value fields with a type for email and phone, I'm not sure if this API will still be required then? 

  2. +++ b/modules/crm_core_contact/src/Entity/Contact.php
@@ -248,9 +248,9 @@ class Contact extends ContentEntityBase implements ContactInterface {
-    $type = $this->get('type')->entity;
-    $name = empty($type->primary_fields[$field]) ? '' : $type->primary_fields[$field];
-    return $this->get($name);
+    /** @var \Drupal\crm_core_contact\ContactTypeInterface $contact_type */
+    $contact_type = $this->get('type')->entity;
+    return $this->get($contact_type->getPrimaryField($field));
   }

    If null is a valid return value, shouldn't we check that? this will throw a veird exception otherwise. 

  3. +++ b/modules/crm_core_contact/src/Form/ContactTypeForm.php
@@ -22,6 +22,13 @@ class ContactTypeForm extends EntityForm {
 
   /**
    * {@inheritdoc}
+   *
+   * @var \Drupal\crm_core_contact\ContactTypeInterface
+   */
+  protected $entity;

    inheritdoc can only be used if it's the only thing, if you extend it, you have to copy the parent implementation. 

  4. +++ b/modules/crm_core_contact/src/Form/ContactTypeForm.php
@@ -69,10 +78,10 @@ class ContactTypeForm extends EntityForm {
 //    $primary_fields = variable_get('crm_core_contact_default_primary_fields', $default_primary_fields);
     $primary_fields = $default_primary_fields;

    Looks like there are supposed to be more of those primary fields, it's just commented out at the moment.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi as a volunteer commented

I discussed the future of contacts and contact types with miro_dietiker over IRC this morning and I understand this patch is likely obsolete since the model is changing.

These are the only possible primary fields?

email, address, primary they are the primary fields listed in the UI, and there's an array on the form which has those fields. Again I understand the existing implementation was probably placeholder. If the model allowed more primary fields then the schema will be very different.

I dont think what I have written will stick, but it gets the existing code running.

grahl’s picture

grahl
he/him
Location Zürich
CreditAttribution: grahl as a volunteer commented
Version: 8.x-1.x-dev » 8.x-3.x-dev
Status: Needs review » Needs work

Probably irrelevant, need to check.

grahl’s picture

grahl
he/him
Location Zürich
CreditAttribution: grahl as a volunteer commented
Status: Needs work » Needs review
Issue tags: +ContributionWeekendCH, +ContributionWeekend2020
FileSize
2711935-6-contact-type-primary-fields.patch7.56 KB

- Primary fields schema potentially still relevant but leaving out to leave the option of adding additional fields in custom code.
- ConfigEntityBundleBase already provides ConfigEntityInterface, not needed
- Primary Fields functions already present, however the function does two things: get the field name and field value, let's take that apart.

And let's add those functions to the interface.