[D7] Workbench Moderation Profile

Hi srjosh,

Here are some quick issues to get started:

1. README.txt or README.md is missing
2. All functions defined by your module must be prefixed with your module's name to avoid name collisions with others.

Also, please take a look at the automated code review results here:
http://pareview.sh/pareview/httpgitdrupalorgsandboxsrjosh2172925git

Thanks
Shantanu

Overall, the module looks great! I just have some nit picky stuff:

1. This module entity-ifies the data on the workbench_moderation_transitions table which comes from Workbench Moderation. I think it'd be best to submit a patch to that project which does this, since that table is its responsibility. It probably won't be accepted right away, so I think it's fine to leave the code you have for this in your project for now.
2. There are class declarations directly in the .module file. It's best practice to put all class and interface declarations in an include file and then add a a line like files[] = workbench_moderation_profile.entity.inc to the .info file. After clearing your cache, this will add your classes/interfaces to the code registry so the files will only be loaded (by the PHP autoloader) when they are used. Of course, those classes are really short so this doesn't provide much value in this case, but it is a best practice! You can learn more about the code registry here: https://drupal.org/node/350780
3. Most of the stuff that's in files[] currently is unnecessary! You only need to specify files which contain classes or interfaces for the code registry to do autoloading. See the link I gave in the previous point for the full info. And .module files never need to be specified because they are always loaded.
4. The WorkbenchModerationProfileUIController class is defined in workbench_moderation_profile.admin.inc, which is mostly fine (espectially since it's listed in files[] in the .info file!) - but if you move the other classes to a workbench_moderation_profile.entity.inc file per the previous point, you should put that one there too for consistency.
5. The Doxygen comments for form constructors, submit and validate callbacks doesn't match the documentation standards here: https://drupal.org/coding-standards/docs#forms - for example, in workbench_moderation_profile.admin.inc it's got:
   

   /**
    * Administrative page callback for profile weighting.
    */
   function workbench_moderation_profile_weights($form, &$form_state) {
   

   ... where as it should be:

   /**
    * Form constructor for the for profile weight form.
    *
    * @see workbench_moderation_profile_weights_submit()
    *
    * @ingroup forms
    */
   function workbench_moderation_profile_weights($form, &$form_state) {
   

   You can pretty much copy and modify the examples from the docs that I linked above. As an aside about that particular function: it's convention (although totally not required) to name form constructors as *_form().

6. In a number of places the Doxygen comments start with /* instead of /** (two stars). In order to get properly parsed by the API module, those comments need to have two stars. Some examples of functions with only one star include: workbench_moderation_profile_workbench_moderation_states_next_alter(), _workbench_moderation_profile_weighted_modules(), workbench_moderation_profile_form_workbench_moderation_admin_transitions_form_alter, and several more.

That's all I have for now! However, I don't think there is much after that stuff...

The patch at #2174343: Make transitions into entities, for use with entity reference fields looks awesome!

All my nitpicks are addressed. I probably should have mentioned originally that I also tested the module and it works as described.

I can't find anything else that this application needs to get approved - marking as RTBC.

Excellent work! :-)

I just noticed something that I didn't catch on my first couple passes through.

On line 380 in workbench_moderation_profile.module, there is:

$wmp_states[$wmpid] = &drupal_static(__FUNCTION__);

What you probably actually mean is:

$wmp_states =& drupal_static(__FUNCTION__, array());

drupal_static() always returns the same reference for the same argument (in this case __FUNCTION__ or "workbench_moderation_profile_get_states"). So, with the current code it's making every entry in $wmp_states a reference to the same value, which would probably lead to errors if you called workbench_moderation_profile_get_states() twice with two different profiles in the same request.

Also, minor coding style issues have snuck in since you last fixed them from PAReview.sh:

http://pareview.sh/pareview/httpgitdrupalorgsandboxsrjosh2172925git

Looks good, thanks!

Minor issue: in the admin/structure menu it shows as "Workbench Moderation Profile settings" and should just be called "Workbench Moderation Profiles"

Hello @srjosh

There are still some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxsrjosh2172925git-7x-1x

Though below is the only release blocker, you should also take care of other coding style issues.

FILE: .../drupal-7-pareview/pareview_temp/workbench_moderation_profile.admin.inc
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------
 162 | WARNING | Messages are user facing text and must run through t() for
     |         | translation
-------------------------------------------------------------------------------

Please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

All issues that have been brought up have been promptly resolved by srjosh.

I'm a comaintainer, so made it a full project as a comaintainer, not sure what else should be done here -- e.g. giving srjosh ability to create more full projects?

Thanks for your contribution, Joshua!

I updated your account so you can 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 stay 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.

Hi Srjosh

When I edit profile encounter system warning as below, I can't save continue, could you help check? thanks in advance.

Notice: Undefined property: stdClass::$id in workbench_moderation_profile_form_alter() (line 573 of C:\Bitnami\openatrium-2.23-0\apps\openatrium\htdocs\sites\all\modules\workbench_moderation_profile\workbench_moderation_profile.module).
Notice: Undefined property: stdClass::$name in workbench_moderation_profile_form_alter() (line 573 of C:\Bitnami\openatrium-2.23-0\apps\openatrium\htdocs\sites\all\modules\workbench_moderation_profile\workbench_moderation_profile.module).
Notice: Undefined property: stdClass::$id in workbench_moderation_profile_form_alter() (line 573 of C:\Bitnami\openatrium-2.23-0\apps\openatrium\htdocs\sites\all\modules\workbench_moderation_profile\workbench_moderation_profile.module).
Notice: Undefined property: stdClass::$name in workbench_moderation_profile_form_alter() (line 573 of C:\Bitnami\openatrium-2.23-0\apps\openatrium\htdocs\sites\all\modules\workbench_moderation_profile\workbench_moderation_profile.module).
Notice: Undefined property: stdClass::$id in workbench_moderation_profile_form_alter() (line 573 of C:\Bitnami\openatrium-2.23-0\apps\openatrium\htdocs\sites\all\modules\workbench_moderation_profile\workbench_moderation_profile.module).
Notice: Undefined property: stdClass::$name in workbench_moderation_profile_form_alter() (line 573 of C:\Bitnami\openatrium-2.23-0\apps\openatrium\htdocs\sites\all\modules\workbench_moderation_profile\workbench_moderation_profile.module).
Transitions field is required.