This is a small and efficent module that allows you to set custom html code into head section (like meta, script, link tags or else) on node edit pages.

Here is link
Here is git link git.drupal.org:sandbox/cainrus/1742794.git

Drupal 6 module

Files: 

Comments

Milena’s picture

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.

swarad07’s picture

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

http://ventral.org/pareview/httpgitdrupalorgsandboxcainrus1742794git

Cheers,
Swarad

Milena’s picture

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()

<?php
 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

<?php
 $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()

<?php
 
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.

Milena’s picture

Status:Needs review» Needs work
Danny Englander’s picture

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!

Danny Englander’s picture

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...

cainrus’s picture

Status:Needs work» Needs review

Milena, swarad07, highrockmedia
Thank you for review. You helped me very much.

While waiting for manual review of your module please correct issues found by automatic review:

Done.

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

Done

It seems that Inject provides very similar functionality.

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 remove the LICENSE.txt file. Drupal.org will add the appropriate version automatically during packaging so your repository should not include it.

Done

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().

It was a bug, i wanted to clean table variable from settings with module name prefixes. Fixed.

It is better to use common names. If you want to credit yourself you have README.txt or project page for it.

Module was developed for my company, i forgot to change settings path. Fixed.

It's a yoda condition (worse readability). Also false should be FALSE.

Some times yoda style can help you to avoid errors. When you type $type = 'page' instead of $type == 'page'. But fixed anyway.

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

Already fixed.

false should be FALSE

Done

After I activated the module, I could find no evidence that it actually existed within Drupal.

Added drupal_set_message to hook_install with instructions.

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.

Fixed.

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.

Fixed.

There is a typo in the README.txt, "efficent" should probably be "efficient"

Fixed.

You also have a "LICENSE.txt" file, you would need to delete that as it gets added automatically later.

Fixed.

Thank you all for review! You helped me very well.

cainrus’s picture

grisendo’s picture

Status:Needs review» Needs work

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');

cainrus’s picture

Status:Needs work» Needs review

grisendo

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

Done.

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

Done. Thank you for your review, i've been learned new things.

stBorchert’s picture

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).

pirog’s picture

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)?
sreynen’s picture

Title:Node custom head html» [D6] Node custom head html
teyser’s picture

StatusFileSize
new576.06 KB
new755.53 KB
new770 KB
new10.64 KB

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.

kscheirer’s picture

Status:Needs review» Reviewed & tested by the community

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.

cainrus’s picture

Hi Teyser!

1.Could you please mention the version of this module in the .info file.

The version string will be added by drupal.org when a release is created

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.

Fixed!

Hi Kscheirer!

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.

Fixed!

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

Fixed!

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

Fixed!

Regarding #4 above, you should probably check the $nid has a value before saving.

Fixed!

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.

Fixed!

---

Thank you for your thoughtful review guys!

kscheirer’s picture

Status:Reviewed & tested by the community» Fixed

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:

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.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.