[D7] Archibald2

Hello,

You might need to download the code_review module (http://drupal.org/project/coder)

As it picks up a lot of coding issues with this module (some of which might be in library files)

I've attached a list.

That's great work.

I have skimmed through most of the PHP code in the repo (module, includes, inc, ...) and it looks it's most cleanly following the standards and is well structured.
We've been discussing the project with the origin already a bit and they have a lot of documentation around (non-english, but on a very detailled level). There's strong interest in further maintenance and improvement of the project.
I consider it a sane base and a future proof one and thus i'd love to vote for a public go. BUT..

Few Details about the code formatting: It's not yet fully clean and there are quite a few obvious things left..
Long SQL statements are not uniform indented. Some outer left, some +2 spaces indented, some +n+2.
There are some places where i see +4 indents that should be fixed to +2 only.
Some watchdogs are not cleanly formatted (multiline also). Same for log() method. It seems, second line +4, laters +2 only... This should be inverted...

Back to work atm. Will you take the time to fix this too?

OK, much better now. I can read this now much faster.
And it's awesome work in the learning area.

Some (lower prio) followup additions:
Checked your tpl.php and they contain quite a bit code. I thought we should use preprocess functions here to prepare stuff instead of using theme() functions inside of the .tpl.php variables...
http://drupalcode.org/sandbox/greenrover33/1271020.git/blob/refs/heads/7...
Here you have an "a href"... that doesn't follow our standard formattings
http://drupalcode.org/sandbox/greenrover33/1271020.git/blob/refs/heads/7...
Sometimes still if you open two brackets on one line, you indent +4 (and you close it on two lines). This is not standard. If you need two levels of indentation, open them on separate lines.
(i hope i'm right with most of this wisdom ;-) )

As i see here fast progress, i now vote for going public. I'm sure you will work a lot on this software in future.

Multiple Applications

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

Project 1: http://drupal.org/node/1270872
Project 2: http://drupal.org/node/1598666

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.

The issue description has been updated above, 3 bonus reviews done by greenrover33 as requested by klausi.

Add reviews

you also have to tag your issue with "PAReview: review bonus"

Wow, great work! Amazing speed. Hope klausi will pick it up. :-)

Review of the 7.x-2.x branch:

• Remove the translations folder, translations are done on http://localize.drupal.org
• ./form/form_life_cycle.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function ajax_dsb_client_content_form_life_cycle_add_empty_contribute($form, $form_state) {
  

• ./form/form_relation.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function ajax_dsb_client_content_form_relations_release_relation() {
  function ajax_dsb_client_content_form_relations_add_empty_relation($form, $form_state) {
  

• ./form/form_keywords.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function ajax_dsb_client_content_form_general_add_empty_keyword($form, $form_state) {
  function ajax_dsb_client_content_form_general_add_empty_coverage($form, $form_state) {
  

• ./form/form_general.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function ajax_dsb_client_content_form_general_add_empty_identifier($form, $form_state) {
  

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./css/curriculum_educa.css:                                             ASCII text, with CRLF line terminators
  

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

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.

manual review:

1. project name is "Archibald 2" but the module machine name is "dsb_client"? Very confusing, what does dsb_client mean?
2. dsb_client.install: do not globally mess with PHP settings! This is currently a security problem for production sites where you should never set "ini_set('display_errors', 'on');" as potential private information could be leaked with those error messages. Remove those ini_set() and error_reporting() calls everywhere.
3. do not declare files in the info if they do not contain a class/interface
4. "class DbStruct": all class names need to be prefixed with your module's name to avoid name collisions. Same for all the other classes.
5. do you really want to depend on the shortcut module? why?
6. dsb_client_uninstall(): drupal_uninstall_schema() is not needed in Drupal 7, so remove that function.
7. dsb_client_update_7201(): doc block: the first line should contain a short description what the update does, because that will be displayed on update.php or un drush updb. Also on the other hook_update_N() functions.
8. dsb_client_update_7205(): use db_update() instead of db_query('UPDATE ...'). Also elsewhere.
9. "define('RES_FIELD_OPTIONAL', '0');": all constants that your module defines should be prefixed with your module's name to avoid name collisions.
10. dsb_client.module: classes are autoloaded, do not require_once them here again globally for every single page request. Same for places where you uase classes: no need for require_once or module_load_include() as the classes will get autoloaded.
11. dsb_client_user_view_alter(): this is a hook implementation and should be documented as such, see http://drupal.org/node/1354#hookimpl
12. dsb_client.module is huge. Have you considered splitting it up into several modules for better maintainability?
13. theme_dsb_client_content_publish_central_status(): put Javascript into a dedicated JS file and pass variables with Drupal.settings to it, see http://drupal.org/node/756722
14. theme_dsb_client_choose_contributer_search_form(): do not build form markup yourself, use theme('button', ...) or whatever fits here.

Otherwise I did not find any severe issues (also because of the overwhelming complexitiy). Point 2 and 4 of my manual review are blockers right now. Point 1 is also important, so I suggest you rename your project to "DSB client" and come up with a clever idea what that means, or you will have to rename all kind of stuff like functions, classes, constants in your project to archibald2 ;-)

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

Dokumentation for "files[]" in info files: http://drupal.org/node/542202
Drush will bootstrap Drupal which means that the class registry is also loaded. Therefore you don't need the require_once statements.

Review of the 7.x-2.x branch:

• Remove the translations folder, translations are done on http://localize.drupal.org
• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./css/curriculum_educa.css:                                           ASCII text, with CRLF line terminators
  

• Remove all old CVS $Id tags, they are not needed anymore.
  

  translations/de.po:1:# $Id$
  

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

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.

manual review:

1. there are still quite a lot of errors from the automated review script, please take a look at those.
2. Info file errors:
   

   FILE: .../drupal-7/sites/all/modules/pareview_temp/test_candidate/archibald.info
   --------------------------------------------------------------------------------
   FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
   --------------------------------------------------------------------------------
    26 | ERROR | Declared file was not found
    27 | ERROR | Declared file was not found
   --------------------------------------------------------------------------------
   

3. The name dsb_client is still used quite often: grep -rin dsb_client *
   

   includes/file_uploader/uploader.ftp.class.php:417:    $ftp_config = (array) variable_get('dsb_client_uploader_plugin_config_ftp_' . $content_partner_id,
   includes/archibald.translate.inc:265:      ':solr_enabled' => variable_get('dsb_client_solr_search_activ', 0),
   

4. "Implements of hook_disable()." should be "Implements hook_disable().". Also elsewhere.
5. archibald_disable(): will throw fatal errors if the shortcut module is not enabled.
6. the older table name "dsbc_settings_content_partner" is still used in the install file and will throw fatal errors on installation.

That should definitively be fixed, but otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Your project page is not very detailed, please have a look at the tips for a great project page, you may also use HTML-tags for better structure.

Thanks for your contribution and welcome to the community of project contributors on drupal.org!

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.

Thank you, patrickd. I'm really happy, that archibald is now part of our community and we have a valueable new contributor.
Welcome, greenrover33!