Redirects 404

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hello Vlad!

I like your module and would like introduce it to my team.

Automated review by http://ventral.org/: no errors/warnings :)

Manual review:

redirects_404.module

• 94 | 'title' => variable_get('redirects_404_title', '404 - Page not found'), - not the best way to alter page title, because when you change redirects_404_title variable it doesn't clear cache, so page title still will be old and makes user confused. I suggest you to use hook_menu_alter and clear menu cache.
• 152 | unused variable $data - you don't use curl return, so you can use less php memory, also above you set CURLOPT_RETURNTRANSFER = 1 to curl, maybe it will be faster to set 0, so you will decrease traffic
• 197 | exit; - it's nicer to use drupal_exit, just a suggestion.

Good luck!

Manual review:

1. redirects_404(): why do you need cURL and cannot use drupal_http_request()? And you should also implement hook_requirements() like http://api.drupal.org/api/drupal/modules!simpletest!simpletest.install/f...
2. redirects_404_admin(): doc block is wrong, hook_admin() does not exist. See http://drupal.org/node/1354#forms
3. "'access arguments' => array('administer redirects 404'),": that permission is not defined with hook_permission()?
4. redirects_404(): why do you use check_url() here, you are not printing the $path to HTML anywhere? Make sure to read http://drupal.org/node/28984 again.
5. Since the redirects_404_text variable can contain arbitrary user provided text you should either sanitize it before you return it in redirects_404() or mark the admin permission as 'restrict access' => TRUE in hook_permission().
6. redirects_404_enable(): you set the site_404 to redirect404, but in hook_menu() you specify redirect_404, so does that even work?

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

After doing some fiddling around with the configuration it looks quite reliable. The only issue I had, and I'm not even sure if it was the fault of the code, was that the redirect speed was lengthy. It got to where I redirected it but took quite a while to get there.

The module seems to be useful and working.
If I specify the path of a legacy server, it works but I get this message:

Notice: Undefined property: stdClass::$redirect_code in redirects_404() (linea 160 di /home/drupal/public_html/sites/all/modules/redirects_404/redirects_404.module).
Notice: Undefined property: stdClass::$redirect_url in redirects_404() (linea 161 di /home/drupal/public_html/sites/all/modules/redirects_404/redirects_404.module)

manual review:

1. redirects_404_settings_validate(): do not run check_markup() when saving stuff to the DB. "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database." from http://drupal.org/node/28984
2. redirects_404(): instead of setting the location header and the status yourself use drupal_goto() instead.
3. redirects_404(): this is also the place wehere you should run your check_markup() (although the config page is now protected by a high level admin permission, so it would not be necessary).

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to chx as he might have time to take a final look at this.

This shouldn't affect RTBC.

1. There's no need, not even for readability to use ($is_https) , just $is_https is fine.
2. $header_info = drupal_http_request($path, array('method' => 'HEAD', 'timeout' => 2)); that timeout won't help you. The module will be rife with support requests from people who find it doesn't work. We learned the hard that drupal_http_request does not work in many environment and times out really unreliably. I wonder whether a client side solution would be better. For what this does, it's fine, I am just warning you.

It does not matter whether you use drupal_http_request or cURL alas, this is about retarded shared host firewall settings.

Exactly.

no other objections for more than a week, so ...

Thanks for your contribution, vlad.pavlovic!

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 get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.