Subscriber updates should be confirmed atomically

> 1. On the SubscriptionItem, remove the "Status" property. Replace it with a
similar field on the Subscriber entity "User status".

I think that is problematic. Changes can still be per- newsletter, e.g. someone can subscribe to a newsletter, when he already is subscribed to another and then we want to track that?

Yes, having status on both is weird, but I'd rather have gone in the opposite direction and removed the global status, that always seemed weird.

So it seems clear that single opt-in is not really a recommended or common scenario.

I'm not sure that's true. There is the case of e.g. customers who have bought a product or signed up to a membership product of sorts. In these cases it's commonplace, generally legal, and sometimes the best UX for the person to be automatically signed up for relevant emails.

I haven't got my head around the bigger picture - I appreciate it's complex.

And following on logically from what I'm saying:

Create a new boolean site-wide setting "Require email verification of subscriptions".

I don't think that email verification is necessarily only a site wide preference; it's reasonable to want it for some newsletters but not others.

But possibly this shows a simple misunderstanding on my part:
if email verification only applies when a visitor initiates the subscription through a signup form, and doesn't apply when someone is added to a newsletter programatically by other logic, then I can see that it makes sense for email verification to be site-wide.

Fundamentally, it's not the subscription that's being verified but the subscriber... You could say that if a person signs up for one newsletter, verifies, and then signs up for another we should verify the second sign up again too. But I can see the case that this is unnecessarily strict: we've already verified that the person wishes to receive emails from this site, a change in exactly what emails they want is much less of a big deal and doesn't require reverification.

You could say that does open the door to malicious extra subscribing if a bad actor knows someone is subscribed to something from a site already; but even if you require verification you've still can't solve that totally because you could have malicious extra signups triggering an annoying stream of verification requests.

There are two ways for an existing user wants to alter their subscription...

Hmm. It seems like it would be very natural for a site to want it to be very easy for someone to sign up to an additional newsletter.

If I had an "Xmas countdown special offers" email I was pushing people to sign up for via a very simple form on a landing page, I'd really want it to just work regardless of whether they had signed up for some other unimportant newsletter of mine long in the past. I really wouldn't want them to have to jump through some authentication hoops.

It's possible that adding a subscription should be treated differently from modifying existing subscriptions.

This would seem to lead in the direction of per-newsletter verification, not per subscriber, as otherwise there's no way to verify the new additional sign-up.

Again, sorry for my lack of understanding of how things currently work ...

Dear all,

this issue has been dormant for almost a year now, and I really agree it's (1) a major blocker for many other issues for this module, and (2) has many important implications for UX and partly even for security. So I'd suggest we come to a conclusion, and crack on with filing some patches to move this module forwards.

I completely agree with @AdamPS's analysis and conclusions. The fundamental conceptual change in my opinion is that we essentially treat anonymous subscribers just like regular users but with the additional requirement to always confirm their authenticity by sending an email with an authentication link that they have to follow. By doing so we move away from authenticating individual subscriptions. I think this makes complete sense.

In summary, the proposed solution would be this:

