Problem/Motivation

hook_simplenews_subscribe is fired before the subscriber is saved, so before the subscription actually happens at a point when it may not happen for sure.

Current code

  public function subscribe($newsletter_id, $status = SIMPLENEWS_SUBSCRIPTION_STATUS_SUBSCRIBED, $source = 'unknown', $timestamp = REQUEST_TIME) {
    if ($subscription = $this->getSubscription($newsletter_id)) {
      $subscription->status = $status;
    }
    else {
      $data = [
        'target_id' => $newsletter_id,
        'status' => $status,
        'source' => $source,
        'timestamp' => $timestamp,
      ];
      $this->subscriptions->appendItem($data);
    }
    if ($status == SIMPLENEWS_SUBSCRIPTION_STATUS_SUBSCRIBED) {
      \Drupal::moduleHandler()->invokeAll('simplenews_subscribe', [$this, $newsletter_id]);
    }
  }

Proposed resolution

Fire the hook in Subscriber::postSave().

CommentFileSizeAuthor
#17 simplenews.hook-subscribe-unsubscribe.3260907-interdiff-15-17.txt309 bytesAdamPS
#17 simplenews.hook-subscribe-unsubscribe.3260907-17.patch10.67 KBAdamPS
#16 simplenews.hook-subscribe-unsubscribe.3260907-15.patch10.71 KBAdamPS
#15 simplenews.hook-subscribe-unsubscribe.3260907-interdiff-14-15.txt8.65 KBAdamPS
#14 simplenews.hook-subscribe-unsubscribe.3260907-interdiff-12-14.txt1.45 KBAdamPS
#14 simplenews.hook-subscribe-unsubscribe.3260907-14.patch1.78 KBAdamPS
#12 simplenews.hook-subscribe-unsubscribe.3260907-12.patch1.7 KBAdamPS
#10 simplenews-hook-subscribe-unsubscribe-3260907-4.x-10.patch1.61 KBsmokris
#10 simplenews-hook-subscribe-unsubscribe-3260907-3.x-10.patch1.63 KBsmokris
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jonathanshaw created an issue. See original summary.

AdamPS’s picture

AdamPS CreditAttribution: AdamPS at AlbanyWeb commented

Changing this could break existing sites however. It could probably be fixed quite naturally as part of #3035367: Track history of subscribe/unsubscribe and proof of consent but nobody especially seems interested in supporting that yet😃.

AdamPS’s picture

AdamPS CreditAttribution: AdamPS at AlbanyWeb commented

Probably in postSave() there is no longer the information to fire the same hook

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented 
Drupal::moduleHandler()->invokeAll('simplenews_subscribe', [$this, $newsletter_id]);

All we need is the newsletter id. We could get that by comparing current subscriptions with those of $subscriber->original.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented

