dwaine [dwaine]

1. Minor: there is still .svn in the directory
2. MINOR:
   

       <?php print theme('nws_weather_forecast_element', $data[$name], $timeforecast, $options) ?>
   

   You could use instead of the the preprocess and set the output for this there. Then other modules could hook in and display something else. Also theme templates should only contain the print.

3. There is no cvs id in in the css file.

4. /**
    * Implementation of hook_schema().
    */
   function nws_weather_schema() {
     $empty = array();
     return $empty;
   }
   

   Thats not needed.

Basically it all looks fine. I didn't tryed out the module yet. Additional i don't see a reason why this shouldn't be rtbc.

Suggestions:

• Spit up the module file in .pages.inc and .admin.inc
• Use system_settings_form for admin settings
• Add check for simplexml in hook_requirements. There are some hosts, where it is not installed
• Make a views query plugin ;)

I am changing the status as per previous comment.

The implementation of hook_requirements() is not correct; or it checks the parameter $phase passed, or it uses get_t().
The global variable doesn't use the correct name, which should prefixed by an underscore, and the module name (and should be in lower case).
Some strings used in the user interface are written as Option Title, when they should be written as Option title.

The code reported in the previous comment is correct. The previous code was not corrected because used t(), and it didn't check the value of $phase.
You can use t() inside hook_requirements(), but only when $phase is equal to runtime.

I am adding the review tags.

Last posted version throws some PHP notice level warnings. Please check the syntax of your define declarations. i.e. use define('NWS_WEATHER_WSDL_URL', '...'). the constant needs to be quoted when first defined.

Ran through Coder and throws only 3 very minor coding standards problems (not a problem).

You do not need all the variable_set()'s in your _install() function. When using variable_get() always include the default setting as the second parameter.

I think the image mapping should not be stored in the variable table but stored in a database table. Your usage here really seems to be stretching the use of variable settings beyond what is intended. Just my opinion, others might disagree.

Hope this helps. Seems like a cool contribution.

Hi dwaine,

The changes look good. On the issue with the use of variable_set and variable_get methods. The function variable_set() writes the value to the variables database table, which means that it gets loaded into the global $conf variable when Drupal is bootstrapped. The correct use of variable_get is to supply your defaults as the second parameter to the function call. The function is there to allow users to override the default.

In your module block view I see you using:

$forecastdata = nws_weather_NDFDgenByDay(variable_get('nws_weather_lat', ''), variable_get('nws_weather_lon', ''), '', variable_get('nws_weather_daily_days', 7));

This is why it doesn't display properly without those variable_set() in the install. You should be using:

$forecastdata = nws_weather_NDFDgenByDay(variable_get('nws_weather_lat', '47.53'), variable_get('nws_weather_lon', '-122.30'), '', variable_get('nws_weather_daily_days', 7));

By supplying the default in the places where you use it means you don't have to load up the global name space with a bunch of default values just for your application. Only those defaults that the user explicitly sets in your admin settings form will get loaded into global scope. That puts the control back in the hands of the user.

See: the code in http://api.drupal.org/api/function/variable_set/6 - how variable_set works and where the value gets stored (i.e. global $conf).

This is part of Drupal best practice and I don't think it should be ignored.

I also find how you are storing those image mappings in the database disturbing. It's good that it's in the database rather than in the global application space, but storing them as a serialized array means that if you need to add or remove a mapping in subsequent updates that you will have to initialize a new complete array in your update function, serialize and rewrite. You will need to do this for every update is any single value changes. If you can be 100% certain there is zero likelihood that these values will never have to be changed it isn't an issue - but that seems unlikely to me.

Alternatively you can simple have a schema:

url (this should always be unique and set as the primary key for indexing lookups)
file

Then you can just lookup the image you need from the URL key.

This is more maintainable and more efficient use of resources. In your nws_weather_override_image($image) function instead of loading in the record, deserializing into an array and then doing an array key lookup, you can simply query the database "SELECT file FROM {nws_weather} WHERE url = '%' " and use db_result() to get your result as a scalar value.

Also - since you are implementing a schema you should use drupal_write_record() instead of db_query() in your install file.

Hope that's helpful and constructive input!

I am changing the status as per previous comment.

drupal_write_record() doesn't work in hook_install(), until hook_install() is not completed and the table system doesn't report the module is enabled.

[Edited by kiamlaluno to correct the last sentence]

@kiamlaluno: good catch on the drupal_write_record(). I always forget that, and always try to do it, duh!

Looks good to me. One thing you should do before releasing is to change the index in your schema to either 'primary key' or 'unique keys'. This isn't going to make a huge amount of difference in the short term, but it's important for maintaining data integrity. With a unique key index there cannot be any duplicate values. The way you are using this table demands that there are no duplicate urls. In general best practice every database table should have a primary key so just changing 'indexed' to 'primary key' should do fine.

At this point I'm happy with it. Good work!