Posted by greg.harvey on May 29, 2009 at 12:31pm
20 followers
| Project: | LoginToboggan |
| Version: | 7.x-1.x-dev |
| Component: | Miscellaneous |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | postponed (maintainer needs more info) |
Issue Summary
Just a thought, but are you in touch with the Login Destination maintainers? (See http://drupal.org/project/login_destination for details, anyone not familiar with the module.)
It would seem sensible to have ONE login utils module, and LoginToboggan is the daddy at the moment. I love it, but I'd love it even more if it included redirect on login. Perhaps two should become one?
What do you think?
Comments
#1
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.
#2
+1!
#3
Interesting. I am working on a site with both currently installed. I have to say this idea has potential.
#4
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.
#5
+1 | It just makes so much sense..
#6
+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.
#7
Nice work Evan, you're one step ahead of us all.
#8
@EvanDonovan, patch would be awesome! Then we can set this to "needs review". =)
#9
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 :)
#10
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...
#11
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 :)
#12
Nice work. Will try and test asap. =)
Edit: @EvanDonovan, see #17, sorry for any confusion!
#13
I am not sure what your last comment means. Care to clarify?
#14
couple of things:
} elseif ($op == 'login' && variable_get('logintoboggan_login_successful_message', 0)) {, which means that it will only redirect if thelogintoboggan_login_successful_messagevariable is also set.#15
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.
#16
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... ;)
#17
@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. =)
#18
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.
#19
@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...?
#20
@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.
#21
@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. =)
#22
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.
#23
Great. LD issue posted here: #511492: Merge with LoginToboggan - volunteer developer required to help maintain merged product
#24
@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.
#25
@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
#26
@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.
#27
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! =/
#28
@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.
#29
the majority of work is in core version upgrades and bug fixes -- we need a drupal dev (even a budding one) to step up.
#30
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 :) ) .
#31
#32
@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_resizeThe same thing for the README.txt file an acknowledgment could be added in there.
#33
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.
#34
@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 :)
#35
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.
#36
@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
#37
This would truly be great!
+ Subscribing
#38
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?
#39
@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.
#40
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.
#41
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.
#42
Hurrah! Great stuff guys. Really look forward to seeing the D7 version of the new, mightier LoginToboggan. It will be one cool module. =)
#43
NOTICE: I am discontinuing my maintainership for LD - after fixing 90%+ of the bugs I believe. This means also that it is up to some other brave guy to merge the two modules together.
#44
so at this point, it now looks like we need either
any takers?
#45
Maybe this is over-simplifying, but someone just pointed out on Twitter that *maybe* LD can be achieved with the Rules module? If that's correct (and it might be!) then merging might just mean creating a patch that provides some "suggested" rules replacing LD's functionality, and posting a note on LD saying it's no longer maintained, there won't be a D7 release, and LT contains a Rules implementation to cover it.
#46
@greg.harvey: Yes, Rules can do the functionality of LoginDestination, plus more. That is what I use (see #11). However, I have had to add some custom PHP condition logic to my Rule. So there might be cases in which a module might be better for the non-technically savvy.
#47
my understanding is that Rules is a very heavy implementation for something this simple. i'm not so sure i want to deal with building rules integration, either.
#48
@hunmonk: the nice thing about Rules is the UI is separated, so novice users never need know it's there. You can just wrap some default rules in a module_exists() call and if people choose to turn Rules on (no need for the UI) they'll have them. If not, no harm, no foul, right? =)
#49
@greg.harvey: i think it's bad design practice to have custom code for other modules in a module. IMO if somebody wants the integration, then they should write a mini-module that connects the two. it's not something i'm interested in doing or maintaining at this time.
#50
Ok, that's a fair point. =)
Edit: So maybe a far better approach is for someone to take over maintainership of LD and make it a rules implementation requiring Rules & LT to work. To save on time and effort and make it more supportable. That being the case, this issue could probably close...?
Hmmm...
#51
If someone is able to look into the rules integration to allow this to work, there is #678684: Rules Integration that can maybe be set to active and progress documented. Thanks!
#52
Skipping past all of the debate about logistics, I think this is a good feature/idea. If you look through closed issues you'll see someone asking about or requesting this feature about a hundred times.
It would be awesome if a simple-dimple one-line global post-login redirect field could be merged into 6.x -- but I'm still glad to see the LD merge is in the plans for 7.x.
#53
I just wanted to +1 the Rules "support" if not integration. I came across this thread looking for a way to "set user login destination page preference." Rules would be a way to do this.
EDIT: However a way to do this outside of LT and LD may be by using this module:
http://drupal.org/project/customdestination
#54
plans for merging Login Destination have fallen through. this feature request would require sponsorship or a brave soul to step up and go the work. please feel free to set to active if you are either one of those people.
#55
@hunmonk thank you for updating on this. The LD module is likely to have some conflicts with other modules (#960924: Conflict with logintoboggan) but merging with LT may actually help. Nevertheless this idea should be never abandoned.
There is currently a possibility of evaluating PHP with login redirects, should the same be done to registration redirects ? Or shall we merge just as it is ?
#56
here's a summary of the last iteration of planning that the former volunteer and i did. i most likely won't follow up on this issue with any more refinements until a commitment of help arrives on the scene, as i don't want to burn hours needlessly:
we add one textarea under LT's 'Redirections' section, called 'Other redirections', with the following approach:
role : path_before : path_afterexamples:
# all roles go to a static url* : user/login : node
# default rule
* : * : admin
There are possible problems when one user has many roles, so the system should be "first matched is last matched - users beware"
this is a simple format that a regular site admin can understand, and actually allows for more non-PHP flexibility than LD currently provides. any time a regular login happens, the setting is parsed, if a match is found for path_before, we redirect to path_after.
a few tokens for the current_path portion should exist, such as <front> means the front page, <all> means all current_paths, <other> means any paths that weren't matched otherwise, etc.
for the PHP portion, provide a checkbox, 'Use PHP snippet for Other redirections', then the admin can use that textarea for their PHP snippet in the traditional LD sense, except that the conditions and the paths are all handled in that one place.
the 'Preserve destination' setting from LD is the exact same as the recently added 'Override destination parameter' setting in LT, so those should be combined.
#57
@hunmonk thank's for the summary. We should definitely add roles to login destination (as this is the common use case) without the necessity of writing PHP snippets. I also agree with the structure (role, origin, destination). But I am not certain about the UI. The syntax is not difficult but though it is not a natural way to handle such things in Drupal. Doing it in the way access rules were done in D6 seems like a nicer idea to me. So we may keep current LT setting the way they are now, but move the extended LD settings to another config page (like e.g. "Search and Metadata"). So I would see it the following way:
- roles (checkboxes with roles)
- condition (paths, without php I think)
- destination (static or PHP)
So the configuration page (for adding a rule) would look similar to how LD looks now (with roles added) but it would be just for one rule, not the whole LD as it is now.
Actually the development (it is quite a lot to change in the current LD) may be done on the LD branch. And then we may merge and resolve conflicts.
I do not know whether you had the intention to allow regexp but I am not for it. First for roles we would need only some kind of checkboxes and second for paths it is better to keep current Drupal way of handling it (/node/1/* for example).
#58
mithy was kind enough to forward me the LD work he has done for a 7.x port of the module, with an eye towards merging it with LT. i've given the code a quick review -- on the whole it seems sensible, but there are a fair number of refinements that would be necessary in order for it to be merged w/ LT. off the top of my head here are a few to get started with:
most importantly, i will NOT merge these two modules unless i have a firm commitment from somebody to co-maintain LT with me -- otherwise i will not increase the coding burden of the module.
#59
I have good news on this. My company is prepared to "sponsor" this, by providing some developer hours to make up a maintainer. I might not be one person all of the time, but it would be one of a few Drupal developers and they could be allocated time to work on this on a regular basis - tbd. I'll ping you via your contact form, but it sounds like I might have the "maintainer" you've been waiting for (or at least the commitment to maintain, not one person necessarily...)
#60
@hunmonk thank's for the review. I am happy to hear that you find it sensible. I would like to agree first on the logic and UI, then dig into implementation refinements as the code is just in prototype stage. If you think that the solution I presented (as it has been pretty much expanded comparing to the current LD shape) fits into LT logic and may be merged that way I will prepare the patches then.
Thank's for the path handling remark, I need to check it as it affects the current LD code.
I will take care of the LD part in LT as I will be pretty much off duty if the 2 modules get merged.
#61
@mithy: perhaps you can find me in #drupal sometime soon, and we can talk about the UI, etc.
#62
any updates on progress for this? no rush, just wondering :)
#63
this is not going to happen. after many talks with the current LD maintainer, we've determined that the best approach is to move LT's redirect functionality into LD, and leave them as separate modules.
the LD maintainer is currently working on the new features, and we will deprecate the LT redirects once they are released in LD.
#64
oh cool, that works for me. Looking forward to the new and improved LD.
#65
Ye, we both decided that this is not the moment for such move. It is not a bad idea in the whole (although there are some serious quirks) but we are long past after the dev period for d7 and LT already has a stable release. The actual idea behind the merge was to have one redirect module that is independent from the core triggers/actions which are underpowered and from rules which are heavy - and this is going to happen.
The main idea behind LT is to be the extension to the core user module. Redirecting may be viewed as a part of it, it may also the other way round. We may actually come back to this idea when the d8 landscape emerges.
#66
LD was released with the new features. I think it is now time to deprecate those features from LD.
Would you also consider fixing the $_GET['q'] variable in the LT's unified login scheme, code provided:
<?phpswitch ($form['#form_id']) {
case 'user_register_form':
if (drupal_match_path($_GET['q'], 'user')) {
$_GET['q'] = 'user/register';
}
break;
case 'user_login':
if (drupal_match_path($_GET['q'], 'user/register')) {
$_GET['q'] = 'user';
}
break;
}
?>
#67
please file #66 as a separate issue, and include the patch in the proper unified diff format. also if you could provide a bit more explanation of why the change is necessary that would be helpful.
which LT features have you duplicated in LD? i want to make sure i'm only deprecating what LD is replacing...
#68
LD is now capable of redirecting to a specific page after registration and after using one time login link (email validation). It is possible to specify whether the redirect should happen after setting the password or before.
I would also like to ask you to do some testing from your point to make sure that LD is properly handling all scenarios when LT is involved.
#69
A single field in LoginToboggan to assign a default destination field upon successful login would save me a huge amount of headache on almost all sites I work on.
#70
@DamienMcKenna: Yes, but it's should be rolebased like LD.
#71
subscribing...