CMIS Views

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Do not use t() in hook_schema(), this will only generate overhead for translators.
  

          'description' => t("CMIS View ID"),
          'description' => t('The unique name of the view'),
          'description' => t('The title of the CMIS view page'),
          'description' => t('Does the CMIS view have a block 0=no, 1=yes'),
          'description' => t('In the query CMIS or file location based'),
          'description' => t('Format the results'),
          'description' => t('The unique name of the view'),
          'description' => t('The unique name of the view'),
          'description' => t('The unique name of the view'),
  

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See http://ventral.org/pareview/httpgitdrupalorgsandboxiannorton1270012git.

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. Go and review some other project applications, so we can get back to yours sooner.

If you got any questions on that please ask!

Please fix your formatting, more detailed reviews will follow.

I did a quick scan of the module files:

- in git try using 7.x-1.x as your main working branch for 7.x initially, because branch names like 7.x are not supported by the release management here on drupal.org
- in your hook_menu you define a form array, i suspect this is leftover code, but should be removed regardless
- also in the hook menu you try to include a file from the cmis_browser module called cmis_browser.content_get.inc, why?
- when you do a hook_FORM_ID_form_alter the third argument (the form id) is not going to be there (line 95)
- its useful to add a description to the permissions
- hook_theme does not accept a variables parameter and try to define the default variable keys when registering the theme function
- you include a lot of files from other modules, but you don't depend on them in the info file (e.g. cmis_views_info())
- you use camel case variables even when not in concjuncture with other APIs, e.g. $repoId. This is not in line with drupals coding standard as people highlighted above.
- using the if statement to check for a number of options is better done via switch().
- since you set the $content to NULL just before that if (line 201), why do you concatenate?
- you say implements hook_block_load, but then create a function cmis_views_load() (line 222)
- I would also tweak that function to just return the result of the query, it will be null when no records found anyway, so no need for the extra logic to test for the result
- on line 256 'info' => t('CMIS view: ' . $data->cmisviewname): incorrect usage of t(), use placeholders (%, @). This would likely allow some XSS attacks
- I would use template files in addition to the theme functions, delegate the business logic of preparing the variables in the theme or preprocess function, and just print the HTML and the variables in the template. Right now you have too much HTML for my taste in code.
- js file seems to be missing a var from path = $(this).dialog("option", "title"); on line 7
- in the admin include file you define a submit callback for the settings form on line 103, but there is no such callback function.
- line 58 in .module file: when using confirmation forms you can simply use drupal_get_form as the page callback and the name of your form definition as page argument instead of defining a page callback that only performs a drupal_get_form.

I personally dont really like the query structure, but i don't know much about cmis_query. Probably there is no other way to define a query.

Overall I think the code looks good, its clear that the author is making an effort to follow drupals coding standards. I know that the above list might look a bit daunting, but most of them are simplifications in terms of code complexity. The author has a good grasp of Drupal APIs and uses them accordingly.

Note: I haven't had the chance to actually test the module functionally.

tagging per

- on line 256 'info' => t('CMIS view: ' . $data->cmisviewname): incorrect usage of t(), use placeholders (%, @). This would likely allow some XSS attacks

Also subscribing.

- Cosmetic issue: the case statements are in one line in cmis_views.utils.inc, it is clearer if you have each in a new line.
- for templates usually file names use hyphens instead of underscores, e.g. cmis_views_bullets.tpl.php
- You have an interesting coding style when it comes to arrays. I generally recommend people to structure their arrays one entry in a row, so instead of

    'cmis_views_table' => array(
      'variables' => array(
        'rows' => NULL, 'repoid' => NULL, 'tableheadings' => array(
          'title' => t('Title'), 'description' => t('Description'),
          'filetype' => t('File type'),
          'lastmodified' => t('Last Modified'), 'size' => t('Size'),
        ),
      ),
      'template' => 'cmis_views_table',
      'path' => drupal_get_path('module', 'cmis_views') . '/templates',
    ),

I would recommend using

    'cmis_views_table' => array(
      'variables' => array(
        'rows' => NULL,
        'repoid' => NULL,
        'tableheadings' => array(
          'title' => t('Title'),
          'description' => t('Description'),
          'filetype' => t('File type'),
          'lastmodified' => t('Last Modified'),
          'size' => t('Size'),
        ),
      ),
      'template' => 'cmis_views_table',
      'path' => drupal_get_path('module', 'cmis_views') . '/templates',
    ),

