Merging Login Destination

I agree. There are so many unique login scenarios involving redirect, and so many modules (this one, Front Page, Agreement to name a few) that seem to step on each other. A combo of LT and LD would solve most of my issues.

+1!

Interesting. I am working on a site with both currently installed. I have to say this idea has potential.

Yes I am also in support for this, I also have site which has both modules installed, if both are integrated, it will increase the ease of use.

+1 | It just makes so much sense..

+1 from me, also.

Our site has so many modules enabled on it that I didn't want another one just for this feature. So I hacked the Login Destination functionality into LoginToboggan (before finding this issue) :)

I can post a patch, if you want.

Nice work Evan, you're one step ahead of us all.

@EvanDonovan, patch would be awesome! Then we can set this to "needs review". =)

I actually decided that I would use the Rules module instead for implementing this feature, because it's a bit more flexible/easily configurable. But I may still post the patch if I have time. I don't know when that'll be though :)

If you don't have time to make a patch, just attach your modified .module file and someone else can do it. =)

Nice idea about Rules though...

Ok, I couldn't resist. Here's a patch, which I tested to correctly apply. I haven't tested the patch, though, although I did test the functionality prior to creating the patch.

I do think in most cases that Rules will be a better implementation since it allows you to set up an arbitrary number of parameters. But this might be more lightweight for those who don't want the complexity of Rules.

One thing I would suggest needs more testing: to make sure that this patch doesn't keep people logged in after they logged out, or else shows the wrong person to be logged in. I've seen a few errors like that on my site recently, but I think they're probably related to one of the other 9,000 modules I have installed :)

Nice work. Will try and test asap. =)

Edit: @EvanDonovan, see #17, sorry for any confusion!

I am not sure what your last comment means. Care to clarify?

couple of things:

1. the redirect code is inside this conditional: } elseif ($op == 'login' && variable_get('logintoboggan_login_successful_message', 0)) {, which means that it will only redirect if the logintoboggan_login_successful_message variable is also set.
2. i'm not at all convinced this is a good idea. the login destination module is far more powerful than this simple patch, and i'm not aware of any specific conflicts between the modules.

There is a conflict. If you choose to display a custom link when a user logs in for the first time and you have Login destination module enabled at the same time, Logintoboggan's function gets overridden and user gets redirected directly to Login destination's URL instead of being welcomed to your site.

i don't understand what you mean by "If you choose to display a custom link when a user logs in for the first time" -- give it to me in settings... ;)

@sonac, you're not supposed to install both. That's the point of this patch. It provides login_destination *within* logintoboggan so of course they will conflict. Disable login_destination to try the patch.

@EvanDonovan, I commented on the thread by accident (meant to comment on another) and accidentally bumped it (though it seems caused some activity, so that's cool...) All I meant was I still intend to test the patch and the edit was a mistake. =)

so if there's no real conflict between LoginToboggan and Login Destination, i'm not seeing a good reason to commit this patch, even if the problem i pointed out in #14 is fixed.

@hunmonk I take your point. Re-read your note above about the complexity of login_destination. I admit this was just an idea and I haven't looked at the code. I didn't imagine it was that complex though, judging by the UI. I prepare to stand corrected.

I still hold the position that I would prefer to have one login behaviour module than several, and logintoboggan seems the most sensible one to extend. If this patch works (and I haven't tried it yet) then it would at least allow many users to only need logintoboggan, which, IMHO, is a good reason to apply it. If login_destination contains added complexities that this patch does not provide, then people can still opt to use it.

Unless you're of the opinion that it should be all or nothing, e.g. a total merging of two the modules (which was my original suggestion - and it was only that) or forget the whole thing...?

@greg.harvey "i want it to be one module" just doesn't seem like a very compelling reason to me.

by nature, drupal's functionality is modular, and there's no ultimate rule about what territory a module *should* cover. to me, your suggestion is similar to saying "any module that has anything to do with organic groups should be merged with the OG module".

the only way that would work (and by the same token your suggested merge) is if the respective module maintainers would all put the same amount of developer resources (read coding time) into the merged module as they do into their separate module, otherwise you lose valuable volunteer developer hours.

i hope you'll understand when i say: i volunteer my time to maintain LT -- i have no desire to maintain _more_ code, especially when it's code that i have no interest in or use for... ;)

so if the folks in this issue really want to see this happen, then the effort should be towards getting the respective module maintainers to merge _all_ of their resources. the current patch on the table won't make that happen.

@hunmonk Personally, I understand completely. In the case of OG there are scores of additional modules. The only reason I came up with this is because it felt to me like LT only lacked one feature to make it *the* login behaviour module, if you follow me. The *only* thing I need another module for is to simply set a redirect after login, which is a bit annoying. I thought the features of login_destination would be reasonably trivial to add - obviously I'm wrong and I did not research it fully before opening the issue.

However it is encouraging to hear you wouldn't rule out a merge if the maintainer(s) of LD thought it were a good idea too? If that is the case, I may still raise an issue in their queue and point them to this discussion to see if they think it is worthwhile or not. What do you think? If you think it will be more of a management headache than the whole job is worth, and they should just stay as separate modules, then I respect that and we should close this issue and I'll say no more about it. =)

As a PS:

@EvanDonovan's patch will still be useful to a lot of people, I'm sure, so thank you very much for contributing that. It covers the use case I note in the first para and if people want to use and maintain the patch then there's no problem with that, as long as they understand it will probably never be committed in it's current form. =)

let's make it official: Evan's patch will not be officially committed to the module, but i will consider a full merge of the two modules if the other module maintainer is a) open to it, and b) willing to contribute a similar amount of developer time to the merged module.

