New module to allow site administrators to create a library of documents. Document content type can be fully tailored to accommodate any type of field that a user needs. Any taxonomy fields that are added to the content type are automatically exposed as filters on the front end (if filtering is enabled and if that specific field is enabled as a filter). Searching and sorting are also available and are configurable through the Drupal back end. Layout of document results is controlled by a template file that can be overridden by a theme. Searching is done using Drupal's search index.

Designed for a public facing library of documents.

Similar in concept to https://www.drupal.org/project/filedepot but less complex.

Project: https://www.drupal.org/sandbox/slydevil/2056487
Git: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/slydevil/2056487.git document_library

This is my first publicly contributed module. All other developed modules are proprietary or were not designed in a way to be shared (they served a very specific purpose).

I have contributed a patch to the constant contact module: https://www.drupal.org/project/constant_contact and the issue that I resolved: https://www.drupal.org/node/1929218

I am active on Stack Exchange: http://drupal.stackexchange.com/users/20027/scott-joudry

This project is currently in use on 3 sites that I know of.

Manual reviews of other projects:
https://www.drupal.org/node/2205693#comment-9059511

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

slydevil’s picture

Issue summary: View changes
PA robot’s picture

Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxslydevil2056487git

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.

slydevil’s picture

Issue summary: View changes
slydevil’s picture

Issue summary: View changes
slydevil’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +PAreview: review bonus
robertwb’s picture

Your git command is requesting me to input a password to pull a snapshot of the repository. None of the other review projects that I have looked at do this. I believe that you need to change your git link in the application to: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/slydevil/2056487.git document_library

In order to eliminate this impediment to review.

robertwb’s picture

Sorry - When testing for creation of a new document, a received a fatal error:


PHP Fatal error:  Cannot unset string offsets in /var/www/html/d.geojoin/modules/field/field.default.inc on line 41, referer: http://deq3.bse.vt.edu/d.geojoin/?q=node/add/document-library-document

I can do the other portions of the review, but this would be a pretty big stumbling block.

slydevil’s picture

robertwb, I was able to clone the repo no problem, are you still having an issue?

As far as the fatal error goes, I am unable to reproduce. I have the module running on 7.26. Are you able to add any other content types?

er.pushpinderrana’s picture

Status: Needs review » Needs work
Issue tags: -PAreview: review bonus +PAReview: review bonus PAReview: security
FileSize
63.02 KB
24.91 KB

@slydevil, thanks to you for contribution.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder.None.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Not Sure: Does not cause module duplication and fragmentation. Can you follow the steps in the link and confirm that the functionality contained in this module should stay separate from filedepot. Also your project page contains small information, it would be better to mention all differences with filedepot on your project page.
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
(*) No. If "no", list security issues identified.
  1. document_library_block() and document_library_folder_block(): this is vulnerable to XSS exploits. If I enter
    alert('XSS');

    in the admin settings form then I will get a nasty javascript popup when the block is displayed. You need to sanitize user provided text before printing, make sure to read https://www.drupal.org/node/28984 again.

    XSS

  2. In following code you are assigning user input value directly to session variable:
    $_SESSION["DOCUMENT_LIBRARY_QUERY"] = $form_state["values"]["query"];
    

    It should be sanitized before using further, as this thing is repeating at number of places in your code that need to be fix.

    Thanks to Klausi for correcting me here as this doesn't come under security issue. But personally I still guess since $_SESSION is a PHP artifact, not something from Drupal, so be more proactive before using this :).
Coding style & Drupal API usage
  1. (*) I assigned Document Library Block and Document Library Folder Block block at one page, getting following warnings:

    warnings

    Other than this, I am also getting following notices:

    Notice: Undefined index: DOCUMENT_LIBRARY_FIELDS in _document_library_build_link() (line 1331 of E:\xampp\htdocs\drupal730\sites\all\modules\2056487\document_library.module).
    Warning: Invalid argument supplied for foreach() in _document_library_build_link() (line 1331 of E:\xampp\htdocs\drupal730\sites\all\modules\2056487\document_library.module).
    
  2. hook_help() contains only single line doc, it should be little bit longer.
  3. (*) I tried to create content for Document Library Document type and get following fatal error after submit.
    http://localhost/drupal730/node/add/document-library-document

    Fatal error: Cannot unset string offsets in E:\xampp\htdocs\drupal730\modules\field\field.default.inc on line 41

  4. (+) Your module file contains 1484 lines code that makes this file so complex, it can be reduced to keep some function in separate files. For example you should keep document_library_admin_settings() form in separate admin.inc file.

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.

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

