Provide token for subscription management url

Here we go.

These forms are getting more and more complex. Depending on the state, it is used as a subscribe (checking means subscribing), unsubscribe (checking means unsubscribing) *and* update (check means subscribe, not checked means unsubscribe).

I'm really glad we have a quite good test coverage.. Took me a while to get these tests green again, I don't think I would have managed to implement this correctly without them.

This is the well-known situation when things get more and more complex.

The concept of strict user (login state) and subscribers (no login state) is no more so clearly different:
Subscription management for subscribers is something like a temporary stateful login (but still no real user / login)...

And the most risky thing with it is e.g. exposure of login-links in search engines...

While i'm happy to have this feature implemented, we need to understand that we won't go further with this non-login subscribers. Because this feature is the start for stateful subscriber management pages where other lists e.g. assign passwords to subscribers (with processes to request password, ...) to allow people to manage their newsletter/list subscriptions. And then we're doubling core processes and push subscribers to the level of users...

If we had users only: core would provide us with password request, subscribers could manage their subscriptions after login.
A manage link in the mail footer could be an auto-login (also risky!)
Also less data duplication with e.g. "subscribers need a name field!" (and much more...)

We previously also splitted up forms for different cases. Also the unsubscribe button was already once removed completely...
Why not splitting up the form into anonymous subscribe (/ unsubscribe) form AND a second self-manage form?

We need to make sure not to see us back in the 6.x-1.x one-form-for-all complexity.
Technical review will follow in a separate post. ;-)

#1: manage_url.patch queued for re-testing.

This will require a re-roll due to the user register patch that has been commited, both touch the same test method.

Fully agree with everything you said :) Trying to separate these forms in a more intelligent way might be worth considering but not right now IMHO.

Except that we should absolutely not do auto-login if subscribers would be real users. Especially not with the current hash (which is just the first 10 characters of the md5 hash + subscriber id + tid). In contrast to password recovery links, these manage links are sent around all the time and users might forward an interesting newsletter to someone else and stuff like that. Right now, this isn't much of a problem because all you can do is see the the current subscriptions.. a change still requires mail confirmation. Anyway, really not important right now :)

Looks good.

The early exits are a little inconsistent.

+      drupal_not_found();
+      return;
...
+      return drupal_not_found();

You're triggering double-opt-in even if the user was "authenticated" via hash link?
I think that's still right since this link is subject to possible exposure... At least it's not that strong as a login.

You're hashing the snid AND the mail. If a user changes his mail address, the hash is wrong. Changes won't happen here for regular subscribers. But for users the hash might change after a mail address update. This is an edge case and i'm not sure if we should consider it.

Generally consider it RTBC once the test code passes.

Oh, cross post.

EDIT: About the hashes ... Beside mail forward they're pretty safe and unpredictable as long as we integrte the private key as part of the hash source.

function simplenews_private_key() {

I still wonder a little why we're using our own hash and not reuse drupals private key...
http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_ge...

Updated mail number and removed the conflicting part from the patch (fixing an accidently disabled test method which already got fixed in the other issue). Fixed the incorrect drupal_not_found() usage.

Also added two test assertions for invalid hash format and wrong hashes. That showed that passing in an invalid hash format would result in a notice, so I unified the load and validation code in a simplenews_subscriber_load_by_hash() function and am now also verifying the basic format with a simple regex there.

This makes the patch a bit bigger but reduces code duplication and increases stability.

Reviewed with Code, but not executed.

Still looks good! (and it's also tested by bot) :-)

Commited and pushed.

Also added the token declaration in hook_token_info() that I forgot to do before.