Great. LD issue posted here: #511492: Merge with LoginToboggan - volunteer developer required to help maintain merged product

@hunmonk & greg.harvey:

I actually am inclined to agree that the patch shouldn't be committed to LoginToboggan. As I looked at providing redirection on my own site, I ultimately decided to go with Rules, because it was a more generic solution. But it would definitely be overkill to install Rules for this simple functionality, so I decided to post my initial patch for the people who could benefit from it, more as a starting point, then as something to commit in its present form.

I think a full merging of Login Destination would definitely be more beneficial in the end. I was not aware of that module until I saw this issue, however. So I think that the greater visibility of LoginToboggan in the Drupal community is one reason, at least, that the login destination functionality should live inside LT. But I agree, hunmonk, that without the support of the maintainer of Login Destination that would be needlessly burdensome for you.

Finally, I would say in response to hunmonk's review in #14 that that the $op='login' conditional should be rewritten. I just posted the code as it was when I was testing it on my site, and we had set the login_successful_message to TRUE, so it worked for us without adding another level to the conditional statement.

So if my code is useful for anyone, it might be good if they would post an updated version of the patch with the rewritten conditional. Then we can mark this issue postponed on #511492: Merge with LoginToboggan - volunteer developer required to help maintain merged product, or something.

@hunmonk: +1 for merging Login Destination valuable features with LoginToboggan module. Because according to Drupal.org webmasters as of 2009-Aug-30 Login Destination module seems abandoned and nobody has offer to maintain it. For these reasons my vote goes for merging without waiting for a reply from Login Destination maintainer.
Read more at
http://drupal.org/node/552602
and
http://drupal.org/node/511492#comment-1987572

@Onopoc: you might want to re-read #22, provision b)... ;)

i have no interest in adding maintenance overhead to LT. adding LD's code to LT will increase the amount of work it takes to maintain LT -- i'm not willing to do that unless i see a porportional increase in developer maintenance time for LT.

Hmm, e.g. since LD developers have disappeared, they will need replacing by somebody. I would step up, but it wouldn't be realistic. I already look after several smaller modules and have a business to run, so I can't see me having the time.

We need a volunteer here! =/

@hunmonk & greg.harvey: Good points.

@all: Any volunteers?

I'm not a developer but I would be happy to contribute dev version testing. As well as writing an updated README.txt file.

the majority of work is in core version upgrades and bug fixes -- we need a drupal dev (even a budding one) to step up.

Hi, I am a "budding" drupal dev and module maintainer. In the first 30 seconds on the Login Destination module's page my prevailing thought was "Man, we should merge this into LoginToboggan! It is a pitty to have 2 modules for one set of features..."

And then I realise that it is abandoned - so here I am volunteering to merge it. All I demand is some form of acknowledgment for my company and me - Are a mention and a link on the logintoboggan page too much?

PS. I like to do things "the Drupal way" i.e. if I find 3-4 modules overlapping/doing the same - I post issues to all lof them with an urge to collaborate (It has happened 3 times so far :) ) .

@rsvelko: Thanks for volunteering. As for the acknowledgment for your company this is up to logintoboggan module maintainer. Usually what the others do is adding an acknowledgment on the module project page at http://drupal.org/project/logintoboggan. An acknowledgment such as This module was sponsored by and developed by Mark W. Jarrell, a.k.a. attheshow, at the Jones Knowledge Integration Group™, Inc. with a link to your Drupal contact page & a link to your company. See a sample at http://drupal.org/project/text_resize
The same thing for the README.txt file an acknowledgment could be added in there.

Great!
The example with text_resize is exactly what I meant and what I am practicing on modules I maintain.

So what is the procedure now? I would need cvs access and full rights over the project - to ensure proper user migration. Correct me if there is a better/simpler way.

@rsvelko: Chad Phillips (aka hunmonk) is logintoboggan module maintainer. He decides if an acknowledgment for your company is added to the module project page. Then he can assist you with editing access to the project. He will probably reply here in this thread within the following days. If needed hunmonk can be contacted directly at http://drupal.org/user/22079

Thanks again :)

in #33 I was talking about rights over the abandoned module - for proper migration.

Some rights over the code of logintoboggan would be nice too, so I will be able to commit there.

@rsvelko: Thanks for clarifying. Here are the steps to get editing access to Login Destination module.
1. Go to http://drupal.org/node/552602
2. Reply there. I mean 'Add new comment'. Saying that you are interested in taking the Login Destination module over.
3. Still at http://drupal.org/node/552602 Re-open the issue by changing the 'Status' to 'Open'.
4. Wait. A Drupal.org webmaster will assist you with getting editing access to Login Destination module. If needed the Drupal.org webmaster for this issue can be contacted directly at http://drupal.org/user/55077

This would truly be great!
+ Subscribing

Done.

BTW:
I spoke with the maintainer of LoginToboggan in which we are going to merge LD. We agreed to comaintain LoginToboggan after/while we merge LD into it ....

Issue fixed?

@rsvelko: Fix indeed. The Drupal webmaster said that as of today Sept 2nd, 2009 the ownership of Login Destination module is passed to you (rsvelko). http://drupal.org/node/552602#comment-1999712

Thanks for volunteering rsvelko. This is great.

fixed? we haven't even started the port yet! i think postponed might be a better status.

we'll merge the two in 7.x.

Yes, postponed sounds better to me :) The initial request was to actually merge the two, not simply to determine who will do the merging. I won't be able to benefit for quite some time from a 7.x version, but I think this is a good decision. Rules module fills my needs for 6.x.

Hurrah! Great stuff guys. Really look forward to seeing the D7 version of the new, mightier LoginToboggan. It will be one cool module. =)