Thanks again for all your contribution!

er.pushpinderrana’s picture

Issue tags: -PAReview: review bonus PAReview: security +PAreview: review bonus, +PAreview: security

Some more points I found during manual review of code.

  1. You have added document_library.css and document_library.js using .info file. Due to this, both will added on all the pages rather use #attached to add the JS/CSS file to the form render array or page render array where these actually required.
  2. Use Drupal.behaviors, not jQuery(document).ready() in your js file.
  3. document_library_block: this is also vulnerable to XSS. All variable comes form user provided input, so you need to sanitize it before passing into your js file.
robertwb’s picture

@slydevil - I am pretty sure that your URL given above is not formed properly or at least in a non-standard way. If you go to your sandbox page you will see the proper form of the URL listed there for you:

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/slydevil/2056487.git document_library 

The ":sandbox" in the URL that you have listed in this application may be interpreted as a port request by some installs of git. Regardless of why it causes me a problem and not a problem for you, I think that the Git address that you list here should be identical to the one that the system shows on the "Version Control" tab of your sandbox page. This URL may very well be causing others to not be able to install, and therefore not review the module.

robertwb’s picture

@slydevil - I am having no troubles adding other content types, only Document Library -- running on 7.27 -- For what it's worth, I will look at my system to see if I have some other configuration issue -- I will check to make sure that I have upload perms set on the files directory later. You may wish to test on 7.27 while you're at it though.

slydevil’s picture

Issue summary: View changes
slydevil’s picture

robertwb, I've updated the git clone url. If I am logged in to drupal.org, I get a different URL than if I'm not logged in which is where the confusion happened.

slydevil’s picture

er.pushpinderrana, I've addressed the issues with the code.

- xss should no longer be an issue
- all errors and warnings have been addressed
- separated out user and admin functions into appropriate inc files.

As far as File Depot is concerned, it's is meant to be an organized interface for users to upload and share documents. The Document Library is meant to be a listing of documents that cannot be modified by anyone other than a content manager.

slydevil’s picture

Status: Needs work » Needs review
mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Assigning to myself for next review. This is a big module, will take me a little while.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs review » Needs work

Automated Review

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

  • 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. I think this is adequately addressed.
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
No. If "no", list security issues identified.

All queries against the {node} table should have an ->addTag('node_access') on them. _document_library_retrieve_data() may reveal links to nodes that have access permissions on them. See https://api.drupal.org/api/drupal/modules!node!node.module/group/node_ac...

In the template, you are direclty printing a $term->name. This is user input and needs to be plained.

Coding style & Drupal API usage

(+) Why aren't you using a proper Drupal behavior? See https://www.drupal.org/node/756722

Typically, you don't variable_set() in a hook_enable(), you just use default values.

(+) I think all of those descriptions in your field_create_instance() need to be run through t();

In general, single quoted strings are preferred over double quoted ones per Drupal coding standards.

In _document_library_document_add_published_date_field(), you have a hardcoded URL that won't work if Drupal isn't installed in the root directory.

In _document_library_create_vocabularies(), don't use t() on the object you save.

(+) Repalce the placeholder help :)

(*) In document_library_block_info, you don't need the check_plain here; it's not going to output. This happens a lot in the module. See https://www.drupal.org/node/28984

In document_library_admin_settings(), don't use sprintf(), just use placeholders with t().

JS settings get escaped, so you don't need to plain them here (but test this with simple XSS to be sure).

document_library_block() should return a proper render array instead of markup.

_document_library_form_url_filtered(), don't access $_SERVER["QUERY_STRING"] directly.

_document_library_build_link(), _document_library_folders_recursive(), don't build the URL that way use drupal_http_build_query().

(+) The template file has way too much logic in it. Add a prepreocess function to set the variables so you are just printing simple variables, or looping over an array to print.

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.

To be honest, I had a hard time tracing things out. The direct use of the URL query string, and session make it look like you are bypassing the Form API to accomplish your tasks (which would be a pretty big issue). Can you explain what this is doing and why you need to do it?

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.

