[D7] State Transition Control

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi @mxr576
I tried your module and find some issues.

・README.txt is missing.
https://drupal.org/node/161085

・Following messages is shown when save field setting.
Try to make 'Allowed values list' empty and save.

Notice: Undefined index: field_workflow_control_fieldset in field_workflow_control_form_field_ui_field_edit_form_validate() (line 70 of /var/www/drupal7/sites/all/modules/field_workflow_control/field_workflow_control.module).
Warning: Invalid argument supplied for foreach() in field_workflow_control_form_field_ui_field_edit_form_validate() (line 70 of /var/www/drupal7/sites/all/modules/field_workflow_control/field_workflow_control.module).

・Undesired item is shown.
Allowed values list:
0|State 0
1|State 1
2|State 2

Workflow control settings:
State 0: State 0, State 1
State 1: State 1, State 2
State 2: State 2

Edit node, set field value to 'State 2' and save.
Edit node again, only 'State 2' should be shown but 'State 0' is also selectable.

Outside of the comment above, I ran this through PAreview.sh (http://pareview.sh/pareview/httpgitdrupalorgsandboxmxr5762150643git). As far as code review goes, it looks like you're basically good to go, but for two things: 1) There is still a master branch, you should switch default to 7.x-1x and delete the master branch. 2) Do you need to extract the keys in the foreach under field_workflow_control_features_export_alter? Having the keys doesn't affect the functionality one way or another, but it does leave two unused variables, which is odd.

...Nice job!

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

@mxr576, thanks for your contribution.

I saw your module and also installed and tested functionality, working as expected. Its looks like a great improvement from your previous code. Good Work!

Your git command is not correct, it should be

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/mxr576/2150643.git

Automated Review:
http://pareview.sh/pareview/httpgitdrupalorgsandboxmxr5762150643git reported following two minor issues.

FILE: /var/www/drupal-7-pareview/pareview_temp/field_workflow_control.install
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
5 | ERROR | Doc comment short description must be on a single line, further
| | text should be a separate paragraph
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/field_workflow_control.module
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
5 | ERROR | Doc comment short description must be on a single line, further
| | text should be a separate paragraph
196 | ERROR | Missing parameter type
--------------------------------------------------------------------------------

Manual Review:
1. In Readme.txt file some special character is coming in following line, fix it.
If you didn’t set any transition for value than all values can be selected
2. Your module name is field_workflow_control that sound like it is something similar to Workflow module but in your case you are just managing field values as a states. There can be a better name for this module, something related to states as this is my personal opinion, might be other people find this name good :).
3. hook_help() function is missing in your module.
4. Inline comments looks good in your code.

Certainly your module looks good to me.

Thanks again!

Hey,

For line 214 - you just need to define that the $all_values parameter accepts only an array. So: function _field_workflow_control_get_selected_values(array $all_values) {...}

That should do the trick.

For more on type hinting: http://php.net/manual/en/language.oop5.typehinting.php

Looks better module name this time.

Automated Review

PAReview came up clean.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No multiple applications
Yes: No multiple application found.
No duplication
Yes: Not found any similar project.
Master Branch
Yes: Follows the guidelines for master branch.
3rd party code
Yes: Follows the guidelines for 3rd party code.
README.txt
No. The format of README.txt does not follow the guidelines for in-project documentation. In particular, see the README.txt Template.
Code long/complex enough for review
Yes. About 300 lines. Follows the guidelines for project length and complexity.
Secure code
Yes.
Coding style & Drupal API usage

1. Your code looks good to me.
2. It would be good, if you extend Project page little bit more.
3. Similarly Readme.txt file need to be enhanced.
4. Inline comments looks good to me.

It looks RTBC from my side.

As I am not a git administrator, so I would recommend you, 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 :-)

Review of the 7.x-1.x branch (commit 156668e):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

manual review:

1. The Git commits are not connected to your user account. You need to specify an email address. See https://www.drupal.org/node/1022156 and https://www.drupal.org/node/1051722
2. Users can easily setup workflows with https://www.drupal.org/project/rules or https://www.drupal.org/project/workbench . Please describe the differences to existing projects on your project page and follow https://www.drupal.org/node/997024 .
3. state_transition_control_install(): why do you increase the module weight, that should be documented in the comment. Also, juggling with module weights can be unreliable, so I would recommend to use hook_module_implements_alter() if you need to run before/after a specific other hook implementation.
4. _state_transition_control_field_edit_form_ajax_callback(): why is this function needed if it doesn't do anything? Please add a comment.
5. _state_transition_control_modify_existing_form_add_checkboxes(): this is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as possible field value then I will get a nasty javascript popup. You need to sanitize user provided text before printing. In your case you must use an "@" placeholder with t(). Make sure to read https://www.drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

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

@mxr576, you can not switch status to RTBC yourself, moving back to Needs Review :).

Also I would recommend you, please help to review other project applications to get a review bonus back. This will put you on the high priority list, then git administrators will take a look at your project right away :)

Thanks to you for your hard work and patience.

Seems like you fixed all the issues found by the previous review and module works just like it is described.

I just have one usability suggestion: when selecting possible transitions on field settings page, it would make sence to make mandatory transition (from "state 1" to "state 1") checkboxes "readonly" so user have no chance to deselect them by mistake.

Review of the 7.x-1.x branch (commit e50ac33):

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

  FILE: /home/klausi/pareview_temp/state_transition_control.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   112 | ERROR | [x] Expected 1 space(s) before asterisk; 0 found
  --------------------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  --------------------------------------------------------------------------------
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

manual review:

1. _state_transition_control_modify_existing_form_add_checkboxes(): there is still one "!value" in the translatable string which needs to be replaced.
2. state_transition_control_form_field_ui_field_edit_form_validate(): this is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as possible field value then I will get a nasty javascript popup as soon as the form error triggers. You need to sanitize user provided text before printing. In your case you must use an "@" placeholder with t(). Make sure to read https://www.drupal.org/node/28984 again.

Looks RTBC to me now after a manual review.

Assigning to mpdonadio as he might have time to take a final look at this.

Automated Review

Review of the 7.x-1.x branch (commit 3c67961):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

Not a fan of using explicit strong elements for the descriptions. Let the theme handle this.

The code is pretty dense. It could use a little vertical whitespace between logical breaks.

Will this be translation safe with all of the LANGUAGE_NONE usage? Double check this.

Not seeing any blockers here. Placeholders all look appropriate.

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

This review uses the Project Application Review Template.

Thanks for your contribution, mxr576!

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

• 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.