The problem may be worse than I realised, because in SubscriptionsBlockForm we do this:

  public function form(array $form, FormStateInterface $form_state) {
   ...
    if (!$form_state->getUserInput()) {
      // Set defaults.
      foreach ($this->defaultNewsletterIds as $newsletter_id) {
        $this->entity->subscribe($newsletter_id, SIMPLENEWS_SUBSCRIPTION_STATUS_SUBSCRIBED, 'website');
      }
    }

Which looks like we fire this "subscribe" hook whenever the form is loaded? Maybe even when no email is entered. It makes me wonder if this is so broken that BC should not be the main concern.

AdamPS’s picture

AdamPS CreditAttribution: AdamPS at AlbanyWeb commented

A site could use the "broken" hooks and workaround the bugs. "Fixing" the brokenness could break their workarounds.

Presumably people could use functions from entity.api.php and check the original entity? So I'm not sure of the purpose of this hook in the long term. The second and third parameters are probably dubious.

As per #2, I don't see much advantage to change this without getting it right for the longer term in context of #3035367: Track history of subscribe/unsubscribe and proof of consent

AdamPS’s picture

AdamPS CreditAttribution: AdamPS at AlbanyWeb commented
Version: 3.x-dev » 4.x-dev

We have a new 4.x in alpha, so it's a good time to look at this again

AdamPS’s picture

AdamPS CreditAttribution: AdamPS at AlbanyWeb commented

We could fire the hook in postSave() with code roughly like this.

if ($this->isConfirmed() && isset($this->original) {
  // Compare subscriptions of new and old.
  $added = ...
  $removed = ...

  foreach ($added as $newsletter_id) {
    \Drupal::moduleHandler()->invokeAll('simplenews_subscribe', [$this, $newsletter_id]);
  }

  foreach ($removed as $newsletter_id) {
    \Drupal::moduleHandler()->invokeAll('simplenews_unsubscribe', [$this, $newsletter_id]);
  }

Does that sound good?

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented

Yeah, looks good.

smokris’s picture

smokris
Location Athens, Ohio, USA
CreditAttribution: smokris at Kosada for Kosada commented
Status: Active » Needs review
FileSize
simplenews-hook-subscribe-unsubscribe-3260907-3.x-10.patch1.63 KB
simplenews-hook-subscribe-unsubscribe-3260907-4.x-10.patch1.61 KB

I've attached patches (based on AdamPS's snippet above) for simplenews 3.x and 4.x.

AdamPS’s picture

AdamPS CreditAttribution: AdamPS at AlbanyWeb commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks @smokris

1) Actually my comment #8 wasn't quite right. I think we need like this:

if (isset($this->original) && $this->original->isConfirmed()) {
  $original = $this->original->getSubscribedNewsletterIds();
}
else {
  $original = [];
}

2) Let's create a variable for the current ones too so we don't calculate them twice:

$current = $this->getSubscribedNewsletterIds();

3) Plus a variable for the result of \Drupal::moduleHandler() so we don't calculate it many times.

4) It would be great to have a test for the hooks. But if you don't want to, it's still useful to fix the other comments😃.

AdamPS’s picture

AdamPS CreditAttribution: AdamPS at AlbanyWeb commented
Assigned: Unassigned » AdamPS
FileSize
simplenews.hook-subscribe-unsubscribe.3260907-12.patch1.7 KB

Reroll of #10

AdamPS’s picture

AdamPS CreditAttribution: AdamPS at AlbanyWeb commented
Status: Needs work » Needs review
AdamPS’s picture

AdamPS CreditAttribution: AdamPS at AlbanyWeb for think modular - digital solutions GmbH commented
FileSize
simplenews.hook-subscribe-unsubscribe.3260907-14.patch1.78 KB
simplenews.hook-subscribe-unsubscribe.3260907-interdiff-12-14.txt1.45 KB
AdamPS’s picture

AdamPS CreditAttribution: AdamPS at AlbanyWeb for think modular - digital solutions GmbH commented
FileSize
simplenews.hook-subscribe-unsubscribe.3260907-interdiff-14-15.txt8.65 KB
AdamPS’s picture

AdamPS CreditAttribution: AdamPS at AlbanyWeb for think modular - digital solutions GmbH commented
Issue tags: -Needs tests
FileSize
simplenews.hook-subscribe-unsubscribe.3260907-15.patch10.71 KB
AdamPS’s picture

AdamPS CreditAttribution: AdamPS at AlbanyWeb for think modular - digital solutions GmbH commented
FileSize
simplenews.hook-subscribe-unsubscribe.3260907-17.patch10.67 KB
simplenews.hook-subscribe-unsubscribe.3260907-interdiff-15-17.txt309 bytes

  • AdamPS committed 72b09ed2 on 4.x 
    Issue #3260907 by AdamPS, smokris: hook_simplenews_subscribe fires...
AdamPS’s picture

AdamPS CreditAttribution: AdamPS at AlbanyWeb for think modular - digital solutions GmbH commented
Status: Needs review » Fixed
AdamPS’s picture

AdamPS CreditAttribution: AdamPS at AlbanyWeb for think modular - digital solutions GmbH commented
Assigned: AdamPS » Unassigned

Status: Fixed » Closed (fixed)

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