Sync subscription email when updating an entity containing a subscribed email
If an entity is updated, and that entity contains the field used for a subscription email address, and the email has changed, update the email address on the Mailchimp end.
This should work whether the email field is
- on the same entity that has the subscription field
- on the author/revision author of the entity with the subscription field
- on an entity referenced by the entity with the subscription field
Closes #3048474
Merge request reports
Activity
- Resolved by xenophyle
655 658 656 659 return $enabled_events; 657 660 } 661 662 /** 663 * Implemenents hook_entity_update(). 664 */ 665 function mailchimp_lists_entity_update($entity) { 666 // When updating an entity, check if we are updating the email address on a 667 // associated with a subscription, and if so, update it in the subscription. changed this line in version 3 of the diff
- Resolved by Gabriel Carleton-Barnes
668 $entity_id = $entity->id(); 669 $entity_type = $entity->getEntityTypeId(); 670 $entity_bundle = $entity->bundle(); 671 672 // Look for any entity whose subscription field gets the email value from this entity. 673 $field_map = \Drupal::service('entity_field.manager')->getFieldMapByFieldType('mailchimp_lists_subscription'); 674 foreach ($field_map as $entity_type_id => $fields) { 675 foreach ($fields as $field_name => $field_info) { 676 foreach ($field_info['bundles'] as $bundle) { 677 $field_storage_config = FieldStorageConfig::loadByName($entity_type_id, $field_name); 678 $mc_list_id = $field_storage_config->getSetting('mc_list_id'); 679 $field_instance_config = FieldConfig::loadByName($entity_type_id, $bundle, $field_name); 680 $email_field = $field_instance_config->getSetting('merge_fields')['EMAIL']; 681 $email_field_components = explode(':', $email_field); 682 683 // Email field and subscription field are both on this entity: Ah, no, I was just reading the previous comment (line 672) as implying 2 different entities. The "they are the same" case is technically a valid interpretation of that comment, but it's a weird interpretation! I guess maybe the comment on line 672 could help clarify this. What if it said, "// Look for a subscription field configuration that uses the email value from this entity." -- is that accurate?
changed this line in version 3 of the diff
679 $field_instance_config = FieldConfig::loadByName($entity_type_id, $bundle, $field_name); 680 $email_field = $field_instance_config->getSetting('merge_fields')['EMAIL']; 681 $email_field_components = explode(':', $email_field); 682 683 // Email field and subscription field are both on this entity: 684 if (strpos($email_field, ':entity:') === FALSE) { 685 mailchimp_lists_update_email($entity, $mc_list_id, $email_field_components); 686 } 687 else { 688 // Entities that are not referenced by a field: 689 if (strpos($email_field, 'uid:') === 0 || strpos($email_field, 'revision_uid:') === 0) { 690 if ($entity_type == 'user') { 691 mailchimp_lists_update_email($entity, $mc_list_id, $email_field_components); 692 } 693 } 694 elseif (strpos($email_field, 'field_') === 0) { it turns out this isn't reliable. Fields can have machine names that don't start with "field_". The reason most of them have this is because they are created via Field API, which actively prepends this. But you can change it! Check out config/field_ui.settings.yml on any of our sites. That's where it comes from.
changed this line in version 3 of the diff
674 foreach ($field_map as $entity_type_id => $fields) { 675 foreach ($fields as $field_name => $field_info) { 676 foreach ($field_info['bundles'] as $bundle) { 677 $field_storage_config = FieldStorageConfig::loadByName($entity_type_id, $field_name); 678 $mc_list_id = $field_storage_config->getSetting('mc_list_id'); 679 $field_instance_config = FieldConfig::loadByName($entity_type_id, $bundle, $field_name); 680 $email_field = $field_instance_config->getSetting('merge_fields')['EMAIL']; 681 $email_field_components = explode(':', $email_field); 682 683 // Email field and subscription field are both on this entity: 684 if (strpos($email_field, ':entity:') === FALSE) { 685 mailchimp_lists_update_email($entity, $mc_list_id, $email_field_components); 686 } 687 else { 688 // Entities that are not referenced by a field: 689 if (strpos($email_field, 'uid:') === 0 || strpos($email_field, 'revision_uid:') === 0) { - Comment on lines +687 to +689
The else/if nesting here implies a grouping that seems wrong to me. Don't we have 3 parallel cases, rather than 1 top-level case and two sub-cases? If I'm reading correctly, the "else { if {}} on line 687 can just be an "elseif {}", and then our comments can describe each of the three cases in parallel.
I was thinking of it as 1) email is on the subscription entity or 2) email is on a connected entity.
But it could just as easily be 1) email is on the subscription entity, 2) email is on an entity associated by a property (always a user as far as I can tell) 3) email is on an custom-ly associated entity...
I am trying to tell if you really prefer this grouping or if my comments could just be improved.
Edited by xenophylechanged this line in version 3 of the diff
683 // Email field and subscription field are both on this entity: 684 if (strpos($email_field, ':entity:') === FALSE) { 685 mailchimp_lists_update_email($entity, $mc_list_id, $email_field_components); 686 } 687 else { 688 // Entities that are not referenced by a field: 689 if (strpos($email_field, 'uid:') === 0 || strpos($email_field, 'revision_uid:') === 0) { 690 if ($entity_type == 'user') { 691 mailchimp_lists_update_email($entity, $mc_list_id, $email_field_components); 692 } 693 } 694 elseif (strpos($email_field, 'field_') === 0) { 695 // Verify referenced entity type matches $entity's type. 696 $config = FieldStorageConfig::loadByName($entity_type_id, $email_field_components[0]); 697 $target_type = $config->getSetting('target_type'); 698 if ($target_type == $entity_type) { changed this line in version 3 of the diff
720 721 /** 722 * Updates the email address on a subscription. 723 * 724 * When the email address used by a MailListsSubscription field is updated 725 * locally, this updates it in the remote subscription. 726 * 727 * @param \Drupal\Core\Entity\EntityInterface $entity 728 * The entity with the email field. 729 * @param string $mc_list_id 730 * The audience (list) ID the email is being updated on. 731 * @param array $email_field_components 732 * The field the MailListsSubscription field is using for the email address, 733 * split into its components. 734 */ 735 function mailchimp_lists_update_email(EntityInterface $entity, $mc_list_id, array $email_field_components) { I think this shouldn't exist, and we should just use
mailchimp_update_member_process()
. That has more arguments, but I think you can adjust it's logic just a little so the$format
argument responds elegently to a NULL value like the$interests
variable does.Doing so would also allow our behavior here to respond to more changes than just the email address change.
changed this line in version 3 of the diff
Sorry for the wall of feedback here! I think we are on the right path, and the logic to sort through the conditions here is really complicated so nice work sorting that out. My feedback is 2 major themes:
- You have this logic figured out, let's make it more readable for future devs!
- Why are we limiting ourself to updates to the email? Isn't it just as valid to say that someone might update a different merge variable in this way, like their name, and that we want that to go over to Mailchimp?
added 5 commits
- 495dfdf6 - Clearer variable names
- 50f5621c - Merge vars will now be updated via mailchimp_lists_entity_update()
- 79860206 - Add table to store where merge values are stored for each entity with a subscription field
- 30b67553 - Update merge values when their source fields have changed
- 94a116af - Merge branch '3048474-sync-email' of git.drupal.org:issue/mailchimp-3048474 into 3048474-sync-email
Toggle commit listadded 7 commits
- 54051c32 - Handle merge data when an entity is added or deleted
- 6b04747e - Text cleanup
- eab2235f - Move mailchimp_lists_entity_update() close to its friends
- 01019034 - Do not update mergevar source tracking table when just checking previous merge values
- 9358132a - Use variable for field name!
- a730a43c - Handle when the updated entity has a subscription field
- 79a3fc2e - A better way to find subscription fields
Toggle commit listadded 34 commits
-
79a3fc2e...7d8dc3d8 - 20 commits from branch
project:2.x
- 7d8dc3d8...5232410c - 4 earlier commits
- 22277154 - Update merge values when their source fields have changed
- 7ae977e1 - Fix spelling
- a7842d36 - Handle merge data when an entity is added or deleted
- d01575d5 - Text cleanup
- 92a2fab6 - Move mailchimp_lists_entity_update() close to its friends
- 5b6ac642 - Do not update mergevar source tracking table when just checking previous merge values
- b336aa14 - Use variable for field name!
- e5586b07 - Handle when the updated entity has a subscription field
- 4a0527a8 - A better way to find subscription fields
- 51ee0e34 - Merge branch '3048474-sync-email' of git.drupal.org:issue/mailchimp-3048474 into 3048474-sync-email
Toggle commit list-
79a3fc2e...7d8dc3d8 - 20 commits from branch
added 1 commit
- 48e95d88 - Move some merge var and subscribe form value updates to...
added 28 commits
-
48e95d88...fb555d88 - 6 commits from branch
project:2.x
- fb555d88...e04310ab - 12 earlier commits
- 462d801c - A better way to find subscription fields
- d9df5377 - Sync subscription email when updating an entity containing an email subscribed...
- a6203995 - Clearer variable names
- b50be924 - Update merge values when their source fields have changed
- ba10d5ec - Move mailchimp_lists_entity_update() close to its friends
- 59cb8177 - Use variable for field name!
- de34be05 - Handle when the updated entity has a subscription field
- 8e1a2d62 - PHPCS
- d1387da6 - Move some merge var and subscribe form value updates to...
- 22d4b691 - Merge branch '3048474-sync-email' of git.drupal.org:issue/mailchimp-3048474 into 3048474-sync-email
Toggle commit list-
48e95d88...fb555d88 - 6 commits from branch
25 */ 26 function mailchimp_lists_entity_insert($entity) { 27 if ($entity->getEntityTypeId() === 'field_config' && $entity->getType() === 'mailchimp_lists_subscription') { 28 // Subscription field is added to entity type. 29 // New entity is a mailchimp_lists_subscription field config. 30 $merge_fields = $entity->getSetting('merge_fields'); 31 if ($merge_fields) { 32 $host_entity_type = $entity->getTargetEntityTypeId(); 33 $host_bundle = $entity->getTargetBundle(); 34 $host_entities = \Drupal::entityTypeManager()->getStorage($host_entity_type)->loadByProperties(['type' => $host_bundle]); 35 $field_name = $entity->getName(); 36 foreach ($host_entities as $host_entity) { 37 // Set merge values on entities of this type. 38 if ($host_entity->$field_name) { 39 $instance = $host_entity->$field_name[0]; 40 mailchimp_lists_update_merge_vars($instance, $host_entity); - Comment on lines +36 to +40
If I'm understanding this correctly, this means when we create a new Mailchimp field we are updating all the entities that have that field? What if I add a new Mailchimp field to Users on an existing site with 2000 users? Seems like this will time out and crash. It also seems like the "add field" operation would go incredibly slowly. I think we have to Queue this -- or at the very least use the batch system for this loop.
48 $field_map = \Drupal::service('entity_field.manager')->getFieldMapByFieldType('mailchimp_lists_subscription'); 49 $fields = $field_map[$entity->getEntityTypeId()]; 50 foreach ($fields as $field_name => $field_info) { 51 if (in_array($entity->bundle(), $field_info['bundles'])) { 52 if ($entity->$field_name) { 53 $instances[] = $entity->$field_name[0]; 54 } 55 } 56 } 57 foreach ($instances as $instance) { 58 mailchimp_lists_update_merge_vars($instance, $entity); 59 } 60 61 // @todo Do we need to handle when a helper entity is created or will that 62 // always be caught when its host is updated? 63 } - Comment on lines +61 to +63
I don't think you'll need this. The helper entity can't become a helper entity without being referenced from the main entity. It can't be referenced without having an ID, so there's no way for it to be referenced before being inserted. I think you can remove this, or note the reason there's no action in these cases in a comment, rather than having a Todo.
73 $merge_fields = $entity->getSetting('merge_fields'); 74 $old_merge_fields = $entity->original->getSetting('merge_fields'); 75 if ($merge_fields != $old_merge_fields) { 76 $host_entity_type = $entity->getTargetEntityTypeId(); 77 $host_bundle = $entity->getTargetBundle(); 78 $host_entities = \Drupal::entityTypeManager()->getStorage($host_entity_type)->loadByProperties(['type' => $host_bundle]); 79 $field_name = $entity->getName(); 80 foreach ($host_entities as $host_entity) { 81 // Merge field settings were updated; update merge values on entities of 82 // this type. 83 if ($host_entity->$field_name) { 84 $instance = $host_entity->$field_name[0]; 85 mailchimp_lists_update_merge_vars($instance, $host_entity); 86 } 87 } 88 } - Comment on lines +79 to +88