Taarikh is a field formatter and widget module to display and input hijri dates using the fields defined by the date module. The formatter can work with all the fields defined by the date module, i.e., Date, Date (ISO format) and Date (Unix timestamp). The widget, however, does not work with Date (Unix timestamp).

The project is a sandbox at https://www.drupal.org/sandbox/hussainweb/2161577.

Repository

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/hussainweb/2161577.git taarikh
cd taarikh

PAReview output can be seen at http://pareview.sh/pareview/httpgitdrupalorgsandboxhussainweb2161577git

Manual Reviews of other projects

#2465165-5: [D7] Fullscreen gallery
#2316301-30: [D7] Responsive Blocks
#2333893-10: [D7] RePEc

-- Previous
#2457903-10: [D7] Notify log
#2458731-8: [D7] Video Filter DemocracyNow
#2453657-10: [D7] United Nations Peacekeeping Operations

Comments

kunalkursija’s picture

kunalkursija commented

Hi Hussain,

taarikh.widgets.inc

1) I am guessing that form_error() message inside taarikh_field_widget_error() should be a translatable string. (because form_set_error demands so)

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review

I have fixed various issues and errors from pareview. The output follows at the end.

The one warning related to using $form_state['input'] is unavoidable as the subsequent code in Date module depends on it. However, this use case is commented in code.

  // This is hopefully temporary as we should rarely work with form_state[input]
  // we use this as date_combo_validate reads values from here for processing.
  $value = array(
    'year' => $year,
    'month' => $month,
    'day' => $day,
    'hour' => 0,
    'minute' => 0,
  );
  drupal_array_set_nested_value($form_state['input'], $element['#parents'], $value, TRUE);

---

Review of the 7.x-1.x branch (commit c87800a):

  • DrupalPractice has found some issues with your code, but could be false positives.
    
FILE: /var/www/drupal-7-pareview/pareview_temp/taarikh.widget.inc
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------
 105 | WARNING | Do not use the raw $form_state['input'], use
     |         | $form_state['values'] instead where possible
---------------------------------------------------------------------------

Time: 253ms; Memory: 10Mb
  • No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Source: http://pareview.sh/ - PAReview.sh online service

hussainweb’s picture

hussainweb
Primary language English
commented

@kunal.kursija: Thank you for the review.

The instance of form_set_error you mention need not use t() as this is not a string literal but a variable. As you can see from the documentation of t() function, variables should never be passed to t() function except in certain cases.

Thank you for the review once again.

hussainweb’s picture

hussainweb
Primary language English
commented
Assigned: hussainweb » Unassigned
Issue summary: View changes
Issue tags: +PAreview: review bonus

Adding review bonus tag.

hussainweb’s picture

hussainweb
Primary language English
commented
Issue summary: View changes

Just updating the IS with PAReview link.

kala4ek’s picture

kala4ek
Primary language Russian
Location 🇷🇺 Novosibirsk
commented
Status: Needs review » Needs work

Hi, manual review:

taarikh.module

  • 17 - I suggest you to use &drupal_static(__FUNCTION__) instead 'static'.

taarikh.js

taarikh.element.inc

  • 103 - unused local variable $granularity, it's rewrited at 118 line;
cafuego’s picture

cafuego commented

Nice one, hussainweb.

I've enabled and played with the module and it looks to be working fine.

On adding a Taarikh date field to a node type and saving that node, I got a warning:

Warning: strtotime() expects parameter 1 to be string, array given in taarikh_process_date_part() (line 218 of sites/all/modules/taarikh/taarikh.element.inc).

I have one functionality question which doesn't affect whether this should be a full project (it should :-)

I did not customise my date formats from the default. The formatter doesn't handle hour/minute/second, which is fine (it's a date, after all) but should taarikh_api be able to handle the day of the week - the l and D characters - or is the day name not at all relevant to Taarikh dates?

hussainweb’s picture

hussainweb
Primary language English
commented

@kala4ek: Thank you for the review. I have pushed changes for the last two of your points.

Regarding drupal_static, from documentation page, you can see that this function is used if there is any possible need for resetting the cached value during a request. Since this controls adding of JS and CSS, I don't see that need at all. And static is fast (it doesn't matter here as it is run only once).

I am looking into the other error pointed out by @cafuego right now.

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review

@cafuego: Thank you again for the review. I have fixed the errors / warnings you experienced and tested it against required and non-required fields. Please let me know if you still face any errors.

cafuego’s picture

cafuego commented
Status: Needs review » Reviewed & tested by the community

The latest version doesn't produce the warning anymore, so all good. Setting the project application to RTBC.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -PAreview: review bonus +PAreview: security

Review of the 7.x-1.x branch (commit 02d896a):

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
    FILE: /home/klausi/pareview_temp/taarikh.js
-------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------
 2 | ERROR | [x] The second line in the file doc comment must be "@file"
-------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------
  • No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

  1. "form_error($element, t('Invalid hijri date specified for !name', array('!name' => $element['#instance']['label'])));": This looks vulnerable to XSS exploits. The label is user provided text, correct? Then you need to sanitize it with the appropriate "@" or "%" placeholder with t(), see https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7 . Make sure to read https://www.drupal.org/node/28984 again. Please check all your t() calls. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
  2. taarikh_select_widget_validate(): form_set_error() expects a string as first parameter. I think you want to use form_error() here?

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review

@klausi: Thanks for the review. You are right about this being a XSS exploit. I looked over the code and found this in a couple of other places and removed it. Other concerns are also fixed.

hussainweb’s picture

hussainweb
Primary language English
commented
Issue summary: View changes
Issue tags: +PAreview: review bonus

Adding PAReview: review bonus tag with new reviews.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Assigned: Unassigned » heddn
Status: Needs review » Reviewed & tested by the community

manual review:

But otherwise looks good to me now.

Assigning to heddn as he might have time to take a final look at this.

hussainweb’s picture

hussainweb
Primary language English
commented

Just a gentle nudge. :)

And thank you @klausi for the review. I will definitely try to move the JS/CSS stuff to #attached. I am just trying to keep it as close as possible to date_popup module for now.

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Assigned: heddn » Unassigned
Status: Reviewed & tested by the community » Fixed

Review of the 7.x-1.x branch (commit 02d896a):

  1. taarikh_api_format : using strlen, better to use drupal_strlen, Similarly drupal_substr.Its also at other places as well.
  2. Readme is nice. hook_help is missing in module.It would be nice to add it.
  3. and @klausi specified in #14.

No blockers.

Thanks for your contribution, hussainweb!

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 to help with excellent maintainership:

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 the dedicated reviewer(s) as well.

Status: Fixed » Closed (fixed)

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