Temperature Field Module for Drupal 7

Hello, thank you for your contribution!

Please remove LICENSE.txt from your module, that will be added by git.

Please ensure that your module has been run through Coder module (http://drupal.org/project/coder) on minor (most). Ignore // $Id$ and similar tags as those are no longer used with git. They can be removed.

Just a quick eyeball review...

/**
 * Implementation of hook_enable()
 */
function temperature_enable() {
  //make a note of who enabled it
  global $user;
  watchdog('temperature', 'temperature module enabled by %user (uid: %uid)', array('%user' => $user->name, '%uid' => $user->uid));
}

This belongs in your install file. Though there's no need for logging this anyway. If there were, it would be system.module's job.

define('TEMPERATURE_DEFAULT_UNITS', 'f');

ORLY? I think you'll find the default international unit is c... ;)

> temperature_field_schema

This looks a bit odd... What is $columns?

    'administer own temperature units' => array(
      'title' => t('Administer Own Temperature Units'), 
      'description' => t('Update own temperature units.'),
    ),

I don't think the description is needed here. Also, check how core does it but I suspect it'll be sentence case not title case.

One last point, some of your files are missing a newline at the end.

Hi Aaron,

I installed the module and checked the code. I have a patch file with some marginal change proposals:

1) It's degrees celsius, so added the º (I know, we bloody Europeans with their units :-) )
2) Moved enable/disable the sub module in temperature_ui.install.

The code doc, readability, text and link escaping, logs and error handling looks all quite good in my
opinion. Notice: I did not check the UI yet by using it, only code review.

Cheers

Stefan

> Regarding the enable disable watchdog calls... I'm guessing that's not a deal breaker; I picked that up somewhere along the lines and it's actually been handy a few times in troubleshooting which user did what, so I usually add that to all my modules

It really is totally superfluous. If you want that kind of logging, add it as a patch to something like devel or util module. It's absolutely not up to modules to do this.

Yes, it doesn't make any real difference, it's just a structural review proposal.

Hi,

[1] My note referred to the temperature_ui/temperature_ui.module file, there are the enable/disable hook with watchdog as well.
It's ok to work on the 7.x-1.x branch. The master branch is just the default branch starting up a GIT project, there is nothing really special about it. Depending on the GIT project, people use it as a guide branch (to see "how far they are away from master"), but technically GIT doesn's care - one of the GIT/mercurial advantages. Probably the first answer in http://superuser.com/questions/80379/git-branches-how-exactly-do-they-wo... can give you some more information how some people deal with the master branch. For downloadable DP releases, the tags are important.

[2] According to the degrees (sorry about not checking the select box again, was quite late, as programmers always say when they commit non-fix :-) ), let me check this again. I'm not sure if you can rely on using utf-8 instead. I will browse a bit the core and doc (because I wanna know now as well how to handle this properly). A first idea was to split the html and utf-8 for the corresponding representations in the assoc unit declaration array, but this would probably not work for the select-field if the encoding is not declared as utf-8 in the http-header/html-header and the browser does not assume it neither. So, ... let's check a way around this, I am sure it will be trivial in the very end, as DP covers a lot :-), but to find it ...

[3] According to getting the project release process - I think the DP experts will be able to say this, I feel to new in the community to make a reviewed statement (as reviewing is also a learning process for me).

Hi again,

Although issues in modules or themes are reported here and there, the core (common.inc) declares content type utf-8 in the header, and there are actually a lot of drupal_**** string functions treating utf-8 strings. So probably the function drupal_convert_to_utf8() it the one you want to use to convert an ISO-8859-1 string in the source code into utf-8. A php native function is (http://www.php.net/manual/en/function.html-entity-decode.php). If I would have to do it without DP, this would have been my approach to ensure that the character is still correct even if someone changes the source code char encoding.

<?php $degree_utf8 = html_entity_decode('&#176;', ENT_NOQUOTES, 'UTF-8'); ?>

Here the common.inc code FYI:

<?php
// [...]
function _drupal_default_html_head() {
  // Add default elements. Make sure the Content-Type comes first because the
  // IE browser may be vulnerable to XSS via encoding attacks from any content
  // that comes before this META tag, such as a TITLE tag.
  $elements['system_meta_content_type'] = array(
    '#type' => 'html_tag',
    '#tag' => 'meta',
    '#attributes' => array(
      'http-equiv' => 'Content-Type',
      'content' => 'text/html; charset=utf-8',
    ),
    // Security: This always has to be output first.
    '#weight' => -1000,
  );
// [...]
?>

Hope it helps, cheers.

* lines in README.txt should not exceed 80 characters
* info file: only list files there that contain classes or interfaces
* info file: do not include "version", this is added by drupal.org packaging automatically
* Remove old CVS $Id tags from all files, not needed anymore
* "Implementation of hook_help()" should be "Implements hook_help()"
* "//if the node id is not yet set, then we haven't stored anything and so we're" there should be a space after '//'. Also comments should start capitalized.

Then delete everything in the master branch if you don't use it, e.g. like here: http://drupalcode.org/project/rules.git/blob/refs/heads/master:/README.txt

No, most of my comments still apply in 7.x-1.x.

 * @param float $temperature
 * @param string $units
 * @param int $scale
 * @param string $target_units

Docblocks that just read back the parameters are no help at all. Each param needs at least a few words to say what it is.

Hi aklump,

Please fix the following issues, then move this back to "needs review":

• version = VERSION can be removed from temperature.info. Version info will be added automatically by the Drupal.org packaging script.
• temperature_ui_enable() and temperature_ui_disable() should be part of a .install file, not a .module file.
• $value = eval('return ' . $formula . ';'); eval should be avoided where possible, since it's both slow and a security hole. It's not necessary here. You have 6 formulas that aren't changing, so there's no need to put this code into strings. Either put them in functions and reference the function names in the strings, e.g. $function = $unit_data[$current_unit]['formulas'][$desired_unit]; $value = $function($current_unit);, or simply reference the units and define the math where you're actually doing it, e.g. if ($current_unit == 'c' && $desired_unit == 'k') { $value = (float) $value + 273; } elseif (...)

Looks good to me.

Please take a moment to make your project page follow tips for a great project page.

Thanks for your contribution, aklump! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in minde: Commit messages - providing history and credit and Release naming conventions.

Better status....