slydevil’s picture

Thanks for pointing everything out. I will fix this afternoon if I get the chance and resubmit.

slydevil’s picture

These have all been addressed:

All queries against the {node} table should have an ->addTag('node_access') on them. _document_library_retrieve_data() may reveal links to nodes that have access permissions on them. See https://api.drupal.org/api/drupal/modules!node!node.module/group/node_ac...

In the template, you are direclty printing a $term->name. This is user input and needs to be plained.

(+) Why aren't you using a proper Drupal behavior? See https://www.drupal.org/node/756722

Typically, you don't variable_set() in a hook_enable(), you just use default values.

(+) I think all of those descriptions in your field_create_instance() need to be run through t();

In general, single quoted strings are preferred over double quoted ones per Drupal coding standards.

In _document_library_document_add_published_date_field(), you have a hardcoded URL that won't work if Drupal isn't installed in the root directory.

In _document_library_create_vocabularies(), don't use t() on the object you save.

(*) In document_library_block_info, you don't need the check_plain here; it's not going to output. This happens a lot in the module. See https://www.drupal.org/node/28984

In document_library_admin_settings(), don't use sprintf(), just use placeholders with t().

JS settings get escaped, so you don't need to plain them here (but test this with simple XSS to be sure).

document_library_block() should return a proper render array instead of markup.

_document_library_form_url_filtered(), don't access $_SERVER["QUERY_STRING"] directly.

_document_library_build_link(), _document_library_folders_recursive(), don't build the URL that way use drupal_http_build_query().

(+) The template file has way too much logic in it. Add a prepreocess function to set the variables so you are just printing simple variables, or looping over an array to print.

To be honest, I had a hard time tracing things out. The direct use of the URL query string, and session make it look like you are bypassing the Form API to accomplish your tasks (which would be a pretty big issue). Can you explain what this is doing and why you need to do it?

Regarding this, I'll explain - I've updated this to combine the two methods into a single $state object. The session is used to store variables from the search/sort and filter forms. The query string is for the permalinks and folder block. So, both are required.

The only thing left is the help, which I should have done today. Stay tuned.

slydevil’s picture

Help has been added.

slydevil’s picture

Status: Needs work » Needs review
hwi2’s picture

FileSize
260.42 KB

Hey SlyDevil,
Great idea for a module. I could have used this a year ago on a large Drupal website, so this module will be popular once it goes live.

Issues I Found:

  1. When uploading a document, allow for .TXT. Some of the documents I upload at .TXT files, but also provide support for DOCX, XLSX, RTF, CSV and other MS Office-related file extensions as I see you have PPT, DOC, XLS, so your file extension list needs updating for more current versions of MS Office extensions
  2. Validation Error: I entered "asd" as the number of pages and it did not correct me. I need to see an error that tells me to enter a number instead of a string.
  3. Help File at admin/help/document_library has a link that is incorrect: Linking to configuration interface takes me to the Help Main Screen, not the config page.
  4. Also, in help file, the #1 link under Set-Up (Configure your Document Library Document Content Type.) is wrong and does not take me to the manage fields page

Love the idea!
Bruce

mpdonadio’s picture

Status: Needs review » Needs work

Sorry for the delay.

Automated Review

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

  • 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

Too many changes b/c the string changes to just read the diff on this.

The JS behavior should use the context that gets passed in to scope jQuery selectors.

Using #attached with render arrays is preferred over drupal_add_js() drupal_add_css().

(+) In _document_library_document_add_language_field, you have a hardcoded URL in a translated string. The HTML should come out of this string, and pass the URL via a placeholder. This happens a few times. See https://api.drupal.org/api/drupal/includes%21common.inc/function/l/7

You have some instances where you bang into the $node to access field values. Look into field_get_items(), or even the Entity API.

You may want to check out format_size() for printing filesizes.

I still think you can eventually refactor this to get rid of $_SESSION.

(*) You missed a check_plain() on the #title in document_library_search_form() on line 185.

(*) You missed a check_plain() on the #title in document_library_sort_form() on line 260.

(*) You missed a check_plain() on the #title in document_library_filter_form() on line 316.

The three missing plains are the only blockers I saw.

slydevil’s picture