1. The status field is removed from the subscriptions, and instead attached to the (anonymous) subscribers.
2. A site-wide setting is added to choose whether anonymous subscribers will be authenticated via email or not when they sign up to a newsletter. The default setting should be that subscribers will be authenticated. (This is likely going to be most commons use case, and makes sense from a legal and UX perspective, yet we retain the option to turn this authentication method off across the entire site in case site builders wish to do so.)
3. Furthermore, all changes to the newsletter subscriptions of a given anonymous subscriber will have to be confirmed by email authentication. We could add the option (either site-wide or per newsletter — although I'd vote for site-wide to keep it simple) to drop email authentication when altering or removing subscriptions for those subscribers who already have at least one active subscription. This would resolve the point by @jonathanshaw in #6 that the user is already authenticated and that you don't want them to jump through another set of hoops to change/update/remove subscriptions.
4. On top of all that, it may be necessary to add something like 'mandatory mailing lists' to address an issue brought up by @jonathanshaw in #4, which I can confirm I've come across when building a website with simplenews myself. Some information (e.g. regarding changes of terms & conditions, information related to purchases or products, etc.) needs to be distributed without giving the user the option to sign up or opt out. I've had users unsubscribe from mandatory legal emails in Simplenews 7.x, which wasn't particularly helpful. Whether this functionality is best accomplished in this module, or perhaps a separate module, or maybe a submodule is up for debate. My personal suggestion is that we keep this module focused on its intention: newsletters (which means the distribution of news information a user can sign up for — not mass emails that users have to receive).
5. I also really like Adam's idea regarding the 'right to be deleted', i.e. the option to block subscribers / email addresses from ever receiving emails from Simplenews again. Keeping a hashed version of the email address would be a suitable option, I think.

Hope this all makes sense. I think this proposed solution will increase simplicity in code, user interface, and management.

Let me know what you think, and then I hope we can start drafting some patches for this soon. I can imagine it'll be quite a bit of rewriting. Maybe Adam is best positioned to do that, but I'm happy to test/comment/review.

Here are a few more comments regarding implementation:

I believe that all code alterations should be targeted to make the newsletter-signup page and block the main point for making changes to the subscriptions. Essentially, all subscribers (users & anonymous) should be adding/removing/updating their newsletter subscriptions on that one single page (or block). This would also help with this: #3037307: Improve experience for anonymous subscribers to modify existing subscriptions

Here are the three variants the form on the page/block can take:

1. Authenticated user
   Access: only logged-in user ($account->id() != 0).
   Display: show email widget: off. Message should be "Update the subscriptions for user {{ user.getDisplayName() }} with email address {{ user.email }}" or something.
2. Authenticated anonymous subscriber (only page, not block)
   Access: can only be accessed via a valid link. Perhaps restrict to anonymous users ($account->id() == 0) and redirect users who are logged in or something.
   Display: show email widget: off. Message should be "Update the subscriptions for email address {{ anonymous_subscriber.email }}" or something.
3. Unauthenticated
   Access: only anonymous user ($account->id() == 0).
   Display: show email widget: on. Message should be "Subscribe to our newsletter!!!" or something.

The action taken when hitting the "Submit" button on this form (either the page or block) should be possible to determine using this diagram:

On that second diamond you could check whether the site-wide setting is for creating or updating subscriptions to address my point number 3. from my comment #9 above.

Okay great, that's helpful feedback! Thanks for reading all my thoughts and responding to them in the context of what had already been planned.

It really looks like there's a clear plan for how to much this forwards, and now it just needs somebody to dedicate their time and efforts to making this happen. I'll have a look, and I'll decide if I could spare the time and potentially be that person.

I see what you mean.

I don't think there's anything wrong with starting the work for the 3.x branch now. I don't know if it's a good strategy to postpone this issue for another year given it seems to be blocking progress on many other important issues. I think the target should definitely be to reach a stable version of the module rather than staying in beta and delaying progress unnecessarily.

But it's up to you, what ever you prefer!

I get your point about the fact that many site maintainers haven't even moved from 1.x to 2.x. Two points about this: 1) my personal experience as a site maintainer is that I also tend to postpone BC-breaking updates as much as I can, and when I do them I prefer to skip all the way to the latest version, rather than doing updates incrementally. So I'm thinking some people might actually be happy to move on to 3.x directly from 1.x? I'm thinking it might be worth offering a migration path for that, too, and that would make it more likely for people to move on. 2) perhaps there's also no good reason for people to migrate? I don't know what the difference between 1.x and 2.x was, but there's a chance people are not seeing the reason why they should go through the hassle of upgrading, when there's not too much of a gain in performance/UX/security/etc. Maybe some site maintainers are simple waiting for a stable release?

My feeling was that a lot of issues for this module are either dependent on or interrelated to this issue. For example these:

#3049356: Anon user can alter any anonymous subscription
#3037307: Improve experience for anonymous subscribers to modify existing subscriptions
#1140606: Permissions Per-Newsletter: Limit subscription options by role
#2937251: Prevent duplicate subscribers
#3037307: Improve experience for anonymous subscribers to modify existing subscriptions

I had the feeling that the new non-BC 3.x branch would be quite a big rewrite of the subscription/subscriber functionality of this module. Therefore, I thought it would make sense to incorporate as many of these interrelated issues in the rewrite as possible. All remaining issues that can be dealt with on the current 2.x branch are probably not related to the subscriber/subscription model of this module, and so would be relatively easy to merge/cherry-pick? Though, I don't have a good overview over all the existing open issues in this module, but this is a hunch.

That sounds good! Just to let you know I am playing around with the code at the moment, and trying out different things.

My plan would be this:

• Start by moving the state field from the subscription field to the subscriber entity. Then modify the newsletter settings form. All other changes should logically follow from this.
• At the moment, I'm trying to come up with a list of functions and files that need changing. I'll post them here in a few days perhaps, so anyone willing to help would be welcome to check if I've missed something.
• My aim would be to get the core module to work first. Then perhaps tests and submodules perhaps (Simplenews Rules). Then D7 migrations, and finally D8 1.x and 2.x upgrade paths.

Does that sound sensible?

Ooph, sounds like it's going to be a lot of work!

I have been wondering if this issue would benefit from a first stage involving centralizing more logic into a subscriptionStatus service or something like that. This can help with backwards compatibility management, future maintenance, and also provides a simple point of extension that reduces the need to handle every use case in the main module.

But this is only a reaction based on the smell of the discussion so far, I haven't looked at the actual code.

@jonathanshaw: That's a good point you're making, thanks for sharing your idea.

However, I think the problem is that a lot of that functionality is best placed and coded up in the subscriber entity class rather than in a service. That's because it relies on a lot of functionalities provided by the entity framework, and also because it depends heavily on what fields are stored and how they are handled.

So I think it's perhaps better/easier to handle most of the subscriber-related parts through the corresponding entity class and maybe a few other things such as the subscription field type.

Plus there already exists a SubscriptionManager. I don't think it contains every important bit of the subscription management model due to the above-mentioned reasons, but it does have some of the logic in it.

One quick question:

It looks like the status field on the subscriber entity is of type bool:

In Subscriber.php

    $fields['status'] = BaseFieldDefinition::create('boolean')
      ->setLabel(t('Status'))
      ->setDescription(t('Boolean indicating the status of the subscriber.'))
      ->setDefaultValue(TRUE);

Yet at the same time it looks like actually it should be int.

In simplenews.module:

/**
 * NEWSLETTER SUBSCRIPTION STATUS.
 */
define('SIMPLENEWS_SUBSCRIPTION_STATUS_SUBSCRIBED', 1);
define('SIMPLENEWS_SUBSCRIPTION_STATUS_UNCONFIRMED', 2);
define('SIMPLENEWS_SUBSCRIPTION_STATUS_UNSUBSCRIBED', 0);

and

/**
 * Returns the available options for the subscriber status.
 */
function simplenews_subscriber_status_list() {
  return [
    SIMPLENEWS_SUBSCRIPTION_STATUS_SUBSCRIBED => t('Subscribed'),
    SIMPLENEWS_SUBSCRIPTION_STATUS_UNSUBSCRIBED => t('Unsubscribed'),
    SIMPLENEWS_SUBSCRIPTION_STATUS_UNCONFIRMED => t('Unconfirmed'),
  ];
}

I guess that's a bug rather than a feature? I'd suggesting changing this to an int, keep the existing states (unconfirmed, confirmed, unsubscribed) and potentially other states could be added later on.

One more questions:

Any ideas what to do about the timestamp of a subscription? I'm guessing this needs to be moved to the Subscriber entity along with the state? The timestamp is mainly used to assess if the verification code has expired, or whether to remove unconfirmed subscription in the periodic tidy-ups if their timestamp is older than some maximum allowed time period. But then do we need this additional field? The Subscriber entity also has created and changed fields.

I remember there was a discussion in this bug

#3035367: Track history of subscribe/unsubscribe and proof of consent

about keeping track of the history of a subscriber/subscription.

Perhaps it would be easier to track via revision API?

Just trying to work out which fields need to go where and for what purpose.

I wonder if using https://www.drupal.org/project/email_confirmer as a dependency would help. It's quite feature rich, with an audit trail of confirmations, and reasonably well maintained.

A very relevant feature of email_confirmer is that confirmations are stored, and scoped to a 'realm' like 'simplenews'.

This means opens up options like keeping the email confirmation setting per newsletter, and calling EmailConfirmerManager->confirm($email, 'simplenews') if the setting is True when processing any newsletter subscription. email_confirmer will elegantly take care of reusing previous confirmations, and only sending a new confirmation request if some reasonable time has elapsed since the last once was sent. Maybe this opens up ways for logic in simplenews to be simplified.

I'm not at all sure what email_confirmer offers to the challenges that motivate this issue, but given its potential to reduce code complexity in simplenews (which is in some sense the ultimate cause of this issue) maybe its worth considering seriously.

We could add the option (either site-wide or per newsletter — although I'd vote for site-wide to keep it simple) to drop email authentication when altering or removing subscriptions for those subscribers who already have at least one active subscription. This would resolve the point by @jonathanshaw in #6 that the user is already authenticated and that you don't want them to jump through another set of hoops to change/update/remove subscriptions.

No I don't think so. We must verify even if there is already an active subscription. Otherwise a random hacker to add/remove subscriptions for any existing user. I think what @jonathanshaw is saying is that we shouldn't make adding a second subscription harder than adding the first one.

Actually Philipp was interpreting me right.

The risk of malicious subscription is non-trivial if a previously unknown email is subscribed anonymously, so it makes sense for some/many sites to require email confirmation despite the friction it gives to the user experience. You can imagine someone finding it rather fun to subscribe all their school teachers to an edgy porn site.

But the risk of malicious subscription of an already-subscribed email is vastly less for many sites, because it requires the malicous party to know an email is already subscribed to a given site. This is both an enormously less common scenario because the relevant nowledge is rarer, and much less common because there are fewer scenarios in which it might be interesting for a malicious party to mess with the subscriptions.

Therefore it's not unreasonable for a site to weight the risk-benefit of allowing unconfirmed additional subscriptions very differently from the risk-benefit of unconfirmed new subscriptions.

Adam, I think you're doing a fantastic job at taming and keeping track of an incredibly complex module!

Okay, just trying to catch up with all previous comments.

Re: #24 #25 #27
I also agree we should keep it simple, and don't add to many dependencies on other module, especially when they are not too well maintained/used. Also I still think the email validation method in the simplenews module is a bit different to the one in email_confirmer, so I don't know whether it'll be compatible.

Re: #26 #28
I agree that most sites will want to stick with double validation for all changes to the subscriptions (both new and existing). Yet, given that we've already decide that we'd preserve a way to switch off these validation in the config, we might as well have a separate setting for existing subscriptions, as I believe there may be people who would prefer to have it that way. I don't think it'll add too much code complexity. That said, I also don't think it's too important to focus on right now, it can be added later on.

Re: #30 #31 #32
I agree your way of splitting up the issue makes sense.

The solutions you are proposing look great (as you described in the IS of this issue and of the other one)! I was playing around with the code myself over the last few days, and I was trying different options with the revision API for the subscriber entity. I came to very similar conclusions as you did, so this gives me confidence that this is the way to go. What I like about all this is that it looks as if the whole module and code will be significantly simpler by the end.

I think we have a roadmap, which I believe you've outlined quite well in this issue (#3066314: Subscriber updates should be confirmed atomically), and this related one (#3035367: Track history of subscribe/unsubscribe and proof of consent). We first make the changes required to fix this issue (remove per-newsletter opt-in/-out setting in favour of a global one, and remove single confirmation mails in favour of always using combined). Then we move on to updating the subscriber entity and its fields, which will allow us to tackle the history tracking (#3035367: Track history of subscribe/unsubscribe and proof of consent), improve UX (#3037307: Improve experience for anonymous subscribers to modify existing subscriptions), and a whole load of other issues.

The biggest question for me now is who does what? Should I have a go at the changes needed for this issue? Or do you want to have a go at that, and I'll start looking at the changes required for #3035367: Track history of subscribe/unsubscribe and proof of consent?

Would it make sense to start a 3.x branch for these changes now? I guess what we're proposing here isn't going to be BC?

Okay perfect!! No worries, I can have a go at this. The required changes look fairly simple anyways!

Okay, this is a first attempt at creating a patch.

I've mostly worked on the code, and I've not done much extensive testing. I'll have to look into more testing in the next few days.

I'm sure some of the automatic tests will fail. I've fixed some of the obvious changes required in the tests, but this will require further attention later.

Also note that I haven't included an update function yet. Is that one required? It's only changes in the module config schema. Does Drupal recognise changes in the *.schema.yml automatically and make updates accordingly?

Feel free to comment or test but expect that it's still preliminary.

Generally, I feel I've hit the 'Delete' button quite a lot. It'd be worth double-checking things I've deleted can actually go. Similarly, there might be more things that is no longer needed and that can be removed.

Next attempt! Mostly changes to the tests.

Oops... looks like the previous patch I submitted was corrupted somehow. Next attempt.

Ah... I think I accidentally changed the underlying version, and that's why the patch doesn't apply?

Changing back to 3.x-dev and giving it another shot.

Oh stupid me. I wasn't patching against the 3.x HEAD. There was a commit by Adam three days ago that I missed. I did a rebase. Hopefully, this patch should at least apply now.

Next attempt to fix some of the broken tests.

Next attempt.

Next attempt. This time I actually made some changes to the module itself since my first patch, and not only the tests as in all the other updated patches.

I'm guessing there will still be some failing tests, but I hope this new patch fixes at least some of the ones that were failing earlier.

Okay, this one should resolve everything except for one failure, which I simply don't seem to be able to understand.

Oops. Next attempt.

It looks as if all tests are now fine except for one, which I simply don't understand.

Failures:

1) Drupal\Tests\simplenews\Functional\SimplenewsSubscribeTest::testSubscribeAnonymous
"Subscribe to <em class="placeholder">Default newsletter</em>" found
Failed asserting that false is true.

2) Drupal\Tests\simplenews\Functional\SimplenewsSubscribeTest::testSubscribeAnonymousSingle
"You have been subscribed." found
Failed asserting that false is true.

