[D6] Node custom head html

While waiting for manual review of your module please correct issues found by automatic review:
http://ventral.org/pareview/httpgitdrupalorgsandboxcainrus1742794git

What's more consider participating in Review bonus to get your application reviewed sooner.

You might want to consider creating branches instead of working on the master branch.

http://ventral.org/pareview/httpgitdrupalorgsandboxcainrus1742794git

Cheers,
Swarad

Hello,

It seems that Inject provides very similar functionality. Could you describe how your module differs?
We prefer collaboration over competition, therefore we want to prevent having duplicating modules on drupal.org. If the differences between these modules are not too fundamental for patching the existing one, we would love to see you joining forces and concentrate all power on enhancing one module. (If the existing module is abandoned, please think about taking it over).

Please remove the LICENSE.txt file. Drupal.org will add the appropriate version automatically during packaging so your repository should not include it.

.install file

hook_uninstall()

 drupal_uninstall_schema('node_custom_html');
  db_query("DELETE FROM {node_custom_html} WHERE name LIKE 'node_custom_html_%'"); 

While you delete all table with drupal_uinstall_schema you are also removing any data from this table. There should be no need to perform db_query().

.module file

$items['admin/teletrade/node-custom-headtags']
It is better to use common names. If you want to credit yourself you have README.txt or project page for it.

hook_nodeapi()

if ('view' === $op && variable_get('node_custom_html_' . $node->type, false)) {
It's a yoda condition (worse readability). Also false should be FALSE.

Overall

You should remove any variables you use in hook_uinstall() in your .install file.

Manual Review

Greetings. I installed the module on a local drupal 6 dev site I have, here is some feedback.

1. After I activated the module, I could find no evidence that it actually existed within Drupal.
2. Looking in your README.txt I see that you are sending the admin link to:
   admin/teletrade/node-custom-headtags. That seems like a non standard Drupal admin link.
3. I took a peek in custom_head_html.module and I see that you have:
   $items['admin/teletrade/node-custom-headtags'] on line 8 for your admin link, it should probably be $items['admin/settings/node-custom-headtags']
4. I also noticed throughout that all your function calls in all files are: "node_custom_html" but your files are named "custom_head_html" so there is a mismatch there. I am guessing that is why I was not able to get very far with testing this.
5. There is a typo in the README.txt, "efficent" should probably be "efficient"
6. You also have a "LICENSE.txt" file, you would need to delete that as it gets added automatically later.

Thanks!

ah, it seems I did a review at the same time as Milena in #3 & 4 but I think I've pointed out some other items as well...

Module Inject is for site-wide settings.
My module allows to add custom head html only for concrete node. But it's gooe idea to combine 2 modules(or more similiar) into one.
Thank you for idea.

Please, explain the difference in your project's page (http://drupal.org/sandbox/cainrus/1742794).

After DELETE query in hook_uninstall in your .install file, clear variables cache:
cache_clear_all('variables', 'cache');

The file .gitignore can be removed from the repository.
Additionally you should consider using filter_xss() in function node_custom_head_html_nodeapi() (when adding the data to the head). This prevents printing malicious code entered by the user (see Writing secure code for more details).

Potential major issues:

Although this is more performant:
db_query("DELETE FROM {variable} WHERE name LIKE 'node_custom_head_html_%'");

I think it is generally best to use variable_del(). I think the impetus for this is if someone had a module named node_custom_head_html_WHATEVER you would also delete all of their variables as well.

Here are some minor issues:

1. Might want to include links to documentation on how to enable a module in your README under the Installation section.
2. As with #11, you do not need .gitignore
3. You might want to have more descriptive commit messages in your git history going forward
4. In node_custom_head_html_save you might want to have the comment say "custom submit function" instead of "callback"
5. Line 58 in .module - "$nid = (int) $form['nid']['#value'];" - might be a better way to get NID without having to use (int)?

Hi Cainrus,

Please the list of issues faced when installed your module.

1.Could you please mention the version of this module in the .info file.
2.Please find the attached image for your reference to identify the issue of your module.
3. Custom HTML data not stored properly in the DB.
4. Got warning when install and create custom html head for the selected content type.

Regards,
-Raj.

In your README, we don't call admins "root" - user 1 does have special permissions, but "Setup permissions for user roles" or something similar would be better.

In your .info file, the name should match your filenames: "Node Custom Head HTML".

In node_custom_head_html_nodeapi() - if you just want a single field returned from the sql statement, you can use db_result().

Referring to #3 above, node_custom_head_html_save() should only take 2 arguments, just remove the third.

Regarding #4 above, you should probably check the $nid has a value before saving. Also, don't trust drupal_write_record() so much - it's a convenient function but it can fail silently.

You should also remove your .gitignore file from the repo, and your project page still refers to the path "admin/teletrade/node-custom-headtags" even though your module is properly updated to use admin/settings/node-custom-headtags.

Those are all fairly minor issues though, RTBC from me.

Don't trust drupal_write_record() so much - it will fail silently if anything goes wrong - check the return value. That's really minor though. This issue has been waiting a long time, thanks for sticking with it!

Thanks for your contribution, cainrus!

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.

----
Top Shelf Modules - Enterprise modules from the community for the community.