Sina App Engine

You got some coding standart issues (http://ventral.org/pareview/httpgitdrupalorgsandboxffbum1363764git), please use http://ventral.org/pareview and try to fix them.
If you got any questions on that, please ask!

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 7.x-1.x branch:
Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):

FILE: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
140 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: ...upal-7-pareview/sites/all/modules/pareview_temp/test_candidate/sae.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
8 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/sae.module
--------------------------------------------------------------------------------
FOUND 8 ERROR(S) AFFECTING 8 LINE(S)
--------------------------------------------------------------------------------
216 | ERROR | Last parameter comment requires a blank newline after it
355 | ERROR | Whitespace found at end of line
356 | ERROR | Whitespace found at end of line
360 | ERROR | Last parameter comment requires a blank newline after it
362 | ERROR | Data type of return value is missing
369 | ERROR | Whitespace found at end of line
370 | ERROR | Whitespace found at end of line
371 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

FILE: .../sites/all/modules/pareview_temp/test_candidate/sae_storage_streams.inc
--------------------------------------------------------------------------------
FOUND 17 ERROR(S) AFFECTING 17 LINE(S)
--------------------------------------------------------------------------------
203 | ERROR | Public method name "SaeStorageStreamWrapper::stream_open" is not
| | in lowerCamel format, it must not contain underscores
252 | ERROR | Public method name "SaeStorageStreamWrapper::stream_read" is not
| | in lowerCamel format, it must not contain underscores
266 | ERROR | Public method name "SaeStorageStreamWrapper::stream_write" is
| | not in lowerCamel format, it must not contain underscores
286 | ERROR | Public method name "SaeStorageStreamWrapper::stream_close" is
| | not in lowerCamel format, it must not contain underscores
296 | ERROR | Public method name "SaeStorageStreamWrapper::stream_eof" is not
| | in lowerCamel format, it must not contain underscores
303 | ERROR | Public method name "SaeStorageStreamWrapper::stream_tell" is not
| | in lowerCamel format, it must not contain underscores
310 | ERROR | Public method name "SaeStorageStreamWrapper::stream_seek" is not
| | in lowerCamel format, it must not contain underscores
362 | ERROR | Public method name "SaeStorageStreamWrapper::stream_flush" is
| | not in lowerCamel format, it must not contain underscores
373 | ERROR | Public method name "SaeStorageStreamWrapper::stream_stat" is not
| | in lowerCamel format, it must not contain underscores
380 | ERROR | Public method name "SaeStorageStreamWrapper::url_stat" is not in
| | lowerCamel format, it must not contain underscores
434 | ERROR | Public method name "SaeStorageStreamWrapper::dir_closedir" is
| | not in lowerCamel format, it must not contain underscores
444 | ERROR | Public method name "SaeStorageStreamWrapper::dir_opendir" is not
| | in lowerCamel format, it must not contain underscores
456 | ERROR | Public method name "SaeStorageStreamWrapper::dir_readdir" is not
| | in lowerCamel format, it must not contain underscores
487 | ERROR | Public method name "SaeStorageStreamWrapper::dir_rewinddir" is
| | not in lowerCamel format, it must not contain underscores
520 | ERROR | Public method name "SaeStorageStreamWrapper::stream_cast" is not
| | in lowerCamel format, it must not contain underscores
527 | ERROR | Public method name "SaeStorageStreamWrapper::stream_lock" is not
| | in lowerCamel format, it must not contain underscores
534 | ERROR | Public method name "SaeStorageStreamWrapper::stream_set_option"
| | is not in lowerCamel format, it must not contain underscores
--------------------------------------------------------------------------------

