[D6] Taobaoke

Don't put chinese or whateva it is in your code :)

* 通过传入的数组数据，请求淘宝客内容列表 is not rly readable for must of us :)

Marking as critical as per http://drupal.org/node/894256
Sorry for the long wait.

I can't really test the functionality of the code because I don't have an API key for that web service, but there are a number of problems that I found with the code in the repository.

First and foremost, it looks like you're including a 3rd-party library in your repository (in the directory topsdk). This is not allowed. If the module needs to use 3rd-party code then you should have instructions in your README.txt or INSTALL.txt telling the user where to download the 3rd-party code and how to install it.

Other problems, in no specific order:

In hook_menu(), you should not use t() around the menu title and description.

In hook_perm(), you should not use t() around the permission name. The permission name should be all lower-case letters.

You need to remove all the CVS $Id$ tags from your code.

Installing and running the Coder module will help you find these things and help you bring your code up to Drupal coding standards http://drupal.org/coding-standards .

Repository should not contain .project

Translations are no longer distributed with modules - you should remove your translations directory from the repository.

theme/theme.inc doesn't have any functioning code. It should be removed, and references to it in hook_theme() should be removed.

theme/taobaoke_block doesn't seem to be used either. If it is, it should have a proper file extension.

Are the images in images/ made by you? If not, are they licensed GPL and do you have permission to redistributed them?

2) Happens automaticly (core handles it)
6) point them to localize.drupal.org and add strings there yourself

You still have some functions without any comments on top of it.
You need to fix that. Code module doesn't find everything :)

There are still a bunch of coding standards issues in the module. I guess that's not enough to hold this up, but you really should read the coding standards document cited above and try to make your code comply with Drupal standards. I see indentation problems, trailing whitespace, missing function documentation, commented-out debugging code, etc.

Other problems:

In your .module file, you have a whole bunch of code not contained in functions. This should not be there. The check for the TopSdk.php library should be done in hook_requirements() in your .install, not here. Likewise, your module CSS should be loaded in hook_init().

But the biggest problem I see is how you handle your system variables. You have implemented your own variable saving and storing mechanism for your "settings". You can eliminate > 100 lines of code, plus make your module easier to understand for people who know the Drupal APIs, if you simply use system_settings_form() in your taobaoke_settings_form(). To do this, you return system_settings_form($form) instead of just $form at the end of your function, and the variables will be automatically created and set based on the values in the submitted form.

Marking needs work for:

In your .module file, you have a whole bunch of code not contained in functions. This should not be there. The check for the TopSdk.php library should be done in hook_requirements() in your .install, not here. Likewise, your module CSS should be loaded in hook_init().

The rest of TR's review is strongly advised but not as critical.

