Skip to content
Snippets Groups Projects

Sync subscription email when updating an entity containing a subscribed email

Open xenophyle requested to merge issue/mailchimp-3048474:3048474-sync-email into 2.x

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

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • 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.
  • 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:
    • Does this contradict the comment on line 672?

    • Author Developer

      Not if "this entity" == "that entity"... but maybe I am misunderstanding?

    • Author Developer

      Would it make more sense if the comment "// Email field and subscription field are both on this entity:" is inside the if, not before it? I can see that it may look like that comment is describing the entire if-else section, which is isn't.

    • 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?

    • xenophyle changed this line in version 3 of the diff

      changed this line in version 3 of the diff

    • Please register or sign in to reply
  • 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.

    • Author Developer

      Are you saying that the prefix can reliably be found by checking field_ui.settings.yml or that examining the string is a hopeless approach to begin with?

    • I'm saying examining a string is hopeless to begin with. You're probably going to have to ask Drupal to tell you what it is -- I have some instinct that properties and fields have a shared inheritance, so you should be able to ask "what is this thing?" somehow.

    • xenophyle changed this line in version 3 of the diff

      changed this line in version 3 of the diff

    • Please register or sign in to reply
  • 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.

    • Author Developer

      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 xenophyle
    • I see what you are saying. Maybe the comment could clarify it!

      But I think I have had an epiphany about this work that might provide a cleaner approach. I'm going to try to put something on your calendar.

    • xenophyle changed this line in version 3 of the diff

      changed this line in version 3 of the diff

    • Please register or sign in to reply
  • 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) {
  • 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) {
  • 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:

    1. You have this logic figured out, let's make it more readable for future devs!
    2. 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?
  • Author Developer

    Excellent feedback! Thank you for wading through the variable names and tortured logic.

  • xenophyle added 1 commit

    added 1 commit

    Compare with previous version

  • xenophyle added 5 commits

    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

    Compare with previous version

  • xenophyle added 7 commits

    added 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

    Compare with previous version

  • xenophyle added 34 commits

    added 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

    Compare with previous version

  • xenophyle added 1 commit
  • xenophyle added 1 commit

    added 1 commit

    • 48e95d88 - Move some merge var and subscribe form value updates to...

    Compare with previous version

  • xenophyle added 28 commits

    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

    Compare with previous version

  • 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.

    • Please register or sign in to reply
  • 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.

    • Please register or sign in to reply
  • 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

      This looks like a copy/paste of the code above. I think maybe a helper function makes sense here, update_all_entities(type, bundle, mailchimp_field) or something. And of course the same timeout concerns exist in this example, so that function could be the one using the Queue.

    • Please register or sign in to reply
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading