This module adds a temperature field to Drupal 7; it uses the Field API. It has settings (via the temperature_ui module) that let users choose their preferred temperature unit and then it allows them to view the value of this field in their preferred value, if field settings allow such localization. It supports fahrenheit, celsius and kelvin. It also provides useful functions to format and convert temperature values. At this time is has a select list widget. I may add other widgets in the future if needed, such as a simple textfield.

I'm applying for Full project access.

Here is a link to the sandbox project; please review the 7.x-1.x branch as this is where commits are being made.
http://drupal.org/sandbox/aklump/1188184

Files: 
CommentFileSizeAuthor
#9 branch-shell-log.txt898 bytesstfwi
#8 degree.png22.28 KBaklump
#5 marginal-changes.patch2.01 KBstfwi

Comments

jordojuice’s picture

Issue tags: +pdx-code-review

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.

aklump’s picture

Hello, Thank you for reviewing my module; I've done as you asked. There is one error on coder module but I've reviewed it for the exception and I believe all is well based on how I'm using the eval()

Line 70: Using e() or d() in your module's code could have a security risk if the PHP input provided to the function contains malicious code. (Drupal Docs)
  $value = eval('return ' . $formula . ';');

Please let me know what's next. Thanks in advance,

Aaron

joachim’s picture

Status: Needs review » Needs work

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.

aklump’s picture

Status: Needs work » Needs review

Thank you joachim for your review! I didn't know that about eof \n chars.

I've updated the code per your suggestions... (with the exceptions below).

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. I'll assume it's a matter of my verbose commenting taste, unless you correct me otherwise. Thanks.

Here are the answers to your questions...

This looks a bit odd... What is $columns?
The schema definition is a bit different between hook_schema and hook_field_schema...
http://api.drupal.org/api/drupal/modules--field--field.api.php/function/...

I don't think the description is needed here.
I pulled from the api which shows it...
http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...

Am I correct to have also changed the status of this thread back to 'needs review' ?

stfwi’s picture

FileSize
2.01 KB

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

joachim’s picture

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

stfwi’s picture

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

aklump’s picture

FileSize
22.28 KB

Hi stfwi,

I've been working and updating the 7.x-1.x version not the master (I've just been ignoring the master, so I must be confused as to workflow. I'm an svn guy, new to git, and new to the module contribution process). That said I had previously moved the enable/disables to .install on that version, not master. Also the master has not been updated since the original branch off to 7.x-1.x.

Regarding the degree symbol; I desperately want it too! But I couldn't figure that out during initial dev on this, how to get it to work; see the attached because the ui isn't allowing the entity to get through, somewhere in the chain. Maybe it's a flag I need to add to the field definition or something, like 'allow html' or html = TRUE. I'm stumped though. It's like a check_plain() or something somewhere.

I'm going to go ahead an remove the enable/disable for this module as it does seem a bit much in this case, per joachim's comment.

So:

1) Do I need to do something to keep master in sync with 7.x-1.x? Why is there a master if I'm going to dev for 7x and 6x?
2) Any idea how to make the degree symbol work?
3) What else should I do at this point to keep the process going?

Thanks for your time,
Aaron

stfwi’s picture

FileSize
898 bytes

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

stfwi’s picture

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.

aklump’s picture

Thanks stfwi,

I'm just now getting back to this module and used the snippet you send in this last comment; it seemed to work just fine in my testing. I've pasted an example of the diff below. I would not have figured this out as quickly without your help. Thanks immensily!

     'f' => array(
       'title' => 'Fahrenheit',
-      'suffix' => ' F',
+      'suffix' => html_entity_decode('&#176; F', ENT_NOQUOTES, 'UTF-8'),
       'formulas' => array(
         'c' => '(5/9) * ((float)$value - 32)',
         'k' => '(5/9 * ((float)$value - 32) + 273 )'
@@ -105,7 +105,7 @@ function temperature_units() {
     ),
klausi’s picture

Status: Needs review » Needs work

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

aklump’s picture

Hello klausi,

Are these comments based on the master branch, by chance?

Please review based on the 7.x-1.x branch; as I understand it that is where I should have been making my commits; the master branch has not been updated at all from the initial import. All the updates I've made have been on the 7.x-1.x branch.

Please let me know...

Thanks,
Aaron

klausi’s picture

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.

joachim’s picture

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

aklump’s picture

Thank you klausi and joachim; I believe I have made the requested changes:

The master has been cleaned up as well as comments in the 7.x-1.x branch.
I also made the changes per #12 as well.

Please let me know what is next, I'm excited to get this finished up.

Thanks,
aklump

aklump’s picture

Status: Needs work » Needs review
sreynen’s picture

Status: Needs review » Needs work

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 (...)
aklump’s picture

Status: Needs work » Needs review

Hi sreynen,

I went the route of function names, which kept the definition array cleaner and added drupal_alter, to allow for complete extension by other modules. Granted F to K will always be the same formula, but as a design model, I felt more comfortable defining functions which in theory could be alterable, rather than hard-coding in the calculations to temperature_convert(). The other changes I made as well. Thanks for you points!

- Aaron

sreynen’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

greggles’s picture

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.

greggles’s picture

Status: Reviewed & tested by the community » Fixed

Better status....

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

Anonymous’s picture

Issue summary: View changes

specified that the 7.x-1.x branch is for review, not master