You still have some coding style issues (http://ventral.org/pareview/httpgitdrupalorgsandboxlugir1170452git, you can use this site to re-test it your self).

If you got any questions on this please ask!

Changed priority from 'major' to 'normal'.

Your priority should not be at 'major' it should only be at 'normal'. The former setting is only for projects that have been awaiting review for 2 weeks or more in line with the project review guidelines.

It appears that there have been multiple project applications opened under your username:

Taobaoke: http://drupal.org/node/1176740
ImageLink: http://drupal.org/node/1275104
Comment Name: http://drupal.org/node/1242806

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, I have marked your secondary applications as 'closed(duplicate)', and left one application open (chosen at random).

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

Thanks in advance for your patience and understanding!

add some description of this module, add git repository link and specify this module is for drupal 6

I've reviewed the module, here's what I think:

Licensing: No issues found. It looks like there is no 3rd party code.

Module duplication: There are no other modules at drupal.org that integrate with the service in question.

Overall understanding of Drupal API's: Looks good to me.

Security: Nothing major found. I think it might be a good idea a pass the content received from the external XML API through check_plain() or check_markup() before printing?

Drupal coding standards: Just a few missing file-ending-newlines, according to PAReview: http://ventral.org/pareview/httpgitdrupalorgsandboxlugir1170452git

Code documentation: Not a huge amount of comments in the code, but it seems easy enough to understand.

Now, working with the project application reviews is a new thing for me, so if there's any feedback about how this should work, feel free to review my review... ;)

I am reviewing this module now.

This review is for the D7 version.

The file documentation for the install file should be

/**
* @file
* Install, update, and uninstall functions for the Taobaoke module.
*/

taobaoke.module

Instead of using taobaoke_init(), you can put this line in your info file if you want the CSS on every page:
stylesheets[all][] = css/taobaoke.css

In taobaoke_load_sdk(), you might consider using the library API module if you want to be more Drupalish
Project page: http://drupal.org/project/libraries
Documentation: http://drupal.org/node/735160

In Drupal 7 there is no hook_perm, instead use hook_permission: http://api.drupal.org/api/drupal/modules!system!system.api.php/function/...

In taobaoke_theme() it looks like taobaoke_items_list is not used and should be removed.

The shop paths might clash with the paths of other modules, since "shop" is go generic. You might change it to "taobaoke-shop" if you think it is likely that a user would have other modules that could also have a "shop" path.

You should remove the commented out code in taobaoke_items_get(). If you want to look at it in the future, you can see it in the git history.

You can make it a little easier for users to find the configuration page by adding this to the .info file:
configure = admin/config/taobaoke
This will add a link on the module overview page.

We are trying to use your module for Drupal-7 already more then 1 month on our web http://tao-market.com . All we succeed in this is: for each category it can create a block and a parsing page for this block from TaoBao.com . The problem is that only one page is parsed ... And that link goes immediately to the Commodity TaoBao on Taobao.com - but not on our website!
Also, we did not find any instructions on how to fill in the fields in the module .. That is a big part of fields which are not used and we not understand how to fill them ... At least one error is made and the website falls entirely (for example, we want to add more pages for one group of items (page is 0 now), and if you put 5 in this field "page" - the site is completely falls and we have to download the dbase dump again ... and again ...
Also, we have not found where we can download your module for Drupal 6 ..
In general, every day for many hours our team of programmers 7 people, including two people of Chinese nationality is trying in vain to understand how to work with this module ... But as long as in Drupal there is nothing else better, we have to suffer every day in search of truth ... Please help us with instructions for configuring your Taobaoke module and give us the link for it for Drupal 6.

This issue is only about the project application.

Please create a new issue.

Set the appropriate default branch: http://drupal.org/node/1659588
Delete the master branch:

git checkout 7.x-1.x
git branch -D master
git push origin :master

Your installation is not very complex, you can simply put it into the readme.

It's not recommended and also really not necessary to set default values on hook_install().
Please rather make use of the second parameter of variable_get() - that's absolutely enough!

t('Taobaoke module is not enable. <br />
    The TopSDK is missing. Download it from http://open.taobao.com
    ...
    the README.txt file in taobaoke module folder.');

Please do not break and indent translatable strings in-line. This will make the translation horribly difficult.

$message = $missing_top_sdk;
Try not to waste memory by defining same variables multiple times (without reason)

if (arg(0) == 'shop') {
    $taobaoke_module_path = drupal_get_path('module', 'taobaoke');
    drupal_add_css($taobaoke_module_path . "/css/taobaoke.css");
  }

Why do you do this in hook_init() ? this is a waste of performance, just add your css directly in the page callbacks where necessary.

Still some coding style issues, you may at least remove these hundreds of whitespaces?
http://ventral.org/pareview/httpgitdrupalorgsandboxlugir1170452git

$library['error message']
I couldn't find any documentation about libraries_load() returning an error message, is there any?

t('#!index', array('!index' => $delta + 1)))
Don't try to translate non-human-readable-non-translatable text.

Rather than unset() form items, use hide() to hide them.

there are a couple of other issues but I'll stop at this point for today.

Change git repository for both Drupal 6 and Drupal 7

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Add test configuration info.