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
Comment | File | Size | Author |
---|---|---|---|
#23 | document_library.jpg | 260.42 KB | hwi2 |
#9 | warnings.png | 24.91 KB | er.pushpinderrana |
#9 | XSS.png | 63.02 KB | er.pushpinderrana |
Comments
Comment #1
slydevil CreditAttribution: slydevil commentedComment #2
PA robot CreditAttribution: PA robot commentedThere 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.
Comment #3
slydevil CreditAttribution: slydevil commentedComment #4
slydevil CreditAttribution: slydevil commentedComment #5
slydevil CreditAttribution: slydevil commentedComment #6
robertwb CreditAttribution: robertwb commentedYour 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.
Comment #7
robertwb CreditAttribution: robertwb commentedSorry - When testing for creation of a new document, a received a fatal error:
I can do the other portions of the review, but this would be a pretty big stumbling block.
Comment #8
slydevil CreditAttribution: slydevil commentedrobertwb, 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?
Comment #9
er.pushpinderrana CreditAttribution: er.pushpinderrana commented@slydevil, thanks to you for contribution.
Automated Review
Best practice issues identified by pareview.sh / drupalcs / coder.None.
Manual Review
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.
In following code you are assigning user input value directly to session variable:
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 :).It should be sanitized before using further, as this thing is repeating at number of places in your code that need to be fix.
Other than this, I am also getting following notices:
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
document_library_admin_settings()
form in separateadmin.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!
Comment #10
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedSome more points I found during manual review of code.
document_library.css
anddocument_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.Comment #11
robertwb CreditAttribution: robertwb commented@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:
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.
Comment #12
robertwb CreditAttribution: robertwb commented@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.
Comment #13
slydevil CreditAttribution: slydevil commentedComment #14
slydevil CreditAttribution: slydevil commentedrobertwb, 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.
Comment #15
slydevil CreditAttribution: slydevil commenteder.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.
Comment #16
slydevil CreditAttribution: slydevil commentedComment #17
mpdonadioAssigning to myself for next review. This is a big module, will take me a little while.
Comment #18
mpdonadioAutomated Review
Review of the 7.x-1.x branch (commit 1ddc67e):
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
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.
(+) 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.
Comment #19
slydevil CreditAttribution: slydevil commentedThanks for pointing everything out. I will fix this afternoon if I get the chance and resubmit.
Comment #20
slydevil CreditAttribution: slydevil commentedThese 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.
Comment #21
slydevil CreditAttribution: slydevil commentedHelp has been added.
Comment #22
slydevil CreditAttribution: slydevil commentedComment #23
hwi2 CreditAttribution: hwi2 commentedHey 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:
Love the idea!
Bruce
Comment #24
mpdonadioSorry for the delay.
Automated Review
Review of the 7.x-1.x branch (commit c78ea90):
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.
Comment #25
slydevil CreditAttribution: slydevil commentedhwi2, I've addressed your issues. Good catch on the links and the number field.
Comment #26
slydevil CreditAttribution: slydevil commentedmpdonadio, 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
Comment #27
slydevil CreditAttribution: slydevil commentedComment #28
mccrodp CreditAttribution: mccrodp commentedSome 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):
Manual Review
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).
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.
Comment #29
slydevil CreditAttribution: slydevil commentedmccrodp, 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!
Comment #30
slydevil CreditAttribution: slydevil commentedComment #31
mpdonadioAssigning to myself for next review.
Comment #32
klausiRemoving 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.
Comment #33
mpdonadioAutomated Review
Review of the 7.x-1.x branch (commit 8d286dd):
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.
Comment #34
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedSorry for the delay!
Automated Review
Review of the 7.x-1.x branch (commit 8d286dd):
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 beconfigure = 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
It should be:
(+) document_library_help(): Wrong placeholder (
%url
) on line number 158, need to fix it.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.
Comment #35
slydevil CreditAttribution: slydevil commentedThanks everyone for taking the time to review this module. Much appreciated!
Comment #36
er.pushpinderrana CreditAttribution: er.pushpinderrana commented@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.
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.
Comment #37
slydevil CreditAttribution: slydevil commenteder.pushpinderrana, I have addressed all of your issues from #36 and #34. Please take another look.
Comment #38
slydevil CreditAttribution: slydevil commentedComment #39
madhusudanmca CreditAttribution: madhusudanmca commentedHi 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!!
Comment #40
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedAutomated Review
Review of the 7.x-1.x branch (commit 58b3421):
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.