[D7] Prayers

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.

I install the module and the prayer page shows following errors:

Notice: Undefined offset: 0 in prayers_page() (line 106 of prayers/includes/prayers.pages.inc).
Warning: Invalid argument supplied for foreach() in prayers_page() (line 106 of prayers/includes/prayers.pages.inc).

Notice: Undefined offset: 0 in prayers_page() (line 114 of prayers/includes/prayers.pages.inc).
Warning: Invalid argument supplied for foreach() in prayers_page() (line 114 of prayers/includes/prayers.pages.inc).

Notice: Undefined offset: 1 in prayers_page() (line 114 of prayers/includes/prayers.pages.inc).
Warning: Invalid argument supplied for foreach() in prayers_page() (line 114 of prayers/includes/prayers.pages.inc).

Notice: Undefined offset: 2 in prayers_page() (line 114 of prayers/includes/prayers.pages.inc).
Warning: Invalid argument supplied for foreach() in prayers_page() (line 114 of prayers/includes/prayers.pages.inc).

You should scheck if $rows contains data before you loop over it.

Very nice README file. Very detailed.

You should consider changing the location of the "prayer" page. I can foresee that someone who would include this on their site may already have the /prayer URL in use. Consider something like: /prayer/prayertimes. It really doesn't matter so long as it has less chance of being in use already.

Other than that, looks good to me.

Hi, my manual review:
main module:

• line 31,33 - Too long string, you can split it into multiple lines.
• line 128 - Unused variabel $time_fmt.
• line 243 - Variable 'end_time' might have not been defined, if we go to 197 line (else).
• line 658 - unused variable $language_content.

• functions function prayers_cong_desc, prayers_page, prayers_make_time haven't variables clarification.
• unused parameters in function prayers_page($account = NULL, $set_title = FALSE)

$result = db_query($sql, array(':type' => 'prayers_funeral'));
  $nids = array();
  foreach ($result as $row) {
    $nids[] = $row->nid;
  }

1. $result may be FALSE, so it causes warning in foreach
2. hm, where ->fetch... method for query?

Have a nice day!

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxmunawar1867970git

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

Hey Munawar,

Loving your use of whitespace & function descriptions, made it rather easy to read. Maybe not needing so much whitespace however, e.g. after returns. Mostly suggestions from me; I did find a few places where you could drupalize your code.

In prayers.module, line 367.
Your returning a javascript link, use http://api.drupal.org/api/drupal/includes!common.inc/function/l/7
As such,

l($text,'javascript:void(0)',array('fragment' => '','external'=>true));

In prayers.module, line 394 - 397.
I don't understand why you have a set number of dates/ year(s) 0 - 3.

Cheers,

Forgot to update status, I'm sorry.

Updated status to needs work.

No problem =).

Now this is just a suggestion so I won't be changing the status. Would it be possible to represent the DST transition dates dynamically? This would allow for future proofing of your project.

Cheers,

Hey. Thanks for your contribution.
Looking at your code and enabling, here are two minor points I'd recommend.

1. prayers_install(): No need to set variables upon installation as you can make use of default values with variable_get() anyway.
2. prayer.info: would be great if you added a link to your configuration page. That provides convenience from the module page to quick link to the configuration of this module

manual review:

• I believe the proper translation function to use during install is st(). From the get_t() page :
  

  Use t() if your code will never run during the Drupal installation phase. Use st() if your code will only run during installation and never any other time. Use get_t() if your code could run in either circumstance.

• Wouldn't it be better to assign the prayers-current ID when the prayer times block is created in prayers_block_view(), rather than after page load via javascript?
• While the query in prayers_congregation_times() is complex, as it doesn't need to be modified after the fact, you should be using db_query() instead of db_select(). See the discussion at https://drupal.org/node/1881146
• After enabling the Feeds module and Feeds Admin UI, the two predefined importers don't have a bundle selected under the node processor, which causes an error when trying to import. This can be manually overridden to get it to work.
• After checking the /prayer/change/0/all RSS feed, the next page load produces errors about undefined variables in lines 670-679 of prayers.module -- the $base_url and $language_content variables are listed as undefined.
• In time_format.inc, you probably meant o use '|\+\d+' for the offset regex, rather than '|\+\d*'. The way you have it now, "+" by itself is a valid time.
• You are using CSS selectors that require IE9 or above. While not necessarily bad, it's something to note, and may restrict usage of your module.
• In your README file, update the first line under "CSS" to use the correct /prayertimes url since you changed it.

I think this is very close to being ready, but there are a few things to work out first.

First off, great documentation and README file!

The date handling code is very complex, and I honestly don't really understand what you're calculating, but if it works then that's great :) I do see some areas where you could simplify though - check out php's strtotime() function - it's a much easier way of manipulating timestamps.
For ex, $start_of_year_tstamp = time() - (60 * 60 * 24 * 365); is just an approximation, strototime() can calculate that exactly.

Please remove the require_once at the top of your file though - that bare php code will be executed on every Drupal page load. Load files like that only where they are needed.

Otherwise this code looks decent, thanks!

----
Top Shelf Modules - Enterprise modules from the community for the community.

Thanks for your contribution, Munawar!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a full project.

Here are some recommended readings on being a project maintainer:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues (this one you get to refer your module's fans to!)

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay 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 all reviewers, including afi_amouzou, hapax legomenon, Samvel, musicalvegan0, eriknewby, grandivory, and kscheirer!