SimpleTopic is an integration module for SMF1.x. Basically, it allows node replication into a SMF forum. Each time a node is inserted, a new topic is automatically created inside the SMF boards. The behaviour is configurable, so that you can choose which content type node - in which board.

This is a module for D6, and it's under developement. It depends on smfforum module, since it provides core integration and API facilities.

Project page: SimpleTopic
Repository: git clone --branch 6.x-1.x http://git.drupal.org/sandbox/brazorf/1388094.git simpletopics (PAReview)

Reviews of other projects:

Edit 2012-07-05, adding some more review

Thank you,
Regards.

CommentFileSizeAuthor
#2 pareview-1568084.txt16.63 KBtraviscarden

Comments

brazorf’s picture

brazorf commented
Issue tags: +PAreview: review bonus

Applying to review bonus program.

traviscarden’s picture

traviscarden commented
Status: Needs review » Needs work
StatusFileSize
new pareview-1568084.txt16.63 KB

Hello, @brazorf. Here's an automated review by PAReview. I've added a link to the online tool to your issue summary so you can re-run it as you fix the errors it reports:

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 6.x-1.x branch:

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. Get a review bonus and we will come back to your application sooner.

Now a few comments on a brief manual inspection:

  • A README file and proper in-code documentation will make it much easier for others to review your module. Fixing the coding standards violations reported above will go a long way.
  • Some of your @fixmes are as naughty as the comments suggest and should be corrected—as obviously you intend to. :)
  • You should probably namespace your template filenames to prevent collisions with other modules.
brazorf’s picture

brazorf commented
Status: Needs work » Needs review

Hello Travis,
thank you for the quick review.

I ran coder and fixed every warning but the 

global $smf_settings;

line: i can't get rid of this, because that global var is defined in the smfforum module itself.

I cleaned the master branch, added basic documentation files, fixed that naughty issue and namespaced the template filenames.

Regards

brazorf’s picture

brazorf commented
Status: Needs review » Needs work

I noticed some more things to fix, changing status to needs work.

brazorf’s picture

brazorf commented
Status: Needs work » Needs review

Hello there,
i ran pareview few seconds ago, after fixing most of the relevant warnings.

This is the last output:

FILE: ...view/sites/all/modules/pareview_temp/test_candidate/simpletopics.module
--------------------------------------------------------------------------------
FOUND 10 ERROR(S) AFFECTING 10 LINE(S)
--------------------------------------------------------------------------------
66 | ERROR | global variables should start with a single underscore followed
| | by the module and another underscore
107 | ERROR | Whitespace found at end of line
149 | ERROR | global variables should start with a single underscore followed
| | by the module and another underscore
164 | ERROR | If the line declaring an array spans longer than 80 characters,
| | each element should be broken into its own line
191 | ERROR | If the line declaring an array spans longer than 80 characters,
| | each element should be broken into its own line
200 | ERROR | Whitespace found at end of line
202 | ERROR | If the line declaring an array spans longer than 80 characters,
| | each element should be broken into its own line
209 | ERROR | If the line declaring an array spans longer than 80 characters,
| | each element should be broken into its own line
222 | ERROR | Parameter comment indentation must be 2 additional spaces at
| | position 1
225 | ERROR | global variables should start with a single underscore followed
| | by the module and another underscore
--------------------------------------------------------------------------------

I have the global variable issue, and i simply cannot fix that. It depends on the smf module as i stated above.
Some "whitespace at end of line" i'm fixing right now, and that long lines: i've read coding standards about this, but i can't find out how to correctly wrap lines. I need some suggestion about how to fix this thing.

Thank you :)

p.s. I fixed all the stuff coming out from Coder, except for the global var issue.

klausi’s picture

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

Coding standards about array formatting can be found at http://drupal.org/coding-standards#array . And you can ignore the global variable false positives.

manual review:

  1. simpletopics_init(): this will be called on every single page request. Are you sure that you need to include that stuff always? You should include code only in functions when you actually need it.
  2. simpletopics_create_topic(): this is vulnerable to SQL injection. The node title is user provided input and you simple concatenate it into a DB query string. Please read http://drupal.org/writing-secure-code again.
  3. mysql_escape_string() is depreacted and should not be used.
  4. _simpletopics_fetch_user(): another SQL injection vulnerability.

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

brazorf’s picture

brazorf commented
Status: Needs work » Needs review

Hello there,
i think i've fixed most of the stuff.

Waiting for your kind comments.

klausi’s picture

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

manual review:

  1. the README should mention that this module only works on MySQL DBs.
  2. "Copyright 2012 Fabrizio Ranieri": no need for copyright notes as all code on drupal.org is automatically GPL. Can add credits to your README file.
  3. simpletopics_admin(): if you are just displaying a form you can directly use drupal_get_form as page callback in hook_menu() and remove that function.

But otherwise looks RTBC to me. Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

brazorf’s picture

brazorf commented

Hello klausi,
i have removed copyright info from source code, and i replaced that callback function with drupal_get_form directly in hook_menu.

About the 1st point, im not sure: the module itself would run over any dbms, since i am using drupal database abstraction layer. You can see some mysql_* that is related to SMF, that only works (in 1.x version) with mysql. This means you can run them on different db instances and still the module should work fine.

Thank you for your revisions :)

Regards

brazorf’s picture

brazorf commented
Issue summary: View changes

Updated issue summary.

brazorf’s picture

brazorf commented
Issue tags: -PAreview: security +PAreview: review bonus

Adding PAReview: review bonus tag

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue tags: +PAreview: security

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

brazorf’s picture

brazorf commented

Ops, sorry.

klausi’s picture

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

No objections in more than a week, so ...

Thanks for your contribution, brazorf!

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:

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.

brazorf’s picture

brazorf commented

Thank you klausi :)
I'll read every useful link, and i'll do my best to properly give my contribute.

Best regards

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Edit 2012-07-05, adding some more review