[D7] Dark Sky Forecast

The automated review of your project by PAReview has found some issues with your code. As coding standards make sure projects are coded in a consistent style we ask you to please have a look at the report and try to fix them.

Also:

• Set the default Git branch.
• Delete README.txt
• Do not include .gitignore in the repo you push (make it ignore itself).

3rd party assets/code

No: Does not follow the guidelines for 3rd party assets/code.
The theme/module comes bundled with Weather Icons (http://erikflowers.github.io/weather-icons/), which appears to be third party asset authored by Erik Flowers. Third party code/assets is not generally allowed on Drupal.org and should be deleted. It is good that you acknowledge Erik Flowers in the README, but this is not enough to comply with our current policy.
This particular asset is made available under the MIT (CSS) and SIL Open Font License (Font) license. This also violates policy, since all assets hosted on Drupal.org must be licensed under the GPL V2+ license.
This policy is described in the 3rd party libraries and content on Drupal.org. It also appears in the Drupal Git Repository Usage policy you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.
In order to comply with our license policy, you need to remove these assets from your repo and provide instructions to users about how to download these assets from their source and combine them with your project.

No duplication

No: Does not inform about module duplication and/or fragmentation on the project page. (This information is given in the application summary, but it belongs on the project page.) The project's users need to be informed about possible functional overlap. This should be made is section with the heading "Similar projects and how they are different" on the project's project page that:

1. acknowledges the existence of similar projects; and
2. briefly explain how they are different.

Project page

Please take a moment to make your project page follow the Project page template and it may be a good idea to also read tips for a great project page.
In particular, you should add the sections "Similar projects and how they are different" (see previous point) and "Dependencies". This module depends on SaaS delivered by Forecast.io. This dependency, and its associated pricing plans must be disclosed on the project page.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi eriktaylor,

Your module looks interesting. I have done code review can you please do below fixes.

1) In ds_forecast_get_api_url() please use url() function and use
$options = array('absolute' => TRUE); as its external url.

2) In ds_forecast.admin.inc, you have used system_settings_form, in this case you are not required to explicitly set variables,

 // Set variables for all settings.
  variable_set('ds_forecast_api_key', $form_state['values']['ds_forecast_api_key']);
  variable_set('ds_forecast_geo_lat', $form_state['values']['ds_forecast_geo_lat']);
  variable_set('ds_forecast_geo_long', $form_state['values']['ds_forecast_geo_long']);
  variable_set('ds_forecast_period', $form_state['values']['ds_forecast_period']);
  variable_set('ds_forecast_cache_lifetime', $form_state['values']['ds_forecast_cache_lifetime']);

System form handler will take care of this.

Thanks,

Thank you for adding the link to Forecast IO integration on the project page. However, there is a typo in the URL. It is "https://www.drupal.org/project/forecastio" - you've written "https://www.drupal.org/projects/forecastio", which gives a 404).

Automated Review

Passed automated review on paraview.sh

Local testing with Coder found some issues identified below

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md

[Yes: Follows] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

[No: List of security issues identified.] See coding issues below.

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (+) Line 73 (ds_forecast.admin.inc): form_set_error() and form_error() only accept filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar.
2. (+) Implement a .install file to clean up variables when the module is uninstalled.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

This review uses the Project Application Review Template.

Your code is super clean and well documented. The README file is detailed and easy to understand without having to cross check items against code. This module will be a good addition to the community.

A couple other things to note:

The hook menu declared to create admin/config/ds_forcast shouldn't be titled "Weather" as it's way too generic. This could be resolved by changing the path to admin/config/weather or changing the title. If left as Weather, the name of your menu in the administration area (titled "Settings") could be too generic. Consider changing this to something like "Dark Sky Settings" in case other modules want to utilize the Weather section for configuration.

Lat/Long default values might have more documentation surrounding them in the form. The values provided by default could be the users current location based on a geo location library, or alternatively, move these default values into a description field if you're simply wanting the user to have an example of what value this field takes.

Forcast period contains the word Minutely. I love this and it makes sense to me, but might be confusing to non-english speakers and wouldn't be caught gracefully with generic translation lists. You might consider checking out a discussion about this exact word at this Stack article.

Code looks clean overall.

Here is my feedback:

1. Consider moving the configuration to /admin/config/services/dark-sky-forecast. I don't really see a need to have two menu items when you only have one settings page.
2. I'm a big fan of having the module name and folder match. I like dark_sky_forecast.module with the appropriate function naming.
3. Consider adding validation to your form; I was able to enter complete nonsense for the API key, longitude, and latitude.
4. To make the module more maintainable in the future, I'd probably go with the following in the .install file:
   db_delete('variable')->condition('name', 'ds_forecast%', 'LIKE')
5. The README configuration location needs to be updated.

Thank you for your contribution!

After 2017 March 7 everyone can promote a project to a full project.  A full project has a short project name and a drupal.org/project URL.  It can also have releases (like alpha1 or 1.0).  Edit your sandbox project, and then choose the 'Promote' tab.

https://www.drupal.org/docs/8/understanding-drupal-version-numbers/drupa...
https://www.drupal.org/docs/8/choosing-a-drupal-version/what-do-version-...
https://www.drupal.org/docs/8/understanding-drupal-version-numbers/what-...
https://www.drupal.org/docs/8/choosing-a-drupal-version/release-stable-v...

If you'd like to opt into security coverage, please ensure your module is ready for a full release, and then set this issue back to 'needs review'

Immense apologies for how long it took to get to this review completed.

I am closing this application for the lack for replies. I take the OP just needed to be able to promote the project.