SubscriberForm overwrites existing subscription data

See description of priority field - this issue does not make the module completely unusable.

To me it's not clear that the "better and longer term fix" is much more complex that the short term fix. Might it make sense to put the better fix in straight away?

You are right there is data loss, but only in specific conditions something like this:

• there is at least one subscription that is hidden
• there is at least one subscription that is visible
• an admin edits the visible subscription of another user
• the other user had the hidden subscription enabled

The issue summary does not actually describe the exact symptoms of the bug. I can confirm there is a bug and have added IS text to describe it. The bug that I found seems to be solved by just part of the supplied patch, by changing simplenews_newsletter_get_visible to simplenews_newsletter_get_all.

@andrewbelcher Please can you confirm, or let me know if you think I have missed anything?

I have added some tests and I will upload a new patch.

The failure was the ONLYTESTS patch as expected, so this looks ready.

Wait a moment.....

I am surprised that the hidden newsletters are visible even on the admin page - that seems like a bug to me.

I have raised #3035521: Admin should not be able to modify subscriptions for hidden newsletters. We need to get that resolved before going any further here.

Both this bug and #3035521: Admin should not be able to modify subscriptions for hidden newsletters are caused by SubscriberForm missing important code in SubscriptionsFormBase. The solution is to update the base class of SubscriberForm to be SubscriptionsFormBase matching the other three subscription forms.

Then I had some confusion trying to understand and update the way these forms work. Here is the explanation of what I did:

• SubscriberForm delete a lot of code that is in the new base class.
• SubscriberForm delete code for isNew because it is wrong and not hittable until #3037036: Create button to add a single subscriber.
• Update SubscriptionsFormBase::actions to work with SubscriberForm. This is a rewrite as I found the old code very difficult to understand/update. New code calls the parent function which reduces duplication. New code gets ready for #3031919: Bugs if user has blank email address.
• SubscriptionsBlockForm::actions is not needed - the base class already has the same code.
• static::SUBMIT_UPDATE keep the default button text of 'Save' except override it in SubscriptionsPageForm.

Hmm tricker than I expected. I've added a detailed comment.

I am keen to avoid checking against empty mail because that breaks in the case of a user account with no email defined - we have another issue for that.

Apologies to the others following this issue. Hopefully this time.....

OK, let's code back closer to the original code, but make allowance for #3031919: Bugs if user has blank email address coming up, and keep the really clear comments.

So there is another bug relating to missing code from SubscriptionsFormBase. SubscriberForm must not allow edit of email address if the subscriber is a user.

OK, I plan to commit this in 1 week. Any comments/review before that would be appreciated.

1. +++ b/src/Form/SubscriptionsFormBase.php
   @@ -132,7 +139,9 @@ abstract class SubscriptionsFormBase extends ContentEntityForm {
        if ($mail = $this->entity->getMail()) {
   -      $form['mail']['#access'] = FALSE;
   +      if ($this->entity->getUserId()) {
   +        $form['mail']['#access'] = FALSE;
   +      }
   

   this overlaps with the access issue, but it is also not quite the same both there and here.

   Right now, the e-mail of an existing subscriber can't be changed, a new e-mail means a new subscription.

   I guess it does make sense to allow that, but I'd suggest to finalize the access issue first and then drop this chunk from the patch?

2. +++ b/src/Form/SubscriptionsFormBase.php
   @@ -149,34 +158,60 @@ abstract class SubscriptionsFormBase extends ContentEntityForm {
   +    if (!$has_widget) {
   +      $subscribed = $this->entity->isSubscribed($this->getOnlyNewsletterId());
   +    }
   

   $subscribed doesn't get initialized if $has_widget.. I guess it's also never accessed but that's actually not easy to figure out from reading the code. What if we just inline two calls we need into the conditions below? The overhead should be minimal. Thinking about how we could simplify the amount of (nested) ifs, but I can't think of anything obvious.

   Overall, really nice that we fix a bug, add test coverage and extensive comments and still end up with 2 lines of code less than before.

3. +++ b/src/Form/SubscriptionsFormBase.php
   @@ -149,34 +158,60 @@ abstract class SubscriptionsFormBase extends ContentEntityForm {
   +
   +    // @todo https://www.drupal.org/project/simplenews/issues/3031919
   +    // If an authenticated user has no email address then they can't subscribe.
   +    // Show a message instead of the form.
   +    // if ($this->entity->getUserId() && !$has_mail)
   

   I find it a bit strange to add commented out code with a todo.. doesn't seem like the right place as we are already deep in the form, building the actions. Might be better handled in the access control handler to never allow access to the form in the first place (we can't display a message then, though). My point is just leave this out completely.

1.

Right now, the e-mail of an existing subscriber can't be changed, a new e-mail means a new subscription.

I don't agree. Right now an admin can use SubscriberForm to change the email of an existing subscriber because right now SubscriberForm does not extend from SubscriptionsFormBase.

A) If the subscriber is also a user, this behaviour is highly undesirable because the User entity still stores the old email. This patch blocks that case.

B) If the subscriber is not a user, this behaviour is useful and is checked in the tests. The alternative of delete and create is tedious and loses important information about the subscription history. This chunk is needed in this patch to ensure that function continues to work.

Yes it overlaps with #3032167: Missing access check for subscriber mail field and both of them fix problem A in slightly different ways. The other issue has the correct long term fix, but is more risky, needs more tidy up and I'd rather leave it to give some time for people to spot problems in dev. This issue is safe and correct as it stands and is Critical due to data loss, so is blocking the next release.

2. Done

3. Done

Another test glitch. I really need to fix these soon!

@Berdir is it OK if I commit this one now please?