FILE: ...areview/sites/all/modules/pareview_temp/test_candidate/安装指南.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 29 WARNING(S) AFFECTING 29 LINE(S)
--------------------------------------------------------------------------------
19 | WARNING | Line exceeds 80 characters; contains 103 characters
24 | WARNING | Line exceeds 80 characters; contains 97 characters
30 | WARNING | Line exceeds 80 characters; contains 120 characters
34 | WARNING | Line exceeds 80 characters; contains 85 characters
40 | WARNING | Line exceeds 80 characters; contains 120 characters
41 | WARNING | Line exceeds 80 characters; contains 105 characters
43 | WARNING | Line exceeds 80 characters; contains 127 characters
44 | WARNING | Line exceeds 80 characters; contains 130 characters
49 | WARNING | Line exceeds 80 characters; contains 109 characters
50 | WARNING | Line exceeds 80 characters; contains 109 characters
52 | WARNING | Line exceeds 80 characters; contains 127 characters
53 | WARNING | Line exceeds 80 characters; contains 121 characters
58 | WARNING | Line exceeds 80 characters; contains 143 characters
67 | WARNING | Line exceeds 80 characters; contains 129 characters
81 | WARNING | Line exceeds 80 characters; contains 108 characters
82 | WARNING | Line exceeds 80 characters; contains 125 characters
87 | WARNING | Line exceeds 80 characters; contains 84 characters
89 | WARNING | Line exceeds 80 characters; contains 81 characters
91 | WARNING | Line exceeds 80 characters; contains 134 characters
92 | WARNING | Line exceeds 80 characters; contains 81 characters
95 | WARNING | Line exceeds 80 characters; contains 134 characters
98 | WARNING | Line exceeds 80 characters; contains 113 characters
101 | WARNING | Line exceeds 80 characters; contains 135 characters
102 | WARNING | Line exceeds 80 characters; contains 98 characters
104 | WARNING | Line exceeds 80 characters; contains 143 characters
107 | WARNING | Line exceeds 80 characters; contains 140 characters
108 | WARNING | Line exceeds 80 characters; contains 141 characters
110 | WARNING | Line exceeds 80 characters; contains 128 characters
111 | WARNING | Line exceeds 80 characters; contains 95 characters
--------------------------------------------------------------------------------

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.
Source: http://ventral.org/pareview - PAReview.sh online service

You should change the status back to 'needs review' after a new commit and not to 'fixed'

Still formatting issues (Stream wrapper methods can be ignored)
http://ventral.org/pareview/httpgitdrupalorgsandboxffbum1363764git

Setting a module as required (.info) should be used by drupal core modules only.
required = TRUE

Your resetting the original mail system back on uninstall but as uninstalled != disabled you should set it back after disabling the module.
variable_set('mail_system', $mail_system);

All variables used and created by your module have to be prefixed with its name, also all smtp ones

In general it's hard to test this out regarding the chinese language, this is also a quite complex module so it'll take some time to get it reviewed in-depth.

Hello, there,
Thanks for your great model firstly, it really saved me lots of time to migrate my side to the SAE.

Unfortunately, a little bug.
SAE_Fatal_error: Using $this when not in object context in sites/all/modules/sae/sae_storage_streams.inc on line 89
It's easy to be fixed , I've fixed this in my module and tested all the whole module functions, it works greatly.

Thank you again.

@xuxizh
This is the application, not it's issue queue.

Please create a new issue in the projects bug-queue here.

See this which can speed up review process - Review bonus

hi, ffbum
do you have a version for drupal6?

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended here: http://drupal.org/node/1011698

Manual review of the 7.x-1.x branch:

1. do not use the "private" visibility modifier, see also http://drupal.org/node/608152
   

   
   FILE: .../sites/all/modules/pareview_temp/test_candidate/sae_storage_streams.inc
   --------------------------------------------------------------------------------
   FOUND 20 ERROR(S) AND 2 WARNING(S) AFFECTING 22 LINE(S)
   --------------------------------------------------------------------------------
      9 | WARNING | The use of private methods or properties is strongly
        |         | discouraged, use "protected" instead
     13 | WARNING | The use of private methods or properties is strongly
        |         | discouraged, use "protected" instead
   

2. "'#title' => check_plain($password_title),": $password_title is not user provided input, why do you need to use check_plain() here? Same for $password_description.
3. "$conf['file_temporary_path'] = SAE_TMP_PATH;": that constant is not defined anywhere?
4. "Implements of mkdir": This should be documented as "Implements InterfaceName::methodName()."
5. "$this->Host = variable_get('sae_smtp_host', '');": all class/instance properties should be lowerCamelCase. Also elsewhere.

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

Fix wrong git url.