The first one occurs through this statement in function testSubscribeAnonymous:

    $this->assertRaw(t('Are you sure you want to confirm the following subscription changes for %user?', ['%user' => simplenews_mask_mail($mail),]));
    $this->assertText(t('Subscribe to %newsletter', ['%newsletter' => $newsletter->name,]));

I added that second assert statement because the confirmation is now always 'combined', which means it should list all the changes on that confirm page.

The second one occurs at the end of this code block in function testSubscribeAnonymousSingle:

    // 1. Subscribe anonymous via block
    // Subscribe + submit
    // Assert confirmation message
    // Verify subscription state.
    $mail = $this->randomEmail(8);
    $edit = [
      'mail[0][value]' => $mail,
    ];
    $this->drupalPostForm('', $edit, t('Subscribe'));
    $this->assertText(t('You have been subscribed.'));

I don't quite get that: both tests are asking to find a string, and apparently the string is in fact found — or am I misinterpreting the output? So I don't understand why it would complain.

Also for the second failure, I can confirm that the test is subscribing the anonymous user directly without confirmation when the global config variable use_verification is set to TRUE because the test a few lines below is not failing:

$this->assertEquals(SIMPLENEWS_SUBSCRIPTION_STATUS_SUBSCRIBED, $subscription->status);

Anyone got any ideas, give me a shout.

