Closed (fixed)
Project:
Simplenews
Version:
8.x-2.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
30 Sep 2019 at 12:31 UTC
Updated:
18 Jan 2020 at 13:34 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
djg_tram commentedComment #3
adamps commentedThanks 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.
Comment #4
djg_tram commentedYes, 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... :-)
Comment #5
gg24 commentedHi 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!
Comment #6
djg_tram commentedYep, sure (sorry, I haven't logged in for a few weeks). It's as simple as the
ConfirmRemovalFormloading the wrong redirect address. Just look at the line insubmitForm: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. :-)
Comment #7
gg24 commentedWorking on it for a patch.
Comment #8
gg24 commentedHi @djg_tram,
Attaching a patch, please review.
Thanks!
Comment #9
adamps commentedGreat 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.
Comment #10
adamps commentedOK I think given that this is a one-line change and fixing a regression I think we should commit even without tests.
Comment #12
adamps commented