Login Redirect

Manual review:

1. What's the difference with LoginToboggan http://drupal.org/project/logintoboggan ?
2. Would you consider merging your project into LoginToboggan?
3. installation, configuration howto.
4. remove version in info file

instructions on your project and a README.txt is always very good, hook_help() ofcourse is welcome.

I think you've misunderstood, you can remove version in info file, because it will be added automatically when release.

You should put your code into a branch, Moving from a master to a major version branch.

Old commits history is no harm, just leave it.

Hi,

I think this module is too short. I tried once with a similar module 2 weeks ago and it wasn't accepted. You should have at least 120 lines of code.

Some things are in plus there: you don't need to implement hook_install only to show the message that module was enabled. Drupal is doing that for you.

Also I don't understand why you need the status setting there. I think you should redirect the page automatically if and only if the redirect is not empty.

Also I would filter the _GET parameter for xss attacks. It is now a big security problem, cause it will not affect your site but the external one could be affected.

Also you should modify the module's weight to be a huge number (else all the user login form submits added by other modules with a bigger weight wil not be executed due to your drupal_goto.) Or another way to do this I think is to register a drupal shutdown function during your submit, which will make the redirect for you.

Also I forgot to mention, that your project page is only one line long. You should try to follow the tips for a great project page: http://drupal.org/node/997024.

Hi,

Drupal is calling the implemented hooks from the modules depending on the module weight. Every module is having a numeric weight, stored in the 'system' table in the database. If you have several modules implementing the same hook, the hook calls are ordered by the module's weight.

Anyway, this is one of the solutions, I think the solution to register a Drupal shutdown function is better.

Yes, and you should send the url as a parameter to that function.

You should write something like: drupal_register_shutdown_function('my_shutdown_function_for_redirect',$url);

and you write that function to make the drupal_goto.

Manual Review:
In manual review I found only one issue.
1) Please remove t() from login_redirect_menu() function it is not neccessary to use t() in hook_menu().

Automated Review:
There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Please try to resolve http://ventral.org/pareview/httpgitdrupalorgsandboxlegendm330661626862git

Manual review - issues in login_redirect.module file:
1. You don't have description for the hooks you created, follow the link below and add description.
http://drupal.org/node/1354#hookimpl
2. In function login_redirect_help($path, $arg) - Use theme('item_list', ...) to create list markup, instead of ul, li.

Hello legendm33066,

I am a fellow applicant and have been reviewing your project towards geeting a review bonus for my application.

It's my first review so please take the following in this perspective.

Automatic review:
- ventral PAreview is clean

Manual review:

'needs work' from my POV:

- lines 10 and 19: menu items 'admin/login_redirect' and 'admin/login_redirect/settings' are too high level within the administration menu. In particular, 'admin/login_redirect' just presents a page with a link to the settings form - it seems redundant to me.
You may consider to remove the first menu item (and its callback login_redirect_main_page() ), and change the
second to 'admin/config/system/login_redirect'. This would place your configuration under a more appropriate section.

- follow up placing a configure = admin/config/system/login_redirect line entry in your login_redirect.info file

- instead of $_GET[$parameter_name], you may want to use Drupal function drupal_get_query_parameters(), which returns an array of elements you can then check against

- implement hook_install and hook_uninstall in a separate login_redirect.install file:

In the hook_install implementation you can place a drupal_message() with recommendation to visit configuration (waht you're currently doing in login_redirect_main_page())

function yourmodule_install() {
  $t = get_t();
  drupal_set_message($t('Thank you for installing Login Redirect. Please make sure you proceed to the <a href="@settings">Settings Page</a> and configure Login Redirect properly.', array('@settings' => url('admin/login_redirect/settings'))),
                     array('@settings' => url('admin/config/system/login_redirect'))));
}

In the hook_uninstall, mind about deleting the variables that you set via the configuration.

- in README.txt

1. Place this module directory in your modules folder (usually
sites/all/modules/).

This will be managed by the installation process once your project is full, so you may instead consider just

1. Install and enable the module.

Food for thought

- you may consider to implement hook_permission() and create a role that administrators could use to identify which users could or could not use the feature

- you may consider to extent the logic so to pass additional values to your destination - e.g. destination is something like

?destination=http://www.mydomain.com&passthrough_user=!!DrupalUserId!!

becomes a call to

http://www.mydomain.com?passthrough_user=1

Just an idea though.

Just for curiosity - what would be the practical usage of this? Maybe some example in the project page would help.

Overall, looks like 'needs work' for me.

Regards
mondrake

I think you should make some review for the other projects applications to get some review bonus. You shouldn't wait for help from others if you not provide any :).

> 1. This module allows redirects based on URL parameter, for example, user/login?redirect=http://www.google.com, not sure if LoginToboggan provides this function.
It probably does, but no problem. It's your first contribution. :)

> 2. No, I believe there are many users out there who just want a simple and easy way of achieving non-Drupal redirects, not a whole lot module with fancy functions.
I agree with this logic for the most part, except I'm personally a Rules fan (and a maintainer along with klausi), so I personally would have went with Rules. And for you, I did! :) See the attached example that accomplishes the goal of your module with only 1 dependency.. Rules! ...well, it has a dependency on Entity API, but that doesn't count ;)

All you need to do is import each one (in the component and rule import pages). The reaction rule is configurable to take any path, and it currently redirects to drupal.org by default. It doesn't force the redirect if the user has a predefined destination parameter, however that can be changed in the component configuration screen.

While I acknowledge that this submission uses Drupal's API well, I would personally prefer not to have this as a full project. There are already a lot of projects and at some point we should clean out the obsolete, deprecated, abandoned, etc projects. I'm not saying yours would end up becoming one if this became a full project, but if you could hold off on full project permissions, I would be happy to review your next project. Thank you for contributing!!

Please let us know how you would like to move forward with your application, and if you have any questions about the attached code, check out the Rules handbook or request support in Rules' issue queue. Thanks again!

> does the export you provide allows redirects to actually the URL parameter supplied?
You're right, it's predefined.. But it is configurable and also easily extendable. So, I'll change it to get the URL parameter and let you know.

Until then, other reviews requested.

@legendm33066: Adding URL and session header parameters to Entity API has been in the back of my mind for a little while, but this motivated me at least to file an issue to #1675020: Add query string property for the current page request. That'd be a direction worth investigating, or alternatively, the Rules URL Arguement. What do you think?

The review bonus tag will put this on klausi's list; he's a good person to get feedback from, especially on stuff like this.

Removing review bonus tag, you have not done any manual review, you just posted the output of an automated review tool. Please read through the source code of other projects and report any issues you find. And don't forget to add your manual reviews to your issue summary.

Don't RTBC your own issues, see http://drupal.org/node/532400

There are some errors according to http://ventral.org.
Make sure you have all of the things are as mentoined in
http://ventral.org/pachecklist

Please check the review i had done here
http://ventral.org/pareview/httpgitdrupalorgsandboxlegendm330661626862git

Minor coding standards are not application blockers, please do a full review.

Manual Review
It seems that everything is done with this module. Marking this as RTBC. But please also make sure you create a documentation page for this module, and even better correct the minor coding inconformity as per #40.

Forgot to change the status.

Thanks again for your contribution, legendm33066!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on irc in #drupal-contribute. So, come hang out and stay involved! Thanks again, and good luck! :)

Edit: Removed accidental, bad irc channel recommendation.

If you're looking for a maintainer, let me know ;)