Otherwise, I'll now look into the upgrade path from 8.x-2.x to 3.x.

"You have been subscribed." found
The output can be misleading. Sometimes it describes the assertion that is failing, rather than the nature of the failure. I'm not sure there's a clear convention about from what perspective to word assertion messages.

"Subscribe to <em class="placeholder">Default newsletter</em>" found
I'd try this one as a more literal string, not wrapped in t(), in case that helps.

Sometimes you need to dump the page html out to a file and have a look at it, sadly I'm not sure Drupal provides a helper out of the box for doing that.

Next attempt. I think I have found the solution to issue 1) in #52 but not sure yet I understand what's going wrong with 2).

Some of these fails are not related to the changes I made I believe.

I'm having one final shot at issue 1) in this patch.

There are a few tests that fail randomly unfortunately. They normally work again if you retest, which I just did.

Ok cheers, thanks!

Here comes another test.

Next attempt.

Another bug fixed in the tests.

One more bug fix.

Sorry the previous test had a syntax error.

YESSS!!!

Looks like I finally managed to resolve all the bugs in the tests! I think I actually spent 10% of my time making changes to the module, and then 90% of my time trying to sort out the tests... Anyway, the tests seem to pass all fine now, and given the amount of manual testing I have done to fix the tests I'm fairly confident that this is all working smoothly.