hwi2, I've addressed your issues. Good catch on the links and the number field.

slydevil’s picture

mpdonadio, I've addressed all of your issues except for:

You have some instances where you bang into the $node to access field values. Look into field_get_items(), or even the Entity API.

I still think you can eventually refactor this to get rid of $_SESSION.

Both of these can be done later

slydevil’s picture

Status: Needs work » Needs review
mccrodp’s picture

Status: Needs review » Needs work

Some very minor comments and suggestions and some that need work. Code looks good to me from looking through it though. Good job.

Automated Review

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

  • 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 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
No: Missing some 'required' sections from the guidelines for in-project documentation's recommended README Template.
As it is currently relatively short it is not required, but in my opinion a "table of contents" always looks good, as shown in the README Template. The README will only expand with age so may be useful for consistency with future edits.
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
  1. (*) I used minimal install profile, then I got the following output/error when using drush to install:
    drush en document_library
    The following projects have unmet dependencies:                                                                                                              [ok]
    document_library requires pathauto, date
    Would you like to download them? (y/n): y
    Project pathauto (7.x-1.2) downloaded to sites/all/modules/pathauto.                                                                                         [success]
    Project date (7.x-2.8) downloaded to sites/all/modules/date.                                                                                                 [success]
    Project date contains 11 modules: date_views, date_tools, date_repeat_field, date_repeat, date_popup, date_migrate_example, date_migrate, date_context, date_api, date_all_day, date.
    The following extensions will be enabled: document_library, options, taxonomy, search, token, path, pathauto, number, date_api, date
    Do you really want to continue? (y/n): y
    WD php: FieldException: Attempt to create a field of unknown type image. in field_create_field() (line 110 of                                                [error]
    /Applications/MAMP/htdocs/test2.local/modules/field/field.crud.inc).
    Cannot modify header information - headers already sent by (output started at /Users/paulmccrodden/.composer/vendor/drush/drush/includes/output.inc:38)      [warning]
    bootstrap.inc:1224
    FieldException: Attempt to create a field of unknown type <em class="placeholder">image</em>. in field_create_field() (line 110 of /Applications/MAMP/htdocs/test2.local/modules/field/field.crud.inc).
    Drush command terminated abnormally due to an unrecoverable error.
  2. (*) I also got this using the UI on minimal install profile. It looks like you are missing some dependencies such as image module. FieldException: Attempt to create a field of unknown type <em class="placeholder">image</em>. in field_create_field() (line 110 of /Applications/MAMP/htdocs/test2.local/modules/field/field.crud.inc).
  3. I liked the information on disable "Document Library successfully disabled. Digital Document content type, taxonomies and url aliases have not been removed." However it is not displayed that these items have been removed on uninstall. Perhaps add similar uninstall message: "Document Library successfully uninstalled. Digital Document content type, taxonomies and url aliases have now been removed."
  4. Spelling error in line 669 for vocabularies, spelt 'vacobularies' in document_library.user.inc
  5. Drupal core uses hyphens as URL separators and most modules do. i.e. - I see document_library in your URLs.
    This seems to be a personal preference, hopefully D8 has more of a standard on this. I can't find a relevant issue to link.

This review uses the Project Application Review Template.

slydevil’s picture

mccrodp, I've updated the README to more closely follow the template.

As far as coding style goes:

1. updated dependency list
2. same fix for 1.
3. The module doesn't remove content type or vocabularies, so no messaging required.
4. addressed
5. updated.

Thanks!

slydevil’s picture

Status: Needs work » Needs review
mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Assigning to myself for next review.

klausi’s picture

Issue summary: View changes
Issue tags: -PAreview: review bonus

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

mpdonadio’s picture

Assigned: mpdonadio » er.pushpinderrana
Status: Needs review » Reviewed & tested by the community

Automated Review

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

  • 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

The README looks better. Nice work.

You changed the way some links are used in translated strings. They were better before. Pass in the url and not the whole link. See https://api.drupal.org/api/drupal/includes%21common.inc/function/l/7

My blocking issues (the three missing check plains have been addressed).

Nothing major jumped out at me when I read `git diff c78ea90..`

You have some places where you directly poke into or manipulate $node objects. Think about refactoring these to use the Field API (eg, field_get_items) or the Entity API (eg, entity_metadata_wrapper).

