[D7] Monfis

I've reviewed and all code looks good however, I would change:

function monfis_menu() {
  $items = array();
  $items['admin/monfis'] = array(
    'title' => 'Monfis',
    'description' => 'Monfis',
    'access callback' => 'monfis_access_callback',
    'page callback' => 'monfis_call_back',
    'type' => MENU_LOCAL_TASK | MENU_NORMAL_ITEM,
    'weight' => 11,
  );
  return $items;
}

to

function monfis_menu() {
  $items = array();
  $items['admin/monfis'] = array(
    'title' => 'Monfis',
    'description' => 'Monfis',
    'access callback' => 'monfis_access_callback',
    'page callback' => 'monfis_call_back',
    'type' => MENU_NORMAL_ITEM,
    'weight' => 11,
  );
  return $items;
}

The reason being you have set a top level admin menu link and the local action tab doesn't fit with this.

Alternatively, you could create a second menu item and use MENU_DEFAULT_LOCAL_ACTION.

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.

Automated Review

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.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows / No: Does not follow] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. (*) Undefined function file_type_load
2. (*) Undefined constant FILE_TYPE_NONE
3. (*) Fatal error after running cron: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'type' cannot be null. See monfis.module:384
4. (*) Use hook_uninstall for removing variables and tables.
5. Use function t() for 'monfis : Sync'
6. Define constants for the numbers in the function monfis_preprocess_table
7. In monfis_theme no need to declare an empty array.
8. Use human-readable variable instead arg1
9. Remove blank lines at the beginning of functions

This review uses the Project Application Review Template.

Hi,

I tried to install the module and found following errors

PHP Fatal error: Uncaught Error: Unsupported operand types in /includes/theme.inc:637\nStack trace:\n#0 /includes/theme.inc(689): _theme_process_registry(Array, 'monfis', 'module', 'monfis', 'sites/all/modul...')\n#1 /includes/theme.inc(325): _theme_build_registry(Object(stdClass), Array, 'phptemplate')\n#2 /includes/theme.inc(277): _theme_load_registry(Object(stdClass), Array, 'phptemplate')\n#3 /includes/theme.inc(411): theme_get_registry()\n#4 /includes/theme.inc(395): ThemeRegistry->initializeRegistry()\n#5 /includes/theme.inc(335): ThemeRegistry->__construct('theme_registry:...', 'cache')\n#6 /includes/theme.inc(277): _theme_load_regist in /includes/theme.inc on line 637, referer: /admin/modules

Automating tools found this https://pareview.sh/node/1230

FILE: /root/repos/pareviewsh/pareview_temp/monfis.module
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------
318 | ERROR | [x] Expected 1 space before "="; 0 found
318 | ERROR | [x] Expected 1 space after "="; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------

Manual review:
I don't found anything wrong. With new drupal install 7.X worked fine.

Hi Drupal8,

Please change the url of config in your monfis.info file to

configure = admin/monfis

the current url -> admin/config/content/monfis does not exist in the monfis_menu.

Along with this comment, the status of the module remains "Needs work"

Hello Drupal8,

Automated Review

Looks good ,, no errors found :)

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md

[No: Does not follow] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[No: Does not follow] the guidelines for project length and complexity.

Secure code

[Yes: Meets the security requirements. / No: List of security issues identified.]

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

(+) Please see the README guidelines .. it looks good but you can do better there.
1. (*) In you hook_menu() .. I can see you are using a callback as your custom permission in hook_permission() monfis_access_callback then user_access to call it.. I know it will work just fine but its unnecessary code since you can just use array('administer monfis') using 'page arguments' it will look like this:
   

   'page arguments' => array('administer monfis'),
   

   instead of this:

   'access callback' => 'monfis_access_callback',
   

   then remove the access callback ..

2. (*) This is similar to the above note .. why not using the hook_menu() to call the drupal_get_form use this in the hook_menu()?:
   

       'page callback' => 'drupal_get_form',
       'page arguments' => array('monfis_content'),
   

   instead of this:

   'page callback' => 'monfis_call_back',
   

   Then using the same thing in extra code?:

   function monfis_call_back() {
     $monfis_form = drupal_get_form('monfis_content');
   

3. (*) In your .module file line:167 ,, I see you are using user_access('administer files') .. which is undefined? can you explain this?
4. (+) In you .module line:365 .. please use t() in watchdog.
5. (+) Not sure about this but in your hook_cron() it runs monfis_run() .. which is a db_insert() .. into your schema .. what if that for somehow fails? are you sure this will not ruin the site running your module? I don't see conditions which control that whether in cron schedule or in the query!
6. (+) Move your hook_help() to the top of your code in .module
7. (+) I'd put my css file in separate folder(e.g: css/monfis.css).

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

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.

Hello Drupal8,

I've pulled your fixes and I think this a RTBC :)

but I still have some points I'd like to mention here but they are NOT blockers:

1. $GLOBALS['base_url'] is a PHP but you can use it the drupal way something like $base_url globally if it were me I'd use base_path()
2. in your .module line:26 default: .. do you really need this empty default case?
3. there is .vcode folder which I believe is hidden and unnecessary in your reop. is this a visual basic json file in there?
4. You should remove LICENSE.txt, it will be added by drupal.org packaging automatically.

I'm putting this as RTBC.

Thank you

Thank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.