PersianTools

persiantools.module

Line 19&20:
Is this really neaded ?

'file' => 'system.admin.inc',
    'file path' => drupal_get_path('module', 'system'),

Line 85: missing space after comma$fa_digits = array('۰','۱','۲','۳','۴','۵','۶','۷','۸','۹');

Line 47: skipping tags: There are other tags that should be skipped beside

persiantools.info

Thanks for the first review.

Is this really neaded ?

'file' => 'system.admin.inc',
    'file path' => drupal_get_path('module', 'system'),

It is, indeed. The page can be accessed through /admin/config/persiantools , and through the top breadcrumb at the module's settings page. Without these lines, it would show a blank page or an error.

Line 47: skipping tags: There are other tags that should be skipped beside and ?

The code skips all tags and does the conversion on the text inside the tags, as is handled by the first regex. but the content inside some tags like javascript and css is code, and inserting unicode chars in them (e.g. ‏) breaks the code, and so even their content should be skipped. added this info as comments inside the code too.

Other notes have been checked and fixed now.

Duplicated project

It looks like you have an older porject application under your username.
http://drupal.org/sandbox/heresh/1325968

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

1. I would like to test this module on a persian drupal, I've tried to find this translation on the drupal translations repository but didn't found any. Could you please post here a link to this translation package to help speed up this review ?

2. Question: On this module you test if $lang == "ar", "ar" code is armenian ?
because country code AR stands for Argentine (wich doesn't have much sens... )

Thanks !!

1. You can find the translation files for persian language here:
http://localize.drupal.org/tranlsate/language/fa
or use the localized drupal installation profile from here:
http://drupal.org/project/l10n_install

2. "ar" is the language code for the Arabic language. The Arabic and Persian language use the same alphabet and rules in writing, and are both "right to left" languages, and so Arabic pages face the same problems that PersianTools tries to solve.

Thank you for taking the time to review this module.

Thank you I will make another review as fast as possible.
Just another question, if your module can be used for Persian and Arabic, and knowing that Arabic language is quite more expanded that Persian, wouldn't it be better to give it a ArabicTools ?

Actually we are currently quite busy with all the project applications, administrators will give priority to your project if you get a review bonus

hi again !

The purpose of this Project Application process is to grant you the rights to promote your projects by your self. It would be much faster if you contribute with a module that any reviewer can easily understand and test.
If you want to be granted using this module (PersianTools) you will have to wait a long time because you will need to wait until a reviewer that has experience facing the problem you mention make a review.
(A persian / arabic reviewer)

My sugestion if you want to pass this PAReview faster is to mark this module as duplicated and create another to prove your knowledge of Drupal. Then you will be able to promote this module by your own.

Right now, there is nothing about the code that a persian reviewer would understand easier.
Most of the code is the result of my understanding of the bidi algorithm used inside browsers to detect the direction of multi-lingual text, described here:
http://www.w3.org/International/tutorials/new-bidi-xhtml/Overview-inline...
http://unicode.org/reports/tr9/

I am attaching two screenshots, from before and after activating our module, of many of the cases of mixed up text, and how they are corrected. These cases all happen in "direction:rtl" blocks containing both english and persian text, mixed with paranthesis, brackets, quotation marks, slashes (e.g. paths), dots, etc. (Please note that all the example text have been entered correctly, and the mix-up happens only in the representation of the text by the browser.)

Added "PAReview: review bonus" tag.

HI , i'm also iranian and persian ;)
it's a good module but just some little things in your .module file :
in line 73 : i think you should remove this line because it's just for testing :
// dsm(microtime() - $debug_time_start);
and a little coding thing ...
it's better that you use switch case instead of if {} elseif in branchings like that you have in function persiantools_get_closing_char in line 302
and i can say your module is great
thank you , Mohsen

hi mohsen.
thanx for the review.
your suggestions have been fixed now.

manual review:

1. persiantools_admin_settings_submit(): empty function, so remove it.
2. persiantools_uninstall(): that function should be placed into a *.install file.
3. "variable_get('digit_method');": all variables defined by your module must be prefixed with your module's name to avoid name collisions.
4. "strlen($str);": do not use strlen(), use drupal_strlen() instead.
5. "'none' => 'None',": the array value is user facing text and must run through t() for translation.
6. "substr($str, 0, $pos) . $char . substr($str, $pos);": do not use substr(), use drupal_substr() instead.
7. persiantools_convert_sm(): that function is really hard to understand and follow. Maybe split it up to more manageable parts? Just a suggestion.

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

thanx klausi, for your complete review!

the issues you pointed out have been fixed. just one note:

• a few cases of 'substr' and 'strlen' could not be replaced with their 'drupal_*' variants, inside 'persiantools_preprocess_html' function, as they relied on values returned from 'preg_match', which pointed to byte positions, rather than char positions required by the 'drupal_*' variants.

added links for my reviews of other projects, for the "bonus review" system.

added 2nd round of reviews in the issue summary.
adding the "review bonus" tag.

Looks RTBC to me now! Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

no objections for a week, so ...

Thanks for your contribution, heresh!

I updated your account to let you 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:

• 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

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get 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.

I installed the module on Drupal 7.23. When I clicked on the button named (Fix persian sort in all tables)
I got this error:

Notice: Undefined property: stdClass::$Tables_in_local in persiantools_sort_fix_submit() (line 61 of sites/all/modules/persiantools/persiantools.admin.inc).

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'convert to character set utf8 collate utf8_persian_ci' at line 1: alter table convert to character set utf8 collate utf8_persian_ci; Array ( ) in persiantools_sort_fix_submit() (line 62 of sites/all/modules/persiantools/persiantools.admin.inc).

@ehsanica This issue was used to promote PersianTools from a sandbox to a full project.
To report bugs, please create an issue in persiantools' issue queue.

added second round of reviews.