The ConfirmRemovalForm redirects users to the add subscription confirmation page rather than the removal confirmation page. It used to be OK in some previous versions, it must be a copypaste kind of regression.

Comments

djg_tram created an issue. See original summary.

djg_tram’s picture

Issue summary: View changes
adamps’s picture

Version: 8.x-1.x-dev » 8.x-2.x-dev
Priority: Critical » Normal
Issue summary: View changes

Thanks for the report - yup looks like a bug.

However please bear in mind that this is open-source software without any kind of warrantee maintained by people in their spare time. You are using a beta release. We are willing to help but it works better to ask nicely - no one is required to take action just because you say so.

Please can you clarify the symptoms? From looking at the code it seems likely that the user is actually unsubscribed, just they get sent to the wrong page so that reduces the severity. Furthermore it looks like the error doesn't occur in the default config, only if a page has been explicitly configured. You could easily workaround by removing that manual setting. So according to the definition of the priority field "normal" seems like the correct setting.

Patches welcome. As per Drupal general policy, the patch needs to be for the latest release 2.x and then we can potentially backport.

djg_tram’s picture

Yes, I know. I'm a project maintainer around here, too, although not very active these days. I did fix it immediately for my sites but I still wanted to make it stand out here for anybody who happens to search for the same issue after me (considering that Google does a great job of picking up issues on d.o almost immediately).

I certainly appreciate all the work you do with the module, don't get it wrong for a minute. But, as it turns out, with all the legal regulations these days, you happen to be active in an area that is heavily regulated and site owners might be subjected to strenuous investigations and are threatened with hefty fines if found negligent in any way. A user unsubscribing and getting a nice confirmation about subscribing can raise a lot of problems to site owners these days, so I don't think my warnings were unwarranted. And I certainly wouldn't mind it if it happened on any project I maintained, it helps pinpoint issues that are more severe. Mentioning the need for an urgent fix was nothing else but a friendly reminder that, at least in my opinion, it shouldn't be treated as one of many fixes rolled into a distant future release but, if possible, tagging a new release sooner for all people out there to be refreshed. (All the more that it seems to be a regression, so anybody who tested it earlier on their sites might not be aware of the issue popping up later at all.)

Yes, I know patches are welcome. But I don't have git set up right now on this machine and, as you saw it, it's all about adding two characters to a single hardcoded string. I'm sure it can be changed by any contributor without me sending in a PR now, I hope... :-)

gg24’s picture

Hi Guys,

I tried to figure out the issue here, but not able to reproduce it. @djg_tram any lead you want to give which can help me figure it out quicker?
Else I would change the status to (Closed it as not reproducible). You can open it back if required later.

cc: @AdamPS.
Thanks!

djg_tram’s picture

Yep, sure (sorry, I haven't logged in for a few weeks). It's as simple as the ConfirmRemovalForm loading the wrong redirect address. Just look at the line in submitForm:

if ($path = $config->get('subscription.confirm_subscribe_page')) {

it should refer to subscription.confirm_unsubscribe_page. That's all.

Apologizing again if the initial wording sounded too demanding. I really just wanted to emphasize that it is, in my opinion, something that should be dealt with outside the regular let's-wait-for-the-next-release cycle because users wanting to unsubscribe and reading about successful subscription can really make the life of a EU-based admin rather miserable these days. :-)

gg24’s picture

Assigned: Unassigned » gg24

Working on it for a patch.

gg24’s picture

Assigned: gg24 » Unassigned
Status: Active » Needs review
StatusFileSize
new707 bytes

Hi @djg_tram,

Attaching a patch, please review.

Thanks!

adamps’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Great thanks @gg24

The policy of this module is that patches should generally include an addition to tests. I know that developers often don't like this, but if there had been tests this bug would never have occurred, and the tests will prevent any bugs in future.

adamps’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests +Plan to commit

OK I think given that this is a one-line change and fixing a regression I think we should commit even without tests.

  • AdamPS committed 39d61a1 on 8.x-2.x authored by gg24
    Issue #3084659 by gg24: Removal form redirects to subscription page
    
adamps’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Plan to commit

Status: Fixed » Closed (fixed)

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