I am not seeing any blocking issues. Assigning to er.pushpinderrana for a second look if he has time.

er.pushpinderrana’s picture

Assigned: er.pushpinderrana » Unassigned
Status: Reviewed & tested by the community » Fixed

Sorry for the delay!

Automated Review

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

  • 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

At module page configure link is wrong configure = admin/config/media/document_library, it should be configure = admin/config/media/document-library

At some places you are using date function, IMHO Instead of using date(), should use format_date().

(+) document_library_preprocess_document_library_document(): Fixed following code on line number 93, wrong if statement case. Also putting comment with every if statement would be useful, as these are huge.
Instead of

if (isset($node->field_library_document_language[$node->language])) {
    foreach ($node->field_library_document_tags[$node->language] as $term) {
	...
	}
}

It should be:

if (isset($node->field_library_document_tags[$node->language])) {
    foreach ($node->field_library_document_tags[$node->language] as $term) {
	...
	}
}

(+) document_library_help(): Wrong placeholder (%url) on line number 158, need to fix it.

$output .= '<li>' . t(
        "After modifying the fields of the Document Library Document Content Type, you will need to update the theme template that is used to render the output. To do so, you will need to copy sites/all/modules/document_library/document_library_document.tpl.php into your theme's template folder. Once you have copied the file, you will need to modify the variables that are added to the template using !url. In your theme it will be named THEME_preprocess_document_library_document.",
        array(
          '%url' => l(t('hook_preprocess_HOOK'), 'https://api.drupal.org/api/drupal/modules!system!theme.api.php/function/hook_preprocess_HOOK/7'))
        );

At number of places, you are using implode() and explode(), drupal_explode_tags() and drupal_implode_tags can be a better option.

Instead of using strlen(), better to use drupal_strlen().

_document_library_build_query(): Please add comment about switch case like case 4, 3 and 2.

document_library.js: Ending semicolon missing in end of Drupal.behaviors.documentlibrary = {. See https://www.drupal.org/node/304258.

(+) _document_library_sort_field(): In db_query() you are directly using LIKE operator, better to use db_like() here to make sure correct escaping.

(+) document_library_admin_settings(): You should validate "Number of Results Per Page" field for numeric field only, as accepting string value doesn't make any sense.

Not seeing any blocking issues. Comments and function docs looks good to me.

_document_library_calculate_trail_recursive(): Nice recursive function. Good Work!

Thanks for your contribution, slydevil!

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:

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.

slydevil’s picture

Thanks everyone for taking the time to review this module. Much appreciated!

er.pushpinderrana’s picture

Status: Fixed » Needs work

@slydevil, sorry for reopening this issue. I missed one security issue that need to be address in high priority before promoting any module to full release. I have revoked your project permission as security issues consider as release blocker.

<?php if (isset($document_body)): ?>
          <div class='document-library-right-body'><?php print $document_body; ?></div>
        <?php endif; ?>

Above code is vulnerable to XSS exploits.You missed a check_plain() before printing this field text, even I also missed during my review (:.

If I enter <script>alert('XSS');</script> in the Description field, every time a nasty javascript popup come. You need to sanitize this before rendering, make sure to read https://www.drupal.org/node/28984 again.Also check the same for other places too in your code.

Sorry for inconvenience.

slydevil’s picture

er.pushpinderrana, I have addressed all of your issues from #36 and #34. Please take another look.

slydevil’s picture

Status: Needs work » Needs review
madhusudanmca’s picture

Status: Needs review » Reviewed & tested by the community

Hi slydevil,

Thanks for your contribution!!

This is a very nice and useful module and working as expected. I don't see any blockers for this so moving it to RTBC.

Thanks!!

er.pushpinderrana’s picture

Status: Reviewed & tested by the community » Fixed

Automated Review

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

  • 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

I again gone through your code as this module is huge, took a extra while to finish with my review.

My blocking issues have been addressed including other recommendations mentioned in #34 also considered except following one.

(+) document_library_admin_settings(): You should validate "Number of Results Per Page" field for numeric field only, as accepting string value doesn't make any sense.

But this is not a blocking issue. I also tested module functionality w.r.t XSS, CSRF and Sql Injection and it worked as expected. Good Job!

Thanks for your contribution, slydevil!

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:

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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.