I think I identified a few potential XSS issues. The users with the 'administer cmis views' permission are able to create and edit the entries in the cmis_views table, therefore entries in this table can potentially contain valid XSS vulnerabilities (its a vulnerability, because the administer cmis views permission is not considered to be a trusted one).

When using links you dont have to assemble the bit after the ?q= manually, you can just pass the rest as an array to url(), using $options['query'] (cmis_views.module:176).

You may want to add your extra module files to the info file (files[] = ...) so Drupal knows about them (this will not mean that they are loaded).

cmis_views.module:279 you set the output on the error, but after this $contents is only returned when there is a $query_result, so if there is an error there is no output. drupal_set_message could be used with an error style message.

Because of this at every location where you use data from this table it should be escaped, using check_plain(), or filter_xss().
- cmis_views.module:336, the $data->cmisviewname is unescaped.
- cmis_views.module:356 and cmis_views.module:363 you take the result of cmis_views_load() which loads data from the blocks and cmis_views tables and uses it as the returned block, this should be escaped.
- cmis_views.module:251 I dont really see how the cmis query is used, but generally concatenating variables into queries is not a good idea, should use a placeholder and filter it.

Sorry to be such a pain again, the module is coming along really nicely, I think once these issues are fixed you should definitely be granted access to create the full module.

Marking rtbc as snufkin has done some solid reviews and the issues have been fixed.

This earlier comment is important:

Overall I think the code looks good, its clear that the author is making an effort to follow drupals coding standards. I know that the above list might look a bit daunting, but most of them are simplifications in terms of code complexity. The author has a good grasp of Drupal APIs and uses them accordingly.

Note: I haven't had the chance to actually test the module functionally.

This is the kind of thing nobody can easily test because it requires setup of a third-party library that is a lot of work, so a visual review is all we can do.

Thanks for your contribution, IanNorton! 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 minde: Commit messages - providing history and credit and Release naming conventions.

I recently tried integrating drupal 7 with alfresco 3.4.d. I had done so successfully wit version 6 of drupal but since 7 had some sync issues, i decided to wait a bit on it. i eventually found a patch on Properties + Versions support (inc. other patches) that fixed the syncing problem with alfresco. The problem i have though now is that i downloaded and enabled the cmis views module and created a cmis view of my alfresco repository.

Bi-directional synchronization is working and the view is working at run time(no need for CRON) but i receive the error below everytime i add or create content.

Notice: Undefined index: cmis:objectTypeId in CMISService->cacheEntryInfo() (line 428 of C:\wamp\www\demo\modules\cmis\cmis_common\lib\cmis_repository_wrapper.php).
Notice: Undefined index: cmis:objectTypeId in CMISService->cacheEntryInfo() (line 428 of C:\wamp\www\demo\modules\cmis\cmis_common\lib\cmis_repository_wrapper.php).
Notice: Undefined index: cmis:objectTypeId in CMISService->cacheEntryInfo() (line 428 of C:\wamp\www\demo\modules\cmis\cmis_common\lib\cmis_repository_wrapper.php).
Notice: Undefined index: cmis:objectTypeId in CMISService->cacheEntryInfo() (line 428 of C:\wamp\www\demo\modules\cmis\cmis_common\lib\cmis_repository_wrapper.php).
Notice: Undefined index: cmis:objectTypeId in CMISService->cacheEntryInfo() (line 428 of C:\wamp\www\demo\modules\cmis\cmis_common\lib\cmis_repository_wrapper.php).

I have a feeling this has something to do with the mapping but i dont know how to fix it. Could anyone please help me ASAP. Any assistance would be greatly appreciated

Regards
Zimsil

@Zimsil,
please open a separate issue in the module's issue queue about this,
as this is the project application issue, we can't help you here.

I have posted the issue as well. You can find it on the "Undefined index: cmis:objectTypeId" Received after adding or creating content page

When querying existing content from Alresco 4.0.d I receive the following error within CMIS Views:

Notice: Undefined index: cmis:objectTypeId in CMISService->cacheEntryInfo() (line 384 of /.../drupal/sites/all/modules/cmis/cmis_common/lib/cmis_repository_wrapper.php).

Can you recommend a fix or anticipate a solution?

@mulligahn - please file a new issue on http://drupal.org/project/cmis_views http://drupal.org/node/add/project-issue/cmis_views