Problem/Motivation

The annotations have quite a bit of cruft. Lets update them and benefit from newer features

Proposed resolution

Looking at Contact, check others too for similar problems:

* fieldable is gone, remove
* not sure if label_callback() still works, we should probably just override the label() method
* vid is a bad revision ID field name, copied from node. possibly rename? (to revision_id)
* user entity key doesn't exist
* link template should be edit-form not edit_form
* call the parent \Drupal\Core\Entity\ContentEntityBase::baseFieldDefinitions() to get the default base field definitions (requires 8.1, but that's OK)
* getChangedTime() is wrong, should return a value, remove it in favor of the trait that is already used.
* \Drupal\crm_core_contact\Entity\ContactType::preDelete() still calls watchdog()

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#9 clean_up_entity_type-2694357-9-interdiff.txt800 bytesmbovan
#9 clean_up_entity_type-2694357-9.patch16.04 KBmbovan
#6 clean_up_entity_type-2694357-6-interdiff.txt721 bytesmbovan
#6 clean_up_entity_type-2694357-6.patch15.26 KBmbovan
#4 clean_up_entity_type-2694357-4.patch14.56 KBmbovan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

One more thing, extend ContactTypeDeleteForm from the entity delete confirm form, remove everything that we no longer need. possibly everything...

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Assigned: Unassigned » mbovan
mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Status: Active » Needs review
FileSize
clean_up_entity_type-2694357-4.patch14.56 KB

Addressed the issue summary points and did some additional cleaning.

  1. +++ b/modules/crm_core_contact/src/Entity/Contact.php
@@ -65,30 +62,7 @@ class Contact extends ContentEntityBase implements ContactInterface {
+    $fields = parent::baseFieldDefinitions($entity_type);

    Do we need to update the labels (Contact ID, Contact Type...)? 

  2. +++ b/modules/crm_core_contact/src/Entity/Contact.php
@@ -273,37 +246,16 @@ class Contact extends ContentEntityBase implements ContactInterface {
+    \Drupal::moduleHandler()->alter('crm_core_contact_label', $label, $entity);

    Is this useful anymore?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

I think the hook is still useful. The standard labels (ID instead of Contact ID) are fine with me.

+++ /dev/null
@@ -1,96 +0,0 @@
-   */
-  public function buildForm(array $form, FormStateInterface $form_state) {
-    $num_nodes = $this->database->query("SELECT COUNT(*) FROM {crm_core_contact} WHERE type = :type", array(':type' => $this->entity->id()))->fetchField();
-    if ($num_nodes) {
-      $caption = \Drupal::translation()->formatPlural(
-          $num_nodes,
-          '%type is used by one contact on your site. You can not remove this contact type until you have removed all of the %type contacts.',
-          '%type is used by @count contacts on your site. You may not remove %type until you have removed all of the %type contacts.',
-          array('%type' => $this->entity->label()));
-      $form['#title'] = $this->getQuestion();

This is handled automatically now? Would be nice to have test coverage, if only to see how it works now.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
FileSize
clean_up_entity_type-2694357-6.patch15.26 KB
clean_up_entity_type-2694357-6-interdiff.txt721 bytes

I think we already have tests for this in ContactUiTest::testContactTypeOperations(), added some more.

thenchev’s picture

thenchev CreditAttribution: thenchev at MD Systems GmbH commented
Status: Needs review » Needs work 
+++ b/modules/crm_core_contact/legacy/crm_core_contact.views.inc
@@ -146,12 +146,12 @@ function crm_core_contact_views_data() {
-    'field' => 'vid',
+    'field' => 'revision_id',
     'title' => t('CRM Core Contact revision'),

Are we doing changes in legacy folder? Just wondering.

Also i see 'vid' in crm_core_contact_ui_revert_form.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

For code that we eventually want to port like this I think it makes sense, yes.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
clean_up_entity_type-2694357-9.patch16.04 KB
clean_up_entity_type-2694357-9-interdiff.txt800 bytes

Updated the variable names in crm_core_contact_ui_revert_form() as well.

thenchev’s picture

thenchev CreditAttribution: thenchev at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Didn't notice anything else.

  • slashrsm committed 1f19ac6 on 8.x-1.x authored by mbovan 
    Issue #2694357 by mbovan, Berdir, Denchev: Clean up entity type...
slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks.

  • slashrsm committed 1f19ac6 on 8.x-2.x authored by mbovan 
    Issue #2694357 by mbovan, Berdir, Denchev: Clean up entity type...
mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.