Next thing, we'll have to look at the migration path.

Maybe better to get these tests working then let me review what's already done??

@Adam: So maybe now is a good time for you to review what I've done?

Currently, no migration path exists at all. I think Adam said he wants to have just one migration path from 8.x-2.x and remove all other (7.x / 8.x-1.x), is that right?

Okay great! Thanks very much for the review.

Regarding your points in #67.

1. An error popped up in those tokens, and the easiest fix was to add a break; statement at that stage.

Essentially, the problem was that the variable $newsletter is not set in a combined confirmation email, simply because there exists no single newsletter that this confirmation email is for. In fact however, I'm also wondering whether those tokens are still needed at all? Perhaps they can still be used in the actual newsletter email somewhere, but I doubt they are still required anywhere in the subscription confirmation emails, as this should only be handled in the [combined-url] or [manage-url] tokens from now on.

I think you can reproduce the error I encountered by adding the [subscribe-url] token somewhere in the combined confirmation message, and commenting/removing my break statement. It should result in the token trying to access the non-existent $newsletter variable, and consequently in a null access warning.

Perhaps is it worth removing the [subscribe-url] and [unsubscribe-url] tokens, or replace their content with the one of the [combined-url] token for the sake of BC?

((I am still convinced that, when we move on to issue #3035367: Track history of subscribe/unsubscribe and proof of consent, we can essentially remove all subscribe and unsubscribe paths, forms and tokens, and have only one single form that handles everything, along with one single token, namely the [manage-url] token. That's because when subscription changes are managed via the revision API, there won't be any need for those forms. But we can cross that bridge when we get there.))

2. OK!
3. OK!

4. I'm not sure I agree. I don't think this has anything to do with permissions. The statement you removed in the interdiff means — if I understand correctly — that you'll be now sending out confirmation emails for changes made to newsletters that are 'hidden', for which subscription should actually be handled silently.

Note that the entire else code block you're talking about does not depend on the user entity, but only on the newsletter entity. So this has nothing to do with permissions, only with the newsletter entity itself.

The requiresConfirmation function is called every time the SubscriptionManager is asked to either subscribe or unsubscribe a subscriber entity to a newsletter. If we proceed with your suggested changes, then doing so for a 'hidden' newsletter will result in a confirmation email, which I don't believe is intended.

5. These are your suggested changes:

+ Hidden: Subscription is mandatory or controlled programmatically using a
+ "Recipient Handler" (missing documentation - see code in
+ RecipientHandlerInterface and examples in the demo sub-module).

Is it perhaps possible that this is incorrect or misleadingly written? If I'm not mistaken, then the recipient handlers just add people to the mail spool for specific issues. However, they don't add/change/remove subscriptions to the newsletters — or am I wrong?

And finally, regarding the new fail that popped up: I'll try to take another close look at this as soon as I can. And I'll also make sure to update the coding standards warnings.

A few updates:

1. Just wanted to confirm that removing those break statements in file simplenews.tokens.inc will in fact lead to an error if the corresponding tokens are used in the combined confirmation email. I believe this bug was actually present prior to the changes suggested in this issue.

Here is the stack trace:

The website encountered an unexpected error. Please try again later.
Error: Call to a member function id() on null in simplenews_tokens() (line 103 of modules/custom/simplenews/simplenews.tokens.inc).

simplenews_tokens('simplenews-subscriber', Array, Array, Array, Object)
call_user_func_array('simplenews_tokens', Array) (Line: 403)
Drupal\Core\Extension\ModuleHandler->invokeAll('tokens', Array) (Line: 1038)
token_tokens('simplenews-subscriber', Array, Array, Array, Object)
call_user_func_array('token_tokens', Array) (Line: 403)
Drupal\Core\Extension\ModuleHandler->invokeAll('tokens', Array) (Line: 304)
Drupal\Core\Utility\Token->generate('simplenews-subscriber', Array, Array, Array, Object) (Line: 196)
Drupal\Core\Utility\Token->replace('We have received a request for the following subscription changes for [simplenews-subscriber:mail] [...] [simplenews-subscriber:subscribe-url]', Array, Array) (Line: 983)
simplenews_token_replace_body('We have received a request for the following subscription changes for [simplenews-subscriber:mail] [...] [simplenews-subscriber:subscribe-url]', Array) (Line: 123)
Drupal\simplenews\Mail\MailBuilder->buildCombinedMail(Array, Array) (Line: 562)
simplenews_mail('subscribe_combined', Array, Array) (Line: 271)
Drupal\Core\Mail\MailManager->doMail('simplenews', 'subscribe_combined', 'philipp.verpoort@mailbox.org', 'de', Array, 'no-reply@music-and-al.de', 1) (Line: 179)
[...]

Here is the code that results in this error:

  case 'subscribe-url':
    #if(!$newsletter) { break; }
    $hash = simplenews_generate_hash($subscriber->getMail(), 'add');
    $bubbleable_metadata->setCacheMaxAge(0);
    $route_parameters += [
      'action' => 'add',
      'newsletter_id' => $newsletter->id(), // this is line 103 where the error occurs
      'hash' => $hash,
    ];
    $replacements[$original] = Url::fromRoute('simplenews.newsletter_confirm_subscription', $route_parameters, $url_arguments)->toString();
    break;

I think adding the break statement will be fine for now. It means you can still use this token in the newsletter issue email, where an admin may want to refer to the link to unsubscribe from the particular newsletter type ("Don't want to receive emails like this any more? Click here to unsubscribe!"). At the same time, it won't cause any trouble in case somebody accidentally uses this token in the combined confirmation emails, where the variable $newletter is not set.

In the long run, as I mentioned in my previous comment, it is worth considering whether we actually want to keep all these tokens and the corresponding routes, or whether it might be easier to remove them altogether for the sake of reduced code complexity.

So long story short: I would keep those break statements in for now, and decide how to proceed with this elsewhere.

6. Regarding the failing test, I will reschedule a test for the patches in #63 and #67. I want to know if the failure has to do with any of the changes Adam introduced in the patch, or whether it might perhaps have to do with external dependencies. It looks to me like the failure arises due a formatting issue so perhaps it's a change in Swift Mailer or Core that raised this problem.

Okay, that's interesting.

I think the new test of patch #63 only failed due to a bizarre random glitch arising from #2575791: Mail subject should not be run through strip_tags(). I'll confirm this by running it again. So I believe this can be ignored.

Interestingly, however, the failure in patch #67 persistently occurred in the new test whereas it did not occur in #63. So believe it must have something to do with the changes Adam introduced going from #63 to #67. Based on the interdiff, most changes that Adam introduced are just changes of labels, descriptions, and README text, so shouldn't actually change anything about the tests. I am now rewinding the actual code changes to see if it resolves the failure.

Okay, I think I got it. Let's test it one more time in order to confirm I'm right.

Okay, here we go again.

I created a new patch file, and this should resolve all the problems from above.

1. I have removed the if(!$newsletter) { break; } statements. We can resolve it in a separate issue. EDIT: I've created this here. #3168355: Error when using [subscribe-url] token.
2. Unchanged.

3. Unchanged.

4. I have reverted file SubscriptionManager.php to the version I had:

return $newsletter->opt_inout == SIMPLENEWS_OPT_INOUT_DEFAULT && $this->config->get('subscription.use_verification');

Adam, please let me know if you have any objections. Please see my comment in #70.

5. I updated the README.txt file according to my comment in #70.

6. Adam, please could you explain why you made the following changes in test file SimplenewsSubscribeTest.php?

+    $this->assertStringContainsString("Rise & <shine>", $email['body']);

This introduced a new failure in the test. Could you explain why you would expect this assertion to be required/relevant/necessary?

7. I've run php code sniffer to deal with coding standards. This has resolved several issues (some corresponding to the changes I made but also some beyond that).

I think this should now be good to go. I'm testing this patch once more. I'm also including an interdiff to #67.

Next, we need to look at the migration path from 8.x-2.x.

Thanks very much for your work and your clarifications, this is great. I agree with you on all points, looks like we can hopefully complete our work on this very soon.

Here are my answers to all your points from above:

Re: Point 4 (from #74) / #81 regarding access vs confirmations of newsletters

Thanks a lot for your clarification. I must admit I hadn't properly understood this concept previously. You are right, my additional access confirmation in SubscriptionManager.php was incorrect, as it is technically possible that somebody would want to send out confirmations for newsletters that cannot be accessed through any of the forms.

And I totally agree with your point in #81. Let's change the name of the opt_inout field to access.

Re: Point 6 (from #74 / #75) regarding your previous changes to SimplenewsSubscribeTest.php

OK! So just confirming that this is fine as it is?

Re: #77

Yes, I agree with you, this an excellent suggestion. Let's change the use_verification field on the Newsletter entity to skip_verification (working in the opposite way obviously). This means in case somebody forgets to run the upgrade, it will default to FALSE.

Re: #78

Yup, well spotted! Concerning the newsletter name token, this is probably the same issue as #3168355: Error when using [subscribe-url] token.?

Re: #80

Good idea, thanks!

I'll try to create a new patch now!

Let's give this a shot.

Looks like this works. Adam, do you want to review (based on the interdiff)? I think it should all be fine now, I've incorporated the changes we discussed above.

• field opt_inout replaced by access
• config setting use_verification replaced by skip_verification
• I also made a few more changes to the README. Let me know what you think. Hope this makes things very clear.

Otherwise, I think it's really getting time to look at the upgrade path? I think we need to

• Create default for skip_verification config setting.
• Remove obsolete config settings for single-confirmation emails.
• Create default for access field on every newsletter type.
• Remove the obsolete opt_inout field on every newsletter type.

Anything else?

Okay, I finally managed to add the missing code for the upgrade path!

I'm uploading a revised patch, which contains the new simplenews_update_830003 function in file simplenews.install. On top of that I found a few minor bugs in the simplenews_demo submodule (I suspect there may be more but perhaps it's worth resolving those manually when all major updates to 3.x have been completed). I'm also providing an interdiff.

I think this is it. I gave it a shot on the dev environment of one of my D8 instances operating on simplenews 8.x-2.x. It seems to be working, and I can confirm the update function seems to be doing the right thing.

It all looks promising!

Oops, sorry. I somehow accidentally changed the title through some really weird autofill of my browser. Apologies.

Changing status to NR.

I agree on both points.

Re: 1) Yes sure. The update you provided seems sensible. There's indeed a good chance the migration code from 7.x is broken now as we haven't tested it thoroughly, but as you said, people can migrate to 2.x and then take it from there.

Re: 2) Sure, no problem, thanks for the revision.

Seems all good to me, looking forward to getting this submitted now.

And yes, I have reviewed your changes based on the interdiff, and it all seems good to me.

Thanks very much, Adam. I also very much appreciate your kind words, as well as your continued support and guidance. I could not have done it without your help!

Yes great, I'd be interested to continue working on this module, and help with addressing those remaining open issues. Me becoming a maintainer for this module sounds like an interesting idea but I guess let's first get to the stable release, and then we can discuss this again. :)

And again, somehow my browser likes inserting old values into this form...

